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

Thu Sep 8 13:08:02 PDT 2022

Mordante marked 8 inline comments as done.
Mordante added a comment.

Thanks for all review comments! I'm a bit out of time so I will look at the other comments later.

================
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>
+
----------------
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?

================
Comment at: clang/www/hacking.html:300
+  CI lacks the resources to build every Clang diff. A single run takes about
+  one hour. Secondly most changes of Clang won't affect libc++.</a>
+
----------------
aaron.ballman wrote:
> philnik wrote:
> > Having a secondly without a firstly seems weird.
> +1 to the "secondly" being a bit weird, but also pushing back a bit on the assertion that most changes in Clang won't affect libc++: we change the output of Clang's diagnostics on an almost-daily basis.
> 
> I think it's honest to say: "It is difficult to determine which changes in Clang will affect libc++ and running the precommit CI in all circumstances is prohibitively resource intensive given how actively the project is updated." or something along those lines.
I based my statement on the observations I made:
- I don't see a lot of Clang changes with a dummy file.
- Clang doesn't break libc++ often. (Still it would be good to get the number down).
Libc++ uses diagnostics, but mainly to validate diagnostics for ill-formed code. So that probably explains why not every diagnostic is an issue. But changing the diagnostic of something like a `static_assert` will be detected.

I've replaced the secondly sentence with `Unfortunately it is difficult to determine which changes in Clang will affect libc++.`

I want to keep the other two sentences since it explains why an opt-in is needed.

================
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>
+
----------------
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. 
```

================
Comment at: libcxx/docs/Contributing.rst:107
+
+The CI tests libc++ for all :ref:`supported platforms <SupportedPlatforms>`.
+The build is started for every diff uploaded. A complete CI run takes
----------------
philnik wrote:
> aaron.ballman wrote:
> > philnik wrote:
> > > Not really currently. We still claim FreeBSD support without a CI runner.
> > Also, do I remember correctly that Windows testing is not done on a CI runner for libc++?
> Nope, we have full Windows coverage in the CI. This might have been the case some time before I started working on libc++, but it's definitely not the case anymore.
> Not really currently. We still claim FreeBSD support without a CI runner.

True. But I wonder whether we still need to claim FreeBSD support. I recently pinged their CI patch to get the current state. I feel that this detail is a bit out of the scope of this review. Still I think this is an important issue.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249