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

Thu Nov 10 11:19:48 PST 2022

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

Thanks for all feedback!

================
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>
+
----------------
aaron.ballman wrote:
> ldionne wrote:
> > 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.
> > 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.
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. 

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133249