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

[Mordante]

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.

[Mordante]

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

[Mordante]

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

[Mordante]

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]

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

[curdeius]

LGTM apart from the comment.

libcxx/utils/ci/run-buildbot

138

Copy-pasta mistake. Please remove this comment.

[• Quuxplusone]

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]

libcxx/utils/ci/run-buildbot

138

+1

[Mordante]

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]

Test whether the CI build fails as expected.

[Mordante]

Test with both scripts failing.

[Mordante]

Test only with a dirty utils/generate_header_tests.py.

[Mordante]

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

[Mordante]

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

[Mordante]

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

[Mordante]

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]

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.

[Mordante]

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]

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]

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]

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]

libcxx/utils/ci/run-buildbot

142

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

[Mordante]

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]

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

[curdeius]

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

[Mordante]

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

[Mordante]

In D99862#2674337, @curdeius wrote:

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

Thanks. What do you mean with I-change?

[ldionne]

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.

[curdeius]

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

[Mordante]

In D99862#2674415, @curdeius wrote:

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.

[ldionne]

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!

libcxx/utils/ci/buildkite-pipeline.yml

agents:
       queue: "libcxx-builders"
     retry:
       automatic:
         - exit_status: -1  # Agent was lost
           limit: 2
     soft_fail:
         - exit_status: 1
 
+  - label: "Generated output"
+    command: "libcxx/utils/ci/run-buildbot check-generated-output"
+    artifact_paths:
+      - "**/generated_output.patch"
+    agents:
+      queue: "libcxx-builders"
+    retry:
+      automatic:
+        - exit_status: -1  # Agent was lost
+          limit: 2
+
+  # All jobs defined before this `wait` will run whenever a CI job is started.
+  # Jobs defined after the `wait` will run only if all the jobs above
+  # succeeded. The build failures before this `wait` can be fixed by running
+  # the proper scripts or clang-format. We use this to reduce the load on the
+  # testers and avoid running builds that we know fail anyway.
+  - wait
+
   - label: "C++03"
     command: "libcxx/utils/ci/run-buildbot generic-cxx03"
     artifact_paths:
       - "**/test-results.xml"
       - "**/*.abilist"
     agents:
       queue: "libcxx-builders"
     retry:

libcxx/utils/ci/run-buildbot

git-clang-format \
         --extensions ',h,hh,hpp,hxx,c,cc,cxx,cpp' HEAD~1 \
         -- \
             libcxx/{benchmarks,include,src,test} \
             libcxxabi/{fuzz,include,src,test} \
         | tee ${BUILD_DIR}/clang-format.patch
     # Check if the diff is empty, fail otherwise.
     ! grep -q '^--- a' ${BUILD_DIR}/clang-format.patch
 ;;
+check-generated-output)
+    clean
+    echo "+++ Checking the output of the generated scripts"
+    mkdir -p ${BUILD_DIR}
+    python3 libcxx/utils/generate_feature_test_macro_components.py
+    python3 libcxx/utils/generate_header_inclusion_tests.py
+    python3 libcxx/utils/generate_header_tests.py
+    git diff | tee ${BUILD_DIR}/generated_output.patch
+    # Check if the diffs are empty, fail otherwise.
+    ! grep -q '^--- a' ${BUILD_DIR}/generated_output.patch
+;;
 generic-cxx03)
     export CC=clang
     export CXX=clang++
     clean
     generate-cmake -C "${MONOREPO_ROOT}/libcxx/cmake/caches/Generic-cxx03.cmake"
     check-cxx-cxxabi
     check-abi-list
 ;;