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

Fri Sep 16 11:15:36 PDT 2022

Mordante marked 24 inline comments as done.
Mordante 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>
+
----------------
zero9178 wrote:
> tahonermann wrote:
> > 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.
> This is currently being implemented by https://reviews.llvm.org/D132438 and landed before but had to be reverted unfortunately.
I removed the part of the dummy file.

Having a `check-all` indeed sounds very nice to have.

================
Comment at: clang/www/hacking.html:311-312
+  <p>Unlike Clang, libc++ supports multiple versions of Clang. Therefore when a
+  patch changes the diagnostics it might be required to use a regex in the
+  "expected" tests to make it pass the CI.</p>
+
----------------
Mordante wrote:
> philnik wrote:
> > aaron.ballman wrote:
> > > Should we document the expectation that breaking libc++ due to conforming changes in Clang (in terms of diagnostics and bug fixes, not so much in terms of introducing new language extensions) are generally the responsibility of libc++ maintainers to address, but Clang contributors should attempt to reduce disruptions as much as possible by collaborating with libc++ maintainers when this situation comes up?
> > That's definitely a good idea. Maybe mention that clang contributors should ping the #libc group to get the attention of libc++ contributors so we can prepare a follow-up patch or, if the author is comfortable with it, also fix libc++ in the same patch (and hopefully get fast approval). Most breakages should be relatively simple to fix.
> I like the idea to collaborate more.
> 
> I want to give this some thought and especially how to word it. I want to avoid that it's seen as a carte blanche to commit Clang changes which break libc++. This would mean HEAD is broken and that's a huge issue for downstream users like Google who tend to follow HEAD closely.
> 
> I would prefer to use commandeering and instead of two patches where one leave HEAD in a broken state. Even if it's short it would make bi-secting harder. Personally I really dislike the idea of having commits that knowingly leave a repository in a broken state. It also doesn't align with the LLVM developer policy https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy 
> ```
> As a community, we strongly value having the tip of tree in a good state while allowing rapid iterative development. 
> ```
> 
> Should we document the expectation that breaking libc++ due to conforming changes in Clang (in terms of diagnostics and bug fixes, not so much in terms of introducing new language extensions) are generally the responsibility of libc++ maintainers to address, but Clang contributors should attempt to reduce disruptions as much as possible by collaborating with libc++ maintainers when this situation comes up?

@aaron.ballman I have given this some thought. I think it would be best to discuss this with some libc++ and Clang contributors/code owners. For libc++ at least @ldionne should be present since he's the libc++ code owner. I would like to be present too. After we have agreement we can update this documentation accordingly.

================
Comment at: clang/www/hacking.html:279
+
+  <p>Some changes in Clang affect <a href="https://libcxx.llvm.org">libc++</a>,
+  for example:</p>
----------------
aaron.ballman wrote:
> ldionne wrote:
> > This follows my previous comment, but we really don't want to encourage Clang folks to start using a `.DELETE_ME` file for testing Clang changes. Indeed, roughly 54/55 of our CI jobs don't use the just-built Clang. The only relevant CI job for a Clang change is the Bootstrapping build. As a result, using a `.DELETE_ME` file is extremely wasteful of  libc++ CI, since almost all the CI jobs are completely irrelevant to the change being tested. Furthermore, it will also provide a false sense of security for Clang folks, who will think "hey but I ran the CI under all configurations", not understanding the details.
> > 
> > So I think we need to set up libc++ testing for Clang changes, but we should discourage folks from running the libc++ CI itself as a poor proxy for testing Clang changes. Most of the jobs are just not useful at all from the perspective of testing a Clang change.
> > 
> > So I'd rather not add this part of the documentation at all, to avoid creating a problem for ourselves.
> > So I'd rather not add this part of the documentation at all, to avoid creating a problem for ourselves.
> 
> FWIW, I love the idea of setting up libc++ testing for Clang changes so we don't need a .DELETE_ME file. I think that's ultimately where we all would love to land. I'm fine either pulling these docs in anticipation of that work or leaving the docs here until that work is done.
Since @ldionne strongly prefers to remove it I'll remove it. Not in the scope of this review, but let's investigate what is needed to get this setup libc++ in the Clang pre-commit CI. Having it in Clang's pre-commit CI will make it easier to test multiple configurations instead of only what's available in libc++'s bootstrapping build. I have experience with the libc++ pre-commit CI so maybe I can help with the Clang version.

Part of the documentation is intended for Clang developers to be aware of libc++'s pre-commit CI. When Clang tests libc++ in their pre-commit CI they might do minor libc++ changes. These changes will trigger the libc++ precommit CI. So that part should still be documented. @ldionne so please have a look after the next update.

Note most of the changed documentation makes really sense when Clang tests libc++. If it's not feasible to realize in short term we might want to comment out the new section. But I hope we can get this working rather soon.

================
Comment at: libcxx/docs/Contributing.rst:182-183
+Note that quite a bit of this infrastructure is heavily Linux focused. This is
+the platform used by most of libc++'s Buildkite runners and developers. Patches
+for other platforms are welcome.
+
----------------
ldionne wrote:
> IMO this would have its place in the part of the documentation where we list all the platforms that we support. We *are* interested in patches for adding support to new platforms. However, I'm not sure what patches for porting our CI infra to other platforms would mean, since e.g. the Docker image is running Linux by definition. Same for the macOS bits of our CI infra -- it wouldn't make sense to try to port that to other platforms.
I've applied your suggestion and removed the Patches part.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249