<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Dec 4, 2020 at 8:07 PM Tom Stellard <<a href="mailto:tstellar@redhat.com">tstellar@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">On 12/4/20 7:19 PM, Mehdi AMINI wrote:<br>
> <br>
> <br>
> On Fri, Dec 4, 2020 at 6:42 PM Tom Stellard via llvm-dev <br>
> <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>> wrote:<br>
> <br>
> On 12/3/20 4:27 PM, Geoffrey Martin-Noble wrote:<br>
> > Apologies for the delayed response here. I was out of the "office".<br>
> ><br>
> > Thanks for taking another look :-)<br>
> ><br>
> > I want to respond first to the process question of pitch vs RFC. My<br>
> > impression was that the pitch process should be used in the case<br>
> that an<br>
> > RFC couldn't reach consensus. I asked a few times in the last thread<br>
> ><br>
> (<a href="https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/uVlV3pMTBAAJ" rel="noreferrer" target="_blank">https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/uVlV3pMTBAAJ</a> and<br>
> > <a href="https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/wF5mu-dpBAAJ" rel="noreferrer" target="_blank">https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/wF5mu-dpBAAJ</a>)<br>
> > whether I should move this to a pitch, but feel like there wasn't a<br>
> > clear response in the context of Renato's support tiers RFC.<br>
> ><br>
> > It seems like Tom and Renato still disagree about whether I<br>
> should move<br>
> > this to a pitch. I would appreciate some consensus on that point at<br>
> > least :-D I do see the appeal of a living document for this sort of<br>
> > thing, so definitely see the appeal there, but also it seems like<br>
> the<br>
> > pitch process is a heavier-weight and more unusual one, so I was<br>
> > hesitant. My inclination is to continue this as an RFC unless we are<br>
> > unable to reach consensus on the issue as outlined in the pitch<br>
> process<br>
> > description. It does feel like this is really not quite as big a<br>
> > decision as you seem to be suggesting. It's also an easily<br>
> reversible<br>
> > one since there are no build dependencies and everything is<br>
> contained.<br>
> ><br>
> <br>
> I still think this should be a pitch. The original mailing list<br>
> discussion was controversial and that's when an RFC should be escalated<br>
> to a pitch according to: [1].<br>
> <br>
> <br>
> You may have missed it, but in the meantime there has been another RFC <br>
> clarifying our policy though: <a href="https://llvm.org/docs/SupportPolicy.html" rel="noreferrer" target="_blank">https://llvm.org/docs/SupportPolicy.html</a><br>
> It seems fair to me to revisit this RFC as is in light of the policy change.<br>
> <br>
<br>
I don't think the questions about whether or not this should be included <br>
in the project are answered by this new policy. </blockquote><div><br></div><div>I'll quote the policy:</div><div><br></div><div><div>> Section: "What is covered"</div><div>> The peripheral tier is composed of:<br></div><div>> Experimental targets and options that haven’t been enable by default yet.</div><div>> Main repository projects that don’t get released or regularly tested.</div><div>> Legacy tools and scripts that aren’t used in upstream validation.</div><div>> <b>Alternative build systems (ex. GN, Bazel) and related infrastructure.</b></div></div><div><br></div><div>The intent of the policy is to cover exactly this proposal.</div><div> </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">To me the part about <br>
how the bazel build files were going to be supported and what <br>
responsibility the community had for maintaining them was always very clear.<br>
<br>
> I'd actually like to request that the objections are reiterated and <br>
> positioned in terms of the policy before we escalate this.<br>
> <br>
<br>
I don't think it's really fair to ask people to re-object to the <br>
proposal. </blockquote><div><br></div><div>Why?</div><div>The objections were mostly answered and have been addressed in the policy. I don't quite get what you would put in a "pitch" while the informations are outdated by the policy.</div><div>On the contrary it seems not only fair to me, but necessary.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">In my opinion, one of the problems with RFCs in the past is <br>
that they turn into an endurance test, because there is no process for <br>
making a decision. Either the proposer gets tired of asking and gives <br>
up or the objectors get tired of objecting and give up. We have a <br>
decision process now with the pitch process, and I think we should use it</blockquote><div><br></div><div>We have to use it when we can't do otherwise. And again, I disagree that this is a case without having objection formulated in light of the policy.</div><div><br></div><div><br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<br>
> <br>
> Thank you for responding to my technical concerns, and I agree that<br>
> working out most of those details may be better left for a patch review<br>
> discussion. But I think at least the presence of build information for<br>
> other projects and the sub-module alternative should be mentioned in<br>
> the<br>
> pitch.<br>
> <br>
> If there were only technical or support policy issues like these to<br>
> resolve then I don't think this would be controversial and require a<br>
> pitch.<br>
> <br>
> My main issue with this RFC, (which I tried to address at the end of my<br>
> previous mail), is the precedent this sets for what gets included in<br>
> tree. Essentially, we have a subset of our community that chose to<br>
> go a<br>
> different direction from upstream, as always there are costs and<br>
> benefits with this decision. The question for the community is do we<br>
> want to help or encourage this in the future by removing some of the<br>
> costs of these decisions and allowing alternative implementations to<br>
> live in tree.<br>
> <br>
> Maybe for build systems this is OK, and for other things this is not,<br>
> I don't know. But if we are going to be setting a precedent, to me,<br>
> the<br>
> best way to do this is through the pitch process.<br>
> <br>
> <br>
> Why are you considering this "setting a precedent" while there is <br>
> already GN in tree?<br>
> <br>
<br>
You are right we are not really setting a precedent here, because GN is <br>
already in tree. However, I don't think we should now just allow any <br>
build system to be added to the tree just because GN is there. We need <br>
to have some kind of process and criteria for deciding what gets added <br>
and what doesn't. I think a pitch will help accomplish this.<br>
<br>
I'll be honest, I don't really understand why there is so much push back <br>
on turning this into a pitch. Is it really that much extra work?<br>
<br>
-Tom<br>
<br>
> -- <br>
> Mehdi<br>
> <br>
> <br>
> <br>
> <br>
> -Tom<br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> <br>
> [1]<br>
> <a href="https://github.com/llvm/llvm-www/blob/master/proposals/LP0001-LLVMDecisionMaking.md" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-www/blob/master/proposals/LP0001-LLVMDecisionMaking.md</a><br>
> <br>
> > On Mon, Nov 16, 2020 at 9:41 PM Tom Stellard <<a href="mailto:tstellar@redhat.com" target="_blank">tstellar@redhat.com</a><br>
> <mailto:<a href="mailto:tstellar@redhat.com" target="_blank">tstellar@redhat.com</a>><br>
> > <mailto:<a href="mailto:tstellar@redhat.com" target="_blank">tstellar@redhat.com</a> <mailto:<a href="mailto:tstellar@redhat.com" target="_blank">tstellar@redhat.com</a>>>> wrote:<br>
> ><br>
> > > This should have approximately the same impact on the<br>
> community<br>
> > as the<br>
> > > current GN build in `llvm/utils/gn` does today. That is, it<br>
> > should not<br>
> > > affect anyone who doesn't care.<br>
> > ><br>
> ><br>
> > I want to push back on this a little bit, because having the<br>
> code in<br>
> > tree does impact everyone, even people who don't care about<br>
> it. It<br>
> > increases disk usage, commit traffic, checkout times,<br>
> bugzilla / issue<br>
> > traffic, and CI builds to name a few things. There are costs<br>
> to having<br>
> > this in tree, the question (as always) is do the benefits<br>
> outweigh the<br>
> > costs?<br>
> ><br>
> > Yes my apologies that this was poorly phrased. I was aiming for a<br>
> pithy<br>
> > summary and a clear statement that our goal here is not to<br>
> significantly<br>
> > impact contributors uninterested in Bazel. My impression is that<br>
> the GN<br>
> > build has achieved that goal. I definitely agree that any<br>
> addition to<br>
> > the monorepo should have a clear weighing of costs vs benefits<br>
> and that<br>
> > the costs are never actually zero. I do think the costs here are<br>
> really<br>
> > quite low however. I am happy to address your concerns and also<br>
> think<br>
> > that it is important to note that if additional issues arise we are<br>
> > still agreeing to be on the hook for addressing them (e.g. if in<br>
> > practice this causes some unforseen issue with the release) and<br>
> deleting<br>
> > this contribution if we cannot do so in a timely manner (`rm -rf<br>
> > utils/bazel` is all it requires).<br>
> ><br>
> > Personally, I do not think we should have alternative build<br>
> systems in<br>
> > tree. However, I still think you should try to propose this<br>
> as a pitch.<br>
> > I would much rather this go through a fair process and land<br>
> than for it<br>
> > to be rejected based on a contentious thread.<br>
> ><br>
> > Here is why I'm not convinced this should be in tree:<br>
> ><br>
> > To me it's not clear why having the build files in-tree is<br>
> better than<br>
> > having a separate repo with an llvm-project sub-module. The<br>
> in tree<br>
> > bazel files will be broken from time to time, since most<br>
> developers will<br>
> > not be updating them, however, with the sub-module approach<br>
> you can<br>
> > ensure that the build will always work by pinning the llvm-bazel<br>
> > repo to<br>
> > a known-working commit of llvm-project. Can you expand on the<br>
> > pros/cons<br>
> > of in-tree vs out-of-tree with sub-modules.<br>
> ><br>
> > Out-of-tree with a submodule is the current approach we have with<br>
> > <a href="https://github.com/google/llvm-bazel" rel="noreferrer" target="_blank">https://github.com/google/llvm-bazel</a>. It's certainly doable, but<br>
> > involves quite a bit of bookkeeping to track which version<br>
> corresponds<br>
> > to a given version of LLVM such that someone can fetch the correct<br>
> > configuration (you'll note that the repository has about 7k tags<br>
> at the<br>
> > moment). To make things somewhat more complicated, the typical<br>
> way to<br>
> > fetch something for use in Bazel is with an http_archive<br>
> ><br>
> <<a href="https://docs.bazel.build/versions/master/repo/http.html#http_archive" rel="noreferrer" target="_blank">https://docs.bazel.build/versions/master/repo/http.html#http_archive</a>> which<br>
> <br>
> > requires one to specify the archive digest to avoid refetching on<br>
> each<br>
> > build. This doesn't work particularly well with tags that change<br>
> which<br>
> > commit they point to. I'm not saying these issues aren't<br>
> solvable, but<br>
> > they add quite a bit of complexity.<br>
> ><br>
> > The other point is that I think this makes contributing to the Bazel<br>
> > configuration quite a bit more complex because you have to apply<br>
> patches<br>
> > across multiple repositories to also be kept in sync. Given that<br>
> LLVM<br>
> > has a monorepo, it still seems like the logical place for a build<br>
> > configuration of LLVM used by multiple projects.<br>
> ><br>
> > Other concerns I have from reviewing the patch:<br>
> ><br>
> > It seems like these are mostly concerns with the specific<br>
> > implementation. Would you be alright with saving the specific<br>
> details<br>
> > for an eventual review on the patch if this moves forward? I've made<br>
> > brief responses below.<br>
> ><br>
> > * It looks like there is a build configuration for at least one<br>
> > external<br>
> > project (zlib) and possibly another (vulkan-headers?). Do we<br>
> really<br>
> > want to have build configurations for non-LLVM projects in our<br>
> > tree? Is<br>
> > there any limit to the number of external projects that can<br>
> and will be<br>
> > added?<br>
> ><br>
> > These are dependencies of the LLVM Project and LLVM keeps its<br>
> > dependencies pretty tightly managed AFAIU. These configurations<br>
> are also<br>
> > pretty trivial, "here are the source files", type things, so I think<br>
> > it's even a bit generous to call them configurations: we're just<br>
> > informing Bazel where the files are located.<br>
> ><br>
> ><br>
> > * There are 3 files (abi-breaking.h.cmake, config.h.cmake,<br>
> > llvm-config.h.cmake) that have been copied from the llvm tree<br>
> into<br>
> > utils/bazel/, is there some way we can avoid carrying multiple<br>
> > copies of<br>
> > the same file in tree?<br>
> ><br>
> ><br>
> > * Similarly, there are some files that are normally generated<br>
> at build<br>
> > time clang/Config/config.h, llvm/Config/config.h,<br>
> > llvm/Config/llvm-config.h that have been copied into<br>
> utils/bazel.<br>
> > Is it<br>
> > really necessary<br>
> > to have these in tree? Especially since some of the<br>
> templates, like<br>
> > llvm-config.h.cmake, are also in utils/bazel?<br>
> ><br>
> > The copy here is pretty much orthogonal to the actual build<br>
> > configuration. The intent is to have a literal change detector<br>
> test for<br>
> > changes to these cmake configurations, since they would invalidate<br>
> > assumptions in the Bazel configuration. Chandler and I went back and<br>
> > forth on a few different ways to do this. We can certainly look<br>
> at other<br>
> > options. The issue is that I don't think there's actually a<br>
> useful way<br>
> > to interpret the .cmake template files since changes to them are<br>
> also<br>
> > made as changes to the cmake configuration and without these<br>
> being in<br>
> > sync the files just drift. Happy to discuss other options for how to<br>
> > handle this. We could, for instance, have some other process that<br>
> just<br>
> > looks at the git diff/log for these files.<br>
> ><br>
> > * I still worry about the bazel files causing merging<br>
> conflicts when<br>
> > backported to the stable branch. If these are added to tree,<br>
> could we<br>
> > have a rule where commits to utils/bazel/ cannot include<br>
> changes to<br>
> > other files?<br>
> ><br>
> > I'd certainly be open to discussing restrictions that would avoid<br>
> > additional burden on release managers. I think that one makes<br>
> > contributing to the Bazel configuration more difficult because you<br>
> > cannot do it as part of a patch that requires a change, but if it's<br>
> > something that would cause issues with the release then we can<br>
> avoid it.<br>
> > My intuition is that this wouldn't actually come up often,<br>
> however. For<br>
> > example, just looking at the gn directory I see several commits<br>
> in the<br>
> > last week that touch this and other files. Have you actually run<br>
> into<br>
> > issues? Since this is unsupported the conflicts could also be<br>
> resolved<br>
> > pretty much however you wanted (e.g. delete the conflict markers,<br>
> delete<br>
> > the file), so they seem pretty trivial to deal with if they only<br>
> happen<br>
> > occasionally. My preference would therefore be to see if this is<br>
> > actually a problem in practice before putting rules in place.<br>
> ><br>
> ><br>
> > On Tue, Nov 17, 2020 at 2:27 AM Renato Golin <<a href="mailto:rengolin@gmail.com" target="_blank">rengolin@gmail.com</a><br>
> <mailto:<a href="mailto:rengolin@gmail.com" target="_blank">rengolin@gmail.com</a>><br>
> > <mailto:<a href="mailto:rengolin@gmail.com" target="_blank">rengolin@gmail.com</a> <mailto:<a href="mailto:rengolin@gmail.com" target="_blank">rengolin@gmail.com</a>>>> wrote:<br>
> ><br>
> > Hi Geoffrey,<br>
> ><br>
> > Thanks for the re-submission.<br>
> ><br>
> > I have some comments below that may sound negative, but they're<br>
> > probably just a reflection of my own ignorance. I want to<br>
> make sure<br>
> > the submission is clear, so it can be accepted on its own right.<br>
> ><br>
> > On Tue, 17 Nov 2020 at 03:02, Geoffrey Martin-Noble via llvm-dev<br>
> > <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>>>><br>
> wrote:<br>
> ><br>
> > This should not affect development of core tier components.<br>
> > One reason we propose adding this to the root utils/<br>
> directory<br>
> > instead of under llvm/utils (where GN is located) is to avoid<br>
> > unnecessarily sending messages to llvm-commits. Others have<br>
> > raised the concern that the existence of an alternative build<br>
> > system might lead to lack of maintenance for the CMake build<br>
> > system. Given that supporting CMake will remain a requirement<br>
> > and maintenance of a Bazel build system will continue to<br>
> happen<br>
> > regardless, we do not expect any significant impact in<br>
> this way.<br>
> ><br>
> ><br>
> > I was under the impression that "utils" was actually<br>
> "llvm/utils",<br>
> > which would be in the same place as GN. I don't think we should<br>
> > treat GN and Bazel as different and I really wouldn't like to<br>
> have a<br>
> > different quality control (for post commit reviews).<br>
> ><br>
> > If the Bazel commits are too verbose (for example, committing<br>
> > auto-generated code), then we should really clean that up and<br>
> commit<br>
> > the script that generates them and make that part of the build.<br>
> ><br>
> > I understand the need to move the noise away, but move it too far<br>
> > away and it's no better than in a separate repo.<br>
> ><br>
> > I am happy to put this in either location and agree it should be<br>
> in the<br>
> > same place as GN. If we were to decide that it should go `utils/`<br>
> then I<br>
> > would also propose we move GN to there as well. I believe the GN<br>
> files<br>
> > were contributed prior to the existence of the monorepo, so a<br>
> top-level<br>
> > `utils/` wouldn't have been an option. I think living under the root<br>
> > `utils/` directory makes more sense because these are not<br>
> configurations<br>
> > for only the LLVM subproject (we also build MLIR and Clang with<br>
> perhaps<br>
> > more to come). I believe it was Mehdi's suggestion that this<br>
> would help<br>
> > mitigate some of the costs to having it in the monorepo because Tom<br>
> > mentioned commit list traffic as a concern. I don't think I agree<br>
> that<br>
> > one directory up is akin to a separate repo though :-D<br>
> ><br>
> > That said, this is a really minor point for me. I'm happy to put<br>
> this<br>
> > wherever people prefer :-)<br>
> ><br>
> ><br>
> > A number of people raised the question of "why not a separate<br>
> > repository". This is indeed possible: It's what we've<br>
> done with<br>
> > <a href="https://github.com/google/llvm-bazel" rel="noreferrer" target="_blank">https://github.com/google/llvm-bazel</a>, which is currently used by<br>
> > <a href="https://github.com/google/iree" rel="noreferrer" target="_blank">https://github.com/google/iree</a>. It is significantly more<br>
> > infrastructure, coordination, and complexity for<br>
> something that<br>
> > is specifically a configuration for the LLVM project<br>
> itself, not<br>
> > its own dependent or adjacent project.<br>
> ><br>
> ><br>
> > I was also under the impression that one of the big reasons<br>
> why we<br>
> > needed it to be in LLVM is that, like CMake, it needed files all<br>
> > over the place. This would indeed be a major infrastructure<br>
> undertaking.<br>
> ><br>
> > But given that it's all being hosted in a single directory, and<br>
> > outside of the LLVM tree, I really can't see what's so much<br>
> harder<br>
> > about an extra checkout in the same tree.<br>
> ><br>
> > Bazel *wants* the build files to be all over the place, but I've<br>
> tricked<br>
> > it with some repository rule symlinking. That's also true of the<br>
> LLVM GN<br>
> > configuration, I believe. My assumption is that having BUILD files<br>
> > actually throughout the repository would be something that would<br>
> receive<br>
> > quite a bit of pushback and would be confusing for people who would<br>
> > naturally expect these BUILD files to be maintained as a<br>
> supported build<br>
> > system. I would happily put a BUILD.bazel file at the root of each<br>
> > subproject and drop the symlinking madness, but I suspect this<br>
> would not<br>
> > be embraced as a solution ;-P<br>
> ><br>
> ><br>
> > I believe this contribution will significantly improve the<br>
> > situation for downstream users that use Bazel while having<br>
> > minimal impact on the community at large.<br>
> ><br>
> ><br>
> > It's not clear to me yet if LLVM/Bazel is only used in Google<br>
> > projects or any other non-Google project. All that you listed<br>
> so far<br>
> > seem to be exclusive to Google.<br>
> ><br>
> > This is not a problem per se, but it does promote the idea that<br>
> > Google could common it up internally instead.<br>
> ><br>
> > The main reasons why it would be upstream are that it's either a<br>
> > product by or requirement to the project itself, or it helps<br>
> unite<br>
> > cross-industry collaboration that wouldn't be possible otherwise.<br>
> ><br>
> > It's clearly not the former (and why it's in the periphery tier),<br>
> > but it's also not clear it's in the latter either.<br>
> ><br>
> > I can really only speak for Google projects. I have also noticed<br>
> several<br>
> > other Bazel build configurations in the wild, e.g. PlaidML<br>
> ><br>
> <<a href="https://github.com/plaidml/plaidml/blob/master/vendor/llvm/llvm.BUILD" rel="noreferrer" target="_blank">https://github.com/plaidml/plaidml/blob/master/vendor/llvm/llvm.BUILD</a>> (Intel)<br>
> <br>
> > or this bazel_llvm<br>
> <<a href="https://github.com/ChrisCummins/bazel_llvm" rel="noreferrer" target="_blank">https://github.com/ChrisCummins/bazel_llvm</a>> project<br>
> > that I found after someone contributed a doc fix. I believe in<br>
> the last<br>
> > thread someone from Facebook mentioned that Bazel build files<br>
> would also<br>
> > be relatively easily translatable to their internal Bazel-derived<br>
> build<br>
> > system, Buck. Someone from Lyft also expressed interest in using<br>
> a Bazel<br>
> > build configuration if it was in-tree. But I can't really speak<br>
> to the<br>
> > motivations, road maps, etc. for any of these people, companies, or<br>
> > projects (if you're reading, please chime in ;-P).<br>
> ><br>
> <br>
> _______________________________________________<br>
> LLVM Developers mailing list<br>
> <a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a> <mailto:<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a><br>
> <br>
<br>
</blockquote></div></div></div>