Page MenuHomePhabricator

[libc++] [CI] Validate the output of the generated scripts.
ClosedPublic

Authored by Mordante on Sun, Apr 4, 11:18 AM.

Details

Summary

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.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Mordante requested review of this revision.Sun, Apr 4, 11:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptSun, Apr 4, 11:18 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Mordante planned changes to this revision.Sun, Apr 4, 11:19 AM
Mordante updated this revision to Diff 335160.Sun, Apr 4, 11:30 AM

Explicitly use python3 as interpreter since using env python fails.
Probably since env python tries to find python2.

Mordante planned changes to this revision.Sun, Apr 4, 11:30 AM
Mordante updated this revision to Diff 335163.Sun, Apr 4, 11:37 AM

Fixes the name of the output file. This should fix the false positive.

Mordante updated this revision to Diff 335164.Sun, Apr 4, 11:41 AM

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.

Mordante updated this revision to Diff 335165.Sun, Apr 4, 11:50 AM

The build https://buildkite.com/llvm-project/libcxx-ci/builds/2293 passess as expected.
Restore all removed pipelines to get a complete CI run.

Mordante retitled this revision from [libc++] [DO NOT LAND] Validate status generate_feature_test_macro_components.py to [libc++] Validate the output of generate_feature_test_macro_components.py.Sun, Apr 4, 11:51 AM
Mordante edited the summary of this revision. (Show Details)
curdeius accepted this revision as: curdeius.Sun, Apr 4, 12:13 PM
curdeius added a subscriber: curdeius.

LGTM apart from the comment.

libcxx/utils/ci/run-buildbot
138

Copy-pasta mistake. Please remove this comment.

Quuxplusone added inline comments.
libcxx/utils/ci/run-buildbot
138

How about throwing in the other libcxx/utils/ generators here while you're at it?
python3 libcxx/utils/generate_header_tests.py
This will also give me an obvious place to add generate_header_inclusion_tests.py once we get some reviews on D99309.

curdeius added inline comments.Sun, Apr 4, 2:13 PM
libcxx/utils/ci/run-buildbot
138

+1

Mordante updated this revision to Diff 335248.Mon, Apr 5, 7:03 AM

Test with utils/generate_header_tests.py as suggested in the review comments.
Disable most CI builds to quickly test the new CI job.

Mordante updated this revision to Diff 335251.Mon, Apr 5, 7:08 AM

Test whether the CI build fails as expected.

Mordante updated this revision to Diff 335254.Mon, Apr 5, 7:14 AM

Test with both scripts failing.

Mordante updated this revision to Diff 335257.Mon, Apr 5, 8:07 AM

Test only with a dirty utils/generate_header_tests.py.

Mordante updated this revision to Diff 335258.Mon, Apr 5, 8:15 AM

Attempt to fix the false positive of the previous build.
tab -> spaces

Mordante updated this revision to Diff 335261.Mon, Apr 5, 8:20 AM

The previous build failed as expected. Not true with only utils/generate_header_tests.py dirty.

Mordante updated this revision to Diff 335262.Mon, Apr 5, 8:27 AM

Add all builds again.
Revert the edits to fail the build.
Minor polishing on the code.

Mordante retitled this revision from [libc++] Validate the output of generate_feature_test_macro_components.py to [libc++] [CI] Validate the output of the generated scripts..Mon, Apr 5, 8:29 AM
Mordante edited the summary of this revision. (Show Details)
Mordante marked 3 inline comments as done.Mon, Apr 5, 8:34 AM

I addressed the review comments and the final test build is running.

libcxx/utils/ci/run-buildbot
138

Thanks for the suggestion!
I added libcxx/utils/generate_header_tests.py.
The validation of the output of libcxx/utils/generate_abi_list.py has already been added to the proper CI jobs.

Quuxplusone accepted this revision.Mon, Apr 5, 8:44 AM

LGTM % comment.

libcxx/utils/ci/run-buildbot
137

Nit: /generated scripts/generator scripts/
and maybe /output/outputs/

142

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.patch

This 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.

This revision is now accepted and ready to land.Mon, Apr 5, 8:44 AM
Mordante marked an inline comment as done.Mon, Apr 5, 8:55 AM
Mordante added inline comments.
libcxx/utils/ci/run-buildbot
142

By making two diffs we have one CI artifact per script.
I think that makes it easier to find the culprit.
Do you think the loss of this information is acceptable?

Quuxplusone added inline comments.Mon, Apr 5, 9:01 AM
libcxx/utils/ci/run-buildbot
142

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.

curdeius added inline comments.
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
142

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.

Mordante added inline comments.Tue, Apr 6, 11:32 AM
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
142

I'll change it to one file. @Quuxplusone I'll wait for D99309 to land and include that test in this patch.

Quuxplusone added inline comments.Tue, Apr 6, 12:32 PM
libcxx/utils/ci/run-buildbot
142

D99309 is now landed, so go ahead when you're ready!

Mordante updated this revision to Diff 335828.Wed, Apr 7, 8:58 AM

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.

Mordante updated this revision to Diff 335835.Wed, Apr 7, 9:13 AM

Implement a wait.
The build should fail and stop after the failure.

Looks great. Thanks! And of course don't forget to remove the I-change :)

Mordante updated this revision to Diff 335851.Wed, Apr 7, 9:52 AM

Remove the temporary breakage. The build should now pass again.

Looks great. Thanks! And of course don't forget to remove the I-change :)

Thanks. What do you mean with I-change?

ldionne accepted this revision.Wed, Apr 7, 10:19 AM

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
144

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.

I meant the temporary change to provoke the failure:). That's already reverted from what I see. :)

Quuxplusone accepted this revision.Wed, Apr 7, 11:06 AM

I meant the temporary change to provoke the failure:). That's already reverted from what I see. :)

Yes I reverted that.

libcxx/utils/ci/run-buildbot
144

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.

LGTM as-is, we can try to address the dirtying-working-directory issue by changing the scripts.

libcxx/utils/ci/run-buildbot
144

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!