[libcxx-commits] [PATCH] D99862: [libc++] [CI] Validate the output of the generated scripts.

[Mark de Wever via Phabricator via libcxx-commits]

Mordante added inline comments.


================
Comment at: libcxx/utils/ci/buildkite-pipeline.yml:42
+          limit: 2
+
   - label: "C++03"
----------------
curdeius wrote:
> Maybe we want to add a `wait` here, so that the CI doesn't run for nothing when this tests fails. @ldionne, WDYT?
Good point! I think that makes sense, especially since our CI runs can take a long time.


================
Comment at: libcxx/utils/ci/run-buildbot:142
+        | tee ${BUILD_DIR}/generate_feature_test_macro_components.patch
+    git reset --hard HEAD
+    python3 libcxx/utils/generate_header_tests.py
----------------
curdeius wrote:
> Quuxplusone wrote:
> > Mordante wrote:
> > > Quuxplusone wrote:
> > > > 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.
> > > 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?
> > 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`.
> 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.
I'll change it to one file. @Quuxplusone I'll wait for D99309 to land and include that test in this patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D99862/new/

https://reviews.llvm.org/D99862