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

Tue Sep 20 09:24:20 PDT 2022

ldionne accepted this revision.
ldionne added inline comments.

================
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:
> 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.
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.

================
Comment at: clang/www/hacking.html:306-308
+  documentation</a> about the pre-commit CI. When having questions regarding
+  libc++, best ask them in the <tt>#libcxx</tt> channel on
+  <a href="https://discord.gg/jzUbyP26tQ">LLVM's Discord server</a>.</p>
----------------

================
Comment at: libcxx/docs/Contributing.rst:103
+The CI tests libc++ for all :ref:`supported platforms <SupportedPlatforms>`.
+The build is started for every diff uploaded. A complete CI run takes
+approximately one hour. To reduce the load:
----------------

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249