Page MenuHomePhabricator

[libc++] Documents details of the pre-commit CI.
ClosedPublic

Authored by Mordante on Sep 3 2022, 1:07 AM.

Details

Reviewers
aaron.ballman
ldionne
philnik
Group Reviewers
Restricted Project
Restricted Project
Commits
rG0c111dd86fff: [libc++] Documents details of the pre-commit CI.
Summary

This documentation aims to make it cleare how the libc++ pre-commit CI
works. For libc++ developers and other LLVM projects whose changes can
affect libc++.

This was discusses with @aaron.ballman as a follow on some unclearities
for the Clang communitee how the libc++ pre-commit CI works.

Note some parts depend on patches under review as commented in the
documentation.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mstorsjo added inline comments.Sep 3 2022, 1:16 PM
libcxx/docs/Contributing.rst
158

Side note - these tests only test one specific version of C++ with these versions of Clang - we don't have fully coverage of all standard versions with all supported versions of Clang. (Not really sure if that's worth mentioning though. Also I do see this kinda hinted at at the end of the file.)

mizvekov added inline comments.
libcxx/docs/Contributing.rst
164–165

Thanks for the review comments. I'll address them in a little while. I want to wait for more review comments.

clang/www/hacking.html
302

Agreed configurations seems more appropriate in this context.

libcxx/docs/Contributing.rst
119

This is the version of Clang in the main branch. How about:
Unless specified otherwise the nightly build of Clang from the main branch is used.

158

We might spell it our more explicitly. But maybe not here but where the hints are. There are a lot of possible permutations not tested. For example the modular build is only tested with C++2b.

philnik accepted this revision as: philnik.Sep 5 2022, 9:32 AM
philnik added a subscriber: philnik.

Mostly LGTM. Just a few nits.

clang/www/hacking.html
276

To make it consistent throughout

284
296
300

Having a secondly without a firstly seems weird.

310

This reads like you want to say that clang doesn't support multiple versions of clang, which seems self-evident. Maybe just drop the Unlike Clang, ?

libcxx/docs/Contributing.rst
87–219

Complete nit, but XX reads to me like the digits have to be the same. I'd suggest XY to make it obvious that they can be different.

88
107

Not really currently. We still claim FreeBSD support without a CI runner.

231–233

Complete nit, but I think CC=clang-14 CXX=clang++-14 run-builtbot generic-cxx17 as an example would be better to avoid polluting people's environment if they're unfamiliar with a terminal.

mstorsjo added inline comments.Sep 5 2022, 12:50 PM
libcxx/docs/Contributing.rst
119

I still don't quite understand what this tries to say. "Unless specified otherwise" - where would I specify a different preference, when I upload a patch and it gets run through CI?

There's three different concepts involved here:

  • The CI build matrix (buildkite-pipeline.yml) which specifies a bunch of different versions of tools and standards to run the tests on. Here you can't specify a different preference - all of them are run (unless you edit the file in your patch).
  • The Docker images, where the default clang executable is a recent nightly build, but older releases are available as clang-<version>
  • The run-buildbot script, which just uses whatever compiler is set as default (via e.g. the CXX env var).

So, what about these three concepts is this paragraph trying to say - the second or the third point? This really needs to specify what it talks about - build matrix, docker images or run-buildbot script.

The same thing goes for the following paragraph.

This is awesome. I have some comments, but I have not done an in-depth pass at this yet.

libcxx/docs/Contributing.rst
139
142

It feels like this whole section might be prone to becoming out of date. I'm not sure how useful it is since the jobs are pretty self-explanatory. Perhaps it is sufficient to link to run-buildbot and buildkite-pipeline.yml?

145

Thank you for working on this!

clang/www/hacking.html
33

Similar capitalization is used in the file.

292
293
295–296

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.)

300

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

311–312

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?

libcxx/docs/Contributing.rst
87–219

Or you could go with something along the lines of C++<version>.

107

Also, do I remember correctly that Windows testing is not done on a CI runner for libc++?

114
119–121

(mostly trying to get rid of the duplicate "Unless specified otherwise".)

186
193–194
philnik added inline comments.Sep 6 2022, 12:10 PM
clang/www/hacking.html
295–296

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.

311–312

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.

libcxx/docs/Contributing.rst
107

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.

ldionne added inline comments.Sep 8 2022, 12:16 PM
clang/www/hacking.html
295–296

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.

Mordante marked 8 inline comments as done.Sep 8 2022, 1:07 PM

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

clang/www/hacking.html
295–296

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

300

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.

311–312

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.
libcxx/docs/Contributing.rst
107

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.

tahonermann added inline comments.
clang/www/hacking.html
295–296

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.

zero9178 added inline comments.
clang/www/hacking.html
295–296

This is currently being implemented by https://reviews.llvm.org/D132438 and landed before but had to be reverted unfortunately.

Mordante updated this revision to Diff 459347.Sep 11 2022, 4:03 AM
Mordante marked 15 inline comments as done.

Addresses review comments.

libcxx/docs/Contributing.rst
88

They are synonyms, since use the underscore in the CI I prefer to use that name.

119

I've reworded it, please have a look whether the new wording is clear.

142

The documentation for these files is already available in this document, more at the end.

I tried to mainly describe the special parts and tried to keep it generic. I could prune a bit more, but I feel some explanation would be good since there are some "suprises" like, formatting allowed to fail, modular build has nothing to do with C++20 modules.

231–233

Good point.

Mordante updated this revision to Diff 459351.Sep 11 2022, 4:25 AM

Rebased to fix CI failure.

mstorsjo added inline comments.Sep 11 2022, 11:55 AM
libcxx/docs/Contributing.rst
119

Right, that makes it clearer to me. Thanks!

ldionne accepted this revision.Sep 13 2022, 9:59 AM

LGTM w/ comments. Thanks a lot for writing this!

clang/www/hacking.html
279

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.

libcxx/docs/Contributing.rst
91–92

I think this is fine in this file for now. We can reorganize the docs if needed with a fresh holistic view if needed.

103–104
125–126
127–129
131–133

This can be removed.

157–158
182–183

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.

196–198

I would remove this comment, since it's going to become irrelevant quite soon, and I think we're likely to forget to update it. There are already comments in our Dockerfile to that effect.

204
This revision is now accepted and ready to land.Sep 13 2022, 9:59 AM
aaron.ballman added inline comments.Sep 15 2022, 8:48 AM
clang/www/hacking.html
279

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.

Mordante marked 24 inline comments as done.Sep 16 2022, 11:15 AM
Mordante added inline comments.
clang/www/hacking.html
279

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.

295–296

I removed the part of the dummy file.

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

311–312

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.

libcxx/docs/Contributing.rst
182–183

I've applied your suggestion and removed the Patches part.

Mordante updated this revision to Diff 460827.Sep 16 2022, 11:20 AM
Mordante marked 2 inline comments as done.

Addresses review comments.

ldionne accepted this revision.Sep 20 2022, 9:24 AM
ldionne added inline comments.
clang/www/hacking.html
306–308
311–312

I remember discussing this with @aaron.ballman.

I am 100% on-board with the notion that if Clang makes a legitimate conforming change and it breaks libc++ (most likely because we are doing something wrong, UB, or relying on Clang details), then it is *primarily* libc++'s responsibility to fix this ASAP. I also remember that we agreed it should be a collaborative effort, i.e. the Clang folks should try to help us understand the failure, and should be understanding if the fix on our side is really non-trivial. But, the bottom line is that if they change something and it breaks us because we're doing something wrong, it's our responsibility to fix our stuff ASAP to get the CI green again.

This is the same situation that we sometimes have with the LLDB data formatters, and I think the dynamics need to be the same there as well. If we break that because of a 100% legitimate change in libc++, our expectation is that they'll fix it ASAP, and their expectation is that we'll provide support as needed.

I think it would be useful to document that, since it technically departs from LLVM's usual policy of "revert when it breaks anything at all". And more generally speaking, I strongly think that this policy needs to be revisited, however that's beyond the scope of this documentation improvement.

libcxx/docs/Contributing.rst
103
aaron.ballman added inline comments.Sep 20 2022, 9:33 AM
clang/www/hacking.html
311–312

I am 100% on-board with the notion that if Clang makes a legitimate conforming change and it breaks libc++ (most likely because we are doing something wrong, UB, or relying on Clang details), then it is *primarily* libc++'s responsibility to fix this ASAP. I also remember that we agreed it should be a collaborative effort, i.e. the Clang folks should try to help us understand the failure, and should be understanding if the fix on our side is really non-trivial. But, the bottom line is that if they change something and it breaks us because we're doing something wrong, it's our responsibility to fix our stuff ASAP to get the CI green again.

Thanks! I think a succinct way to put it is: I think of libc++ as a downstream consumer of Clang. If we break a downstream, we want to fix it ASAP, but if the break is because of a conforming change, we don't want to revert the changes in Clang. Communication between Clang and libc++ developers will help us get to the bottom of any such problem to find a good solution.

I think it would be useful to document that, since it technically departs from LLVM's usual policy of "revert when it breaks anything at all". And more generally speaking, I strongly think that this policy needs to be revisited, however that's beyond the scope of this documentation improvement.

+1 to all of this.

Mordante marked 6 inline comments as done.Thu, Nov 10, 11:19 AM

Thanks for all feedback!

clang/www/hacking.html
311–312

It seems this patch slipped under the radar, sorry about that.

I wouldn't mind to change the policy but it would be good discuss with Clang and libc++ developers what the expectations are. I really prefer to *not* break the CI, instead I would like to collaborate on patches so we have one patch that makes the Clang *and* libc++ changes. Ideally I would like to do the same for LLDB.

I actually like the LLVM revert policy, especially since the reverter has obligations too. But that would be better to discuss on Discourse.

This revision was automatically updated to reflect the committed changes.
Mordante marked an inline comment as done.