<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 6:42 PM Tom Stellard via llvm-dev <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</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/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 that an <br>
> RFC couldn't reach consensus. I asked a few times in the last thread <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 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 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 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 reversible <br>
> one since there are no build dependencies and everything is 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></blockquote><div><br></div><div>You may have missed it, but in the meantime there has been another RFC clarifying our policy though: <a href="https://llvm.org/docs/SupportPolicy.html">https://llvm.org/docs/SupportPolicy.html</a></div><div>It seems fair to me to revisit this RFC as is in light of the policy change.</div><div><br></div><div>I'd actually like to request that the objections are reiterated and positioned in terms of the policy before we escalate this.</div><div> <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">
<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 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 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 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, the <br>
best way to do this is through the pitch process.<br></blockquote><div><br></div><div>Why are you considering this "setting a precedent" while there is already GN in tree?</div><div><br></div><div>-- </div><div>Mehdi</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>
-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>>> wrote:<br>
> <br>
> > This should have approximately the same impact on the 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 code in<br>
> tree does impact everyone, even people who don't care about it. It<br>
> increases disk usage, commit traffic, checkout times, bugzilla / issue<br>
> traffic, and CI builds to name a few things. There are costs to having<br>
> this in tree, the question (as always) is do the benefits outweigh the<br>
> costs?<br>
> <br>
> Yes my apologies that this was poorly phrased. I was aiming for a pithy <br>
> summary and a clear statement that our goal here is not to significantly <br>
> impact contributors uninterested in Bazel. My impression is that the GN <br>
> build has achieved that goal. I definitely agree that any addition to <br>
> the monorepo should have a clear weighing of costs vs benefits and that <br>
> the costs are never actually zero. I do think the costs here are really <br>
> quite low however. I am happy to address your concerns and also 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 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 systems in<br>
> tree. However, I still think you should try to propose this as a pitch.<br>
> I would much rather this go through a fair process and land 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 better than<br>
> having a separate repo with an llvm-project sub-module. The in tree<br>
> bazel files will be broken from time to time, since most developers will<br>
> not be updating them, however, with the sub-module approach 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 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 at the <br>
> moment). To make things somewhat more complicated, the typical way to <br>
> fetch something for use in Bazel is with an http_archive <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>
> requires one to specify the archive digest to avoid refetching on each <br>
> build. This doesn't work particularly well with tags that change which <br>
> commit they point to. I'm not saying these issues aren't 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 patches <br>
> across multiple repositories to also be kept in sync. Given that 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 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 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 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 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 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 at build<br>
> time clang/Config/config.h, llvm/Config/config.h,<br>
> llvm/Config/llvm-config.h that have been copied into utils/bazel. <br>
> Is it<br>
> really necessary<br>
> to have these in tree? Especially since some of the 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 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 at other <br>
> options. The issue is that I don't think there's actually a useful way <br>
> to interpret the .cmake template files since changes to them are also <br>
> made as changes to the cmake configuration and without these 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 just <br>
> looks at the git diff/log for these files.<br>
> <br>
> * I still worry about the bazel files causing merging conflicts when<br>
> backported to the stable branch. If these are added to tree, could we<br>
> have a rule where commits to utils/bazel/ cannot include 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 avoid it. <br>
> My intuition is that this wouldn't actually come up often, however. For <br>
> example, just looking at the gn directory I see several commits in the <br>
> last week that touch this and other files. Have you actually run into <br>
> issues? Since this is unsupported the conflicts could also be resolved <br>
> pretty much however you wanted (e.g. delete the conflict markers, delete <br>
> the file), so they seem pretty trivial to deal with if they only 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>>> 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 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>>> wrote:<br>
> <br>
> This should not affect development of core tier components.<br>
> One reason we propose adding this to the root utils/ 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 happen<br>
> regardless, we do not expect any significant impact in this way.<br>
> <br>
> <br>
> I was under the impression that "utils" was actually "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 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 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 in the <br>
> same place as GN. If we were to decide that it should go `utils/` then I <br>
> would also propose we move GN to there as well. I believe the GN files <br>
> were contributed prior to the existence of the monorepo, so a 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 configurations <br>
> for only the LLVM subproject (we also build MLIR and Clang with perhaps <br>
> more to come). I believe it was Mehdi's suggestion that this 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 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 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 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 something that<br>
> is specifically a configuration for the LLVM project itself, not<br>
> its own dependent or adjacent project.<br>
> <br>
> <br>
> I was also under the impression that one of the big reasons 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 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 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 tricked <br>
> it with some repository rule symlinking. That's also true of the LLVM GN <br>
> configuration, I believe. My assumption is that having BUILD files <br>
> actually throughout the repository would be something that would 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 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 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 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 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 several <br>
> other Bazel build configurations in the wild, e.g. PlaidML <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>
> or this bazel_llvm <<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 the last <br>
> thread someone from Facebook mentioned that Bazel build files would also <br>
> be relatively easily translatable to their internal Bazel-derived build <br>
> system, Buck. Someone from Lyft also expressed interest in using a Bazel <br>
> build configuration if it was in-tree. But I can't really speak 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><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>
</blockquote></div></div></div>