This adds a CI job validating that the output of
utils/generate_feature_test_macro_components.py and
utils/generate_header_tests.py are up to date.
The validation method has been copied from the Format job.
Details
Details
- Reviewers
curdeius • Quuxplusone ldionne - Group Reviewers
Restricted Project - Commits
- rGae103003b219: [libc++] [CI] Validate the output of the generated scripts.
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
Explicitly use python3 as interpreter since using env python fails.
Probably since env python tries to find python2.
Comment Actions
The build https://buildkite.com/llvm-project/libcxx-ci/builds/2292 fails as expected.
This commit reverts the change to utils/generate_feature_test_macro_components.py.
This build should pass.
Comment Actions
The build https://buildkite.com/llvm-project/libcxx-ci/builds/2293 passess as expected.
Restore all removed pipelines to get a complete CI run.
Comment Actions
LGTM apart from the comment.
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 139 | Copy-pasta mistake. Please remove this comment. | |
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 139 | +1 | |
Comment Actions
Test with utils/generate_header_tests.py as suggested in the review comments.
Disable most CI builds to quickly test the new CI job.
Comment Actions
The previous build failed as expected. Not true with only utils/generate_header_tests.py dirty.
Comment Actions
Add all builds again.
Revert the edits to fail the build.
Minor polishing on the code.
Comment Actions
I addressed the review comments and the final test build is running.
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 139 | Thanks for the suggestion! | |
Comment Actions
LGTM % comment.
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 138 | Nit: /generated scripts/generator scripts/ | |
| 143 | I suggest you remove this git reset, and do just one git diff at the end. I.e., python3 libcxx/utils/generate_feature_test_macro_components.py
python3 libcxx/utils/generate_header_tests.py
git diff \
| tee ${BUILD_DIR}/check-generated-output.patch
! grep -q '^--- a' \
${BUILD_DIR}/check-generated-output.patchThis makes it a one-line diff to add a new generator script, as opposed to a 5-line diff (where one line is even separated from the other 4). Personally I'm willing for you to make this change and not re-run all your tedious testing, if you don't want to expend that effort a second time. :) I will undoubtedly make my suggested change in D99309 if you don't make it here. | |
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 143 | By making two diffs we have one CI artifact per script. | |
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 143 | The artifact is still the output of git diff, containing the offending filename right there on the first and second lines, right? IMO that is totally acceptable! :) For fixing bugs, I think I would even prefer to trawl through a single large git diff than, like, two or three different files each containing a smaller git diff. | |
| libcxx/utils/ci/buildkite-pipeline.yml | ||
|---|---|---|
| 42 | Maybe we want to add a wait here, so that the CI doesn't run for nothing when this tests fails. @ldionne, WDYT? | |
| libcxx/utils/ci/run-buildbot | ||
| 143 | Same for me, I prefer a single file. Anyway, the generator scripts change different files and it's easy to know which script hasn't been run. | |
| libcxx/utils/ci/buildkite-pipeline.yml | ||
|---|---|---|
| 42 | Good point! I think that makes sense, especially since our CI runs can take a long time. | |
| libcxx/utils/ci/run-buildbot | ||
| 143 | I'll change it to one file. @Quuxplusone I'll wait for D99309 to land and include that test in this patch. | |
Comment Actions
Rebased on main.
Generate only one diff.
Add utils/generate_header_inclusion_tests.py.
Temporary disable unneeded CI jobs.
Modify utils/generate_header_inclusion_tests.py to fail the build.
Comment Actions
Please wait for CI to finish before you commit this :-). Thanks!
| libcxx/utils/ci/buildkite-pipeline.yml | ||
|---|---|---|
| 42 | Yeah, I guess I'm fine with adding a wait here. We can tweak it if we see it doesn't interact well with the pipeline. | |
| libcxx/utils/ci/run-buildbot | ||
| 145 | It's annoying that we'll be modifying our git index just to run this test, i.e. if it generates changes, then those changes will "pollute" the working directory. I think the cleanest approach to this would be to change the various header/test generation files to behave more like clang-format and to produce a diff that we can then apply instead of making the changes in place, but that would be more work indeed. That way, we could run this test without modifying our local state. That's definitely not blocking for this review, but something to keep in mind. | |
Comment Actions
I meant the temporary change to provoke the failure:). That's already reverted from what I see. :)
Comment Actions
Yes I reverted that.
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 145 | Good to know. I wasn't aware the same checkout was reused by other jobs. If wanted I can add a git reset --hard HEAD after generating the diff. | |
Comment Actions
LGTM as-is, we can try to address the dirtying-working-directory issue by changing the scripts.
| libcxx/utils/ci/run-buildbot | ||
|---|---|---|
| 145 | Jobs do reuse the same checkout, but they use git checkout --force <SHA>, so it'll override any diff created by running this. Don't do git reset --hard HEAD here, as this could cause people to lose work if they run this job locally. I run jobs locally often, not sure if others do! | |
Maybe we want to add a wait here, so that the CI doesn't run for nothing when this tests fails. @ldionne, WDYT?