[llvm-dev] RFC: Contributing Bazel BUILD files in the "peripheral" support tier

[Eric Christopher via llvm-dev]

Sweet, thanks for the message. It's really appreciated. If you do see any
problems please let me or someone else know as the ease of llvm upstream
development is something I care deeply about.

-eric

On Mon, Dec 7, 2020 at 12:24 PM Chris Tetreault via llvm-dev <
llvm-dev at lists.llvm.org> wrote:

> Just to be clear, I’m still not in love with this plan. However, it’s
> being done in accordance with the new policy on this sort of thing, and I’m
> going to have faith that the policy has teeth if it becomes any sort of
> issue.
>
>
>
> Thanks,
>
>    Christopher Tetreault
>
>
>
> *From:* Geoffrey Martin-Noble <gcmn at google.com>
> *Sent:* Thursday, December 3, 2020 4:27 PM
> *To:* LLVM Dev <llvm-dev at lists.llvm.org>
> *Cc:* tstellar at redhat.com; Renato Golin <rengolin at gmail.com>; Chris
> Tetreault <ctetreau at quicinc.com>
> *Subject:* [EXT] Re: [llvm-dev] RFC: Contributing Bazel BUILD files in
> the "peripheral" support tier
>
>
>
> Apologies for the delayed response here. I was out of the "office".
>
>
>
> Thanks for taking another look :-)
>
>
>
> I want to respond first to the process question of pitch vs RFC. My
> impression was that the pitch process should be used in the case that an
> RFC couldn't reach consensus. I asked a few times in the last thread (
> https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/uVlV3pMTBAAJ and
> https://groups.google.com/g/llvm-dev/c/u07o3QREVUg/m/wF5mu-dpBAAJ)
> whether I should move this to a pitch, but feel like there wasn't a clear
> response in the context of Renato's support tiers RFC.
>
>
>
> It seems like Tom and Renato still disagree about whether I should move
> this to a pitch. I would appreciate some consensus on that point at least
> :-D I do see the appeal of a living document for this sort of thing, so
> definitely see the appeal there, but also it seems like the pitch process
> is a heavier-weight and more unusual one, so I was hesitant. My inclination
> is to continue this as an RFC unless we are unable to reach consensus on
> the issue as outlined in the pitch process description. It does feel like
> this is really not quite as big a decision as you seem to be suggesting.
> It's also an easily reversible one since there are no build dependencies
> and everything is contained.
>
>
>
> On Mon, Nov 16, 2020 at 9:41 PM Tom Stellard <tstellar at redhat.com> wrote:
>
> > This should have approximately the same impact on the community as the
> > current GN build in `llvm/utils/gn` does today. That is, it should not
> > affect anyone who doesn't care.
> >
>
> I want to push back on this a little bit, because having the code in
> tree does impact everyone, even people who don't care about it.  It
> increases disk usage, commit traffic, checkout times, bugzilla / issue
> traffic, and CI builds to name a few things.  There are costs to having
> this in tree, the question (as always) is do the benefits outweigh the
> costs?
>
> Yes my apologies that this was poorly phrased. I was aiming for a pithy
> summary and a clear statement that our goal here is not to significantly
> impact contributors uninterested in Bazel. My impression is that the GN
> build has achieved that goal. I definitely agree that any addition to the
> monorepo should have a clear weighing of costs vs benefits and that the
> costs are never actually zero. I do think the costs here are really quite
> low however. I am happy to address your concerns and also think that it is
> important to note that if additional issues arise we are still agreeing to
> be on the hook for addressing them (e.g. if in practice this causes some
> unforseen issue with the release) and deleting this contribution if we
> cannot do so in a timely manner (`rm -rf utils/bazel` is all it requires).
>
>
>
> Personally, I do not think we should have alternative build systems in
> tree.  However, I still think you should try to propose this as a pitch.
> I would much rather this go through a fair process and land than for it
> to be rejected based on a contentious thread.
>
> Here is why I'm not convinced this should be in tree:
>
> To me it's not clear why having the build files in-tree is better than
> having a separate repo with an llvm-project sub-module.  The in tree
> bazel files will be broken from time to time, since most developers will
> not be updating them, however, with the sub-module approach you can
> ensure that the build will always work by pinning the llvm-bazel repo to
> a known-working commit of llvm-project.  Can you expand on the pros/cons
> of in-tree vs out-of-tree with sub-modules.
>
> Out-of-tree with a submodule is the current approach we have with
> https://github.com/google/llvm-bazel. It's certainly doable, but involves
> quite a bit of bookkeeping to track which version corresponds to a given
> version of LLVM such that someone can fetch the correct configuration
> (you'll note that the repository has about 7k tags at the moment). To make
> things somewhat more complicated, the typical way to fetch something for
> use in Bazel is with an http_archive
> <https://docs.bazel.build/versions/master/repo/http.html#http_archive> which
> requires one to specify the archive digest to avoid refetching on each
> build. This doesn't work particularly well with tags that change which
> commit they point to. I'm not saying these issues aren't solvable, but they
> add quite a bit of complexity.
>
>
>
> The other point is that I think this makes contributing to the Bazel
> configuration quite a bit more complex because you have to apply patches
> across multiple repositories to also be kept in sync. Given that LLVM has a
> monorepo, it still seems like the logical place for a build configuration
> of LLVM used by multiple projects.
>
>
>
> Other concerns I have from reviewing the patch:
>
> It seems like these are mostly concerns with the specific implementation.
> Would you be alright with saving the specific details for an eventual
> review on the patch if this moves forward? I've made brief responses below.
>
> * It looks like there is a build configuration for at least one external
> project (zlib) and possibly another (vulkan-headers?).  Do we really
> want to have build configurations for non-LLVM projects in our tree?  Is
> there any limit to the number of external projects that can and will be
> added?
>
> These are dependencies of the LLVM Project and LLVM keeps its dependencies
> pretty tightly managed AFAIU. These configurations are also pretty trivial,
> "here are the source files", type things, so I think it's even a bit
> generous to call them configurations: we're just informing Bazel where the
> files are located.
>
>
> * There are 3 files (abi-breaking.h.cmake, config.h.cmake,
> llvm-config.h.cmake) that have been copied from the llvm tree into
> utils/bazel/, is there some way we can avoid carrying multiple copies of
> the same file in tree?
>
>
> * Similarly, there are some files that are normally generated at build
> time clang/Config/config.h, llvm/Config/config.h,
> llvm/Config/llvm-config.h that have been copied into utils/bazel.  Is it
> really necessary
> to have these in tree?  Especially since some of the templates, like
> llvm-config.h.cmake, are also in utils/bazel?
>
> The copy here is pretty much orthogonal to the actual build configuration.
> The intent is to have a literal change detector test for changes to these
> cmake configurations, since they would invalidate assumptions in the Bazel
> configuration. Chandler and I went back and forth on a few different ways
> to do this. We can certainly look at other options. The issue is that I
> don't think there's actually a useful way to interpret the .cmake template
> files since changes to them are also made as changes to the cmake
> configuration and without these being in sync the files just drift. Happy
> to discuss other options for how to handle this. We could, for instance,
> have some other process that just looks at the git diff/log for these files.
>
>
>
> * I still worry about the bazel files causing merging conflicts when
> backported to the stable branch.  If these are added to tree, could we
> have a rule where commits to utils/bazel/ cannot include changes to
> other files?
>
> I'd certainly be open to discussing restrictions that would avoid
> additional burden on release managers. I think that one makes contributing
> to the Bazel configuration more difficult because you cannot do it as part
> of a patch that requires a change, but if it's something that would cause
> issues with the release then we can avoid it. My intuition is that this
> wouldn't actually come up often, however. For example, just looking at the
> gn directory I see several commits in the last week that touch this and
> other files. Have you actually run into issues? Since this is unsupported
> the conflicts could also be resolved pretty much however you wanted (e.g.
> delete the conflict markers, delete the file), so they seem pretty trivial
> to deal with if they only happen occasionally. My preference would
> therefore be to see if this is actually a problem in practice before
> putting rules in place.
>
>
>
>
>
> On Tue, Nov 17, 2020 at 2:27 AM Renato Golin <rengolin at gmail.com> wrote:
>
> Hi Geoffrey,
>
>
>
> Thanks for the re-submission.
>
>
>
> I have some comments below that may sound negative, but they're probably
> just a reflection of my own ignorance. I want to make sure the submission
> is clear, so it can be accepted on its own right.
>
>
>
> On Tue, 17 Nov 2020 at 03:02, Geoffrey Martin-Noble via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
>
>  This should not affect development of core tier components. One reason we
> propose adding this to the root utils/ directory instead of under
> llvm/utils (where GN is located) is to avoid unnecessarily sending messages
> to llvm-commits. Others have raised the concern that the existence of an
> alternative build system might lead to lack of maintenance for the CMake
> build system. Given that supporting CMake will remain a requirement and
> maintenance of a Bazel build system will continue to happen regardless, we
> do not expect any significant impact in this way.
>
>
>
> I was under the impression that "utils" was actually "llvm/utils", which
> would be in the same place as GN. I don't think we should treat GN and
> Bazel as different and I really wouldn't like to have a different quality
> control (for post commit reviews).
>
>
>
> If the Bazel commits are too verbose (for example, committing
> auto-generated code), then we should really clean that up and commit the
> script that generates them and make that part of the build.
>
>
>
> I understand the need to move the noise away, but move it too far away and
> it's no better than in a separate repo.
>
> I am happy to put this in either location and agree it should be in the
> same place as GN. If we were to decide that it should go `utils/` then I
> would also propose we move GN to there as well. I believe the GN files were
> contributed prior to the existence of the monorepo, so a top-level `utils/`
> wouldn't have been an option. I think living under the root `utils/`
> directory makes more sense because these are not configurations for only
> the LLVM subproject (we also build MLIR and Clang with perhaps more to
> come). I believe it was Mehdi's suggestion that this would help mitigate
> some of the costs to having it in the monorepo because Tom mentioned commit
> list traffic as a concern. I don't think I agree that one directory up is
> akin to a separate repo though :-D
>
>
>
> That said, this is a really minor point for me. I'm happy to put this
> wherever people prefer :-)
>
>
>
>
>
> A number of people raised the question of "why not a separate repository".
> This is indeed possible: It's what we've done with
> https://github.com/google/llvm-bazel, which is currently used by
> https://github.com/google/iree. It is significantly more infrastructure,
> coordination, and complexity for something that is specifically a
> configuration for the LLVM project itself, not its own dependent or
> adjacent project.
>
>
>
> I was also under the impression that one of the big reasons why we needed
> it to be in LLVM is that, like CMake, it needed files all over the place.
> This would indeed be a major infrastructure undertaking.
>
>
>
> But given that it's all being hosted in a single directory, and outside of
> the LLVM tree, I really can't see what's so much harder about an extra
> checkout in the same tree.
>
> Bazel *wants* the build files to be all over the place, but I've tricked
> it with some repository rule symlinking. That's also true of the LLVM GN
> configuration, I believe. My assumption is that having BUILD files actually
> throughout the repository would be something that would receive quite a bit
> of pushback and would be confusing for people who would naturally expect
> these BUILD files to be maintained as a supported build system. I would
> happily put a BUILD.bazel file at the root of each subproject and drop the
> symlinking madness, but I suspect this would not be embraced as a solution
> ;-P
>
>
>
> I believe this contribution will significantly improve the situation for
> downstream users that use Bazel while having minimal impact on the
> community at large.
>
>
>
> It's not clear to me yet if LLVM/Bazel is only used in Google projects or
> any other non-Google project. All that you listed so far seem to be
> exclusive to Google.
>
>
>
> This is not a problem per se, but it does promote the idea that Google
> could common it up internally instead.
>
>
>
> The main reasons why it would be upstream are that it's either a product
> by or requirement to the project itself, or it helps unite cross-industry
> collaboration that wouldn't be possible otherwise.
>
>
>
> It's clearly not the former (and why it's in the periphery tier), but it's
> also not clear it's in the latter either.
>
>
>
> I can really only speak for Google projects. I have also noticed several
> other Bazel build configurations in the wild, e.g. PlaidML
> <https://github.com/plaidml/plaidml/blob/master/vendor/llvm/llvm.BUILD> (Intel)
> or this bazel_llvm <https://github.com/ChrisCummins/bazel_llvm> project
> that I found after someone contributed a doc fix. I believe in the last
> thread someone from Facebook mentioned that Bazel build files would also be
> relatively easily translatable to their internal Bazel-derived build
> system, Buck. Someone from Lyft also expressed interest in using a Bazel
> build configuration if it was in-tree. But I can't really speak to the
> motivations, road maps, etc. for any of these people, companies, or
> projects (if you're reading, please chime in ;-P).
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201207/05d0d723/attachment-0001.html>