[PATCH] D133249: [libc++] Documents details of the pre-commit CI.

[Tom Honermann via Phabricator via cfe-commits]

tahonermann added inline comments.


================
Comment at: clang/www/hacking.html:295-296
+  directory will cause the update of the diff to start a CI run. This dummy
+  file will also add the libc++ group to the list of reviewers. The status of
+  the build will be available in Phabricator.</p>
+
----------------
ldionne wrote:
> Mordante wrote:
> > philnik wrote:
> > > aaron.ballman wrote:
> > > > I am guessing the dummy file should then be removed before landing the commit and we should document that?
> > > > 
> > > > (FWIW, adding a dummy file feels really unclean as a design -- it's mysterious behavior for new contributors, which the documentation helps with a bit, but also it's a risk for checking in files that are never intended to be in the tree. If there's a way to improve this somehow so we don't need to trick the precommit CI into running, that would be really nice and totally outside of the scope of this review.)
> > > It would be great if just the bootstrapping build could be run on all clang reviews. That should show most problems with changes in clang. If there are any problems in libc++, fixing them would result in a full CI run. Having libc++ run against the patch would probably also be awesome as a smoke test for any clang changes.
> > +1 to what @ldionne said.
> > 
> > One of the advantages would be that it's possible to test Clang against the libc++ code base using different language versions, by defining multiple test runs using different language versions. (Slightly related to https://discourse.llvm.org/t/lit-run-a-run-line-multiple-times-with-different-replacements/64932) @aaron.ballman is looking at extending the Clang pre-commit CI something worth looking into?
> > 
> > 
> I agree. Let's:
> 
> 1. Setup a `buildkite-pipeline.yml` file that controls the jobs done for Clang. I think right now this is hardcoded somewhere in https://github.com/google/llvm-premerge-checks
> 2. Simply add `LLVM_ENABLE_RUNTIMES=libcxx` to the config that Clang uses so they'll automatically build libc++ with the Clang that was just built anyway. That would run on Clang's own agents that are used for Clang tests, not on the libc++ CI agents. We can adjust if running the libc++ tests causes too much strain on the infrastructure running Clang CI.
> 
> That way, a basic bootstrapping build of libc++ will be run on every Clang change, and TBH I think all of this added documentation for Clang folks to test libc++ changes can simply go away. There really wouldn't be a reason to ever add a `.DELETE_ME` file to libc++ for a Clang patch anymore.
This sounds like a good approach to me. This might be a bit tangential, but if `libcxx` is present in `LLVM_ENABLE_RUNTIMES`, could we also have the `check-all` target depend on `check-libcxx`? That would help ensure that developers run libc++ tests locally before they hit the CI infrastructure. Likewise for `compiler-rt` and `libcxxabi` and the `check-runtimes` target.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249