-
Notifications
You must be signed in to change notification settings - Fork 547
Add ABI compatibility tasks to EVG config (CXX-641, CXX-2746) #1083
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
kevinAlbs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a fix to the GitHub PR tasks. I left other minor suggestsions.
I like the abi-prohibited-symbols task as a safeguard to prevent leaking internal symbols. And the display tasks for grouping the tasks visually.
.evergreen/abi-stability-setup.sh
Outdated
|
|
||
| # Restore all pending changes. | ||
| git -C mongo-cxx-driver reset --hard "HEAD@{1}" | ||
| git -C mongo-cxx-driver stash pop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check if a stash entry exists:
if git -C mongo-cxx-driver stash list | grep -q "stash"; then
# Restore stash if one exists.
git -C mongo-cxx-driver stash pop
fiThis may resolve the abi-compliance-check task failure on the PR:
[2024/01/18 16:20:10.380] No stash entries found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed this patch vs. commit/waterfall build discrepancy. Addressed via a direct test for $is_patch (EVG expansion) to detect when stashing is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like I may have misunderstood the criteria for when a build is considered a patch (includes those triggered by GitHub PRs). Reverted and added a simple || true to the stash pop command to account for the "no stashed changes" scenario.
.evergreen/abi-stability-setup.sh
Outdated
|
|
||
| # Install old (base) to install/old. | ||
| echo "Building old libraries..." | ||
| [[ -d install/old ]] || { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect install/old is not expected to exist since the teardown task runs rm -rf *.
Consider changing to assert: [[ ! -d install/old ]], or removing the [[ -d install/old ]] since it suggests there is an expectation of reuse.
Same suggestion applies to install/new below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftovers from when developing and debugging the setup/teardown task definitions. Will remove.
.evergreen/abidiff-test.sh
Outdated
| declare status | ||
| status='{"status":"failed", "type":"test", "should_continue":true, "desc":"abidiff returned an error for bsoncxx (stable)"}' | ||
| curl -sS -d "${status:?}" -H "Content-Type: application/json" -X POST localhost:2285/task_status || true | ||
| : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove stray :? Same question applies below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from when porting over the HTTP endpoint commands from the abi-compliance-check scripts. Removed as suggested.
kkloberdanz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Looks good!
Description
This PR resolves both CXX-641 and CXX-2746. Verified by this patch.
This PR proposes adding three tasks to the CXX Driver EVG config:
grepto detect symbols that should not be present in the ABI.These three tasks are run by two build variants: one testing the CXX Driver libraries configured to use a polyfill library (currently mnmlstc/core by default), and one configured to use the C++17 standard library.
The abi-stability Task Group
This PR uses an EVG task group to select and run the ABI stability tasks.
A task group allow one to define "pre" and "post" commands unique to the task group in the form of "setup" and "teardown" tasks. These overrule the global "pre" and "post" commands, providing better separation of concerns. On the other hand, tasks in a task group may potentially reuse the same host, which may include whatever contents remain after running the prior task. To avoid unexpected filesystem conflicts and encourage reproducibility, the teardown task simply removes all contents in the default directory with
rm -rf *.The setup task fetches the CXX Driver sources, installs the C Driver, then runs
abi-stability-setup.sh. The setup script requires only one externally-provided environment,cxx_standard, to control selection of the polyfill library viaCMAKE_CXX_STANDARD. Beyond some conveniences such as Ninja generator selection and enabling ccache support, the "base" and "current" state of the CXX Driver repository are built and installed into separate prefixes using the exact same build configuration, with the sole exception of the C++ standard requested. The separate prefixes are then used by the ABI stability tasks for comparison and testing.The "base commit" is the latest tag reachable from the "current commit", obtained via
git describe --tags --abbrev=0. For this PR, this is the 3.9.0 release. The "current commit" is the latest commit in a PR or waterfall build, or the base commit + staged changes in an EVG patch build. This means the generated reports will be comparing the current state of the CXX Driver against the most recent release version (including patch releases, when a patch release tag is reachable from the current commit).The unstable and stable ABIs are tested separately. If a binary-incompatible change is detected for a stable ABI symbol, the task will fail. If a binary-incompatible change is detected for an unstable ABI symbol, the task will not fail. This is to allow generating informative reports for the unstable ABI independent of the state of the stable ABI, e.g. to track ongoing and upcoming work for CXX-2625, CXX-2796, and CXX-2797 (the unstable ABI reports with stdlib should not report incompatibilities once CXX-2796 is resolved).
The polyfill and stdlib configurations are also tested as separate build variants. This is to ensure the build system correctly configures the CXX Driver libraries to use the C++17 standard library features when available, and to ensure our pre-C++17 polyfill library alternatives do not affect post-C++17 compatibility. For example, the ongoing changes made for CXX-2625 are currently reflected in the stdlib abi-compliance-check source compatibility report as high/medium severity issues. Once C++17 support is restored by CXX-2796, this report should return to a much cleaner state.
abi-compliance-check
The
abi-compliance-checkertool requires the set intersection of the symbols being checked to be non-empty: at least one common symbol must be present in the old and new libraries. This means the stable ABI cannot be tested yet due to the set of symbols being empty. Once stable ABI symbols are present in both the current and base commits being compared, the commented-out commands can be enabled: this pending task is tracked by CXX-2812.The HTML compatibility reports describe both binary and source compatibility. The source compatibility reports in particular are expected to be a useful tool independent of ABI compatibility tests and informing the release of minor vs. major versions.
Both the
abi-compliance-checkertool and its requiredctagstool are build from source to obtain suitably recent versions, as the system-provided binary was observed to be susceptible to confusing runtime bugs.abidiff
The
abidifftool provides a very detailed report comparing both definite and indirect potential ABI incompatibility issues. This tool is obtained via the system package manager.abi-prohibited-symbols
This task greps for any symbols in the
bsoncxxormongocxxnamespaces that are also members of adetailortestnamespace. If such a symbol is found, suggesting leak of implementation details or test-specific interfaces into the ABI, this task will fail.Task Status via HTTP Endpoint
The
curlcommands in the*-test.shscripts is to allow for the followings3.putcommands to upload the generated reports before marking the task as failed. Normally, when a command within a task fails, the remaining commands are not executed; this is to circumvent that usual behavior. If this is seen as too confusing or complicated to maintain, a[[ -f error_occurred.txt ]]check after thes3.putcommands can be used instead.Display Tasks
The "display tasks" EVG feature is used to reduce verbosity of the EVG results page. This also allows for the uploaded files to be conveniently grouped by display task, e.g. see all the reports for the polyfill and stdlib variant tasks each on a single page.