[llvm-dev] RFC: Contributing Bazel BUILD files in the "peripheral" support tier
Mehdi AMINI via llvm-dev
llvm-dev at lists.llvm.org
Fri Dec 4 20:17:01 PST 2020
On Fri, Dec 4, 2020 at 8:07 PM Tom Stellard <tstellar at redhat.com> wrote:
> On 12/4/20 7:19 PM, Mehdi AMINI wrote:
> >
> >
> > On Fri, Dec 4, 2020 at 6:42 PM Tom Stellard via llvm-dev
> > <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
> >
> > On 12/3/20 4:27 PM, Geoffrey Martin-Noble wrote:
> > > 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.
> > >
> >
> > I still think this should be a pitch. The original mailing list
> > discussion was controversial and that's when an RFC should be
> escalated
> > to a pitch according to: [1].
> >
> >
> > You may have missed it, but in the meantime there has been another RFC
> > clarifying our policy though: https://llvm.org/docs/SupportPolicy.html
> > It seems fair to me to revisit this RFC as is in light of the policy
> change.
> >
>
> I don't think the questions about whether or not this should be included
> in the project are answered by this new policy.
I'll quote the policy:
> Section: "What is covered"
> The peripheral tier is composed of:
> Experimental targets and options that haven’t been enable by default yet.
> Main repository projects that don’t get released or regularly tested.
> Legacy tools and scripts that aren’t used in upstream validation.
> *Alternative build systems (ex. GN, Bazel) and related infrastructure.*
The intent of the policy is to cover exactly this proposal.
> To me the part about
> how the bazel build files were going to be supported and what
> responsibility the community had for maintaining them was always very
> clear.
>
> > I'd actually like to request that the objections are reiterated and
> > positioned in terms of the policy before we escalate this.
> >
>
> I don't think it's really fair to ask people to re-object to the
> proposal.
Why?
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.
On the contrary it seems not only fair to me, but necessary.
> In my opinion, one of the problems with RFCs in the past is
> that they turn into an endurance test, because there is no process for
> making a decision. Either the proposer gets tired of asking and gives
> up or the objectors get tired of objecting and give up. We have a
> decision process now with the pitch process, and I think we should use it
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.
>
> >
> > Thank you for responding to my technical concerns, and I agree that
> > working out most of those details may be better left for a patch
> review
> > discussion. But I think at least the presence of build information
> for
> > other projects and the sub-module alternative should be mentioned in
> > the
> > pitch.
> >
> > If there were only technical or support policy issues like these to
> > resolve then I don't think this would be controversial and require a
> > pitch.
> >
> > My main issue with this RFC, (which I tried to address at the end of
> my
> > previous mail), is the precedent this sets for what gets included in
> > tree. Essentially, we have a subset of our community that chose to
> > go a
> > different direction from upstream, as always there are costs and
> > benefits with this decision. The question for the community is do we
> > want to help or encourage this in the future by removing some of the
> > costs of these decisions and allowing alternative implementations to
> > live in tree.
> >
> > Maybe for build systems this is OK, and for other things this is not,
> > I don't know. But if we are going to be setting a precedent, to me,
> > the
> > best way to do this is through the pitch process.
> >
> >
> > Why are you considering this "setting a precedent" while there is
> > already GN in tree?
> >
>
> You are right we are not really setting a precedent here, because GN is
> already in tree. However, I don't think we should now just allow any
> build system to be added to the tree just because GN is there. We need
> to have some kind of process and criteria for deciding what gets added
> and what doesn't. I think a pitch will help accomplish this.
>
> I'll be honest, I don't really understand why there is so much push back
> on turning this into a pitch. Is it really that much extra work?
>
> -Tom
>
> > --
> > Mehdi
> >
> >
> >
> >
> > -Tom
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> >
> > [1]
> >
> https://github.com/llvm/llvm-www/blob/master/proposals/LP0001-LLVMDecisionMaking.md
> >
> > > On Mon, Nov 16, 2020 at 9:41 PM Tom Stellard <tstellar at redhat.com
> > <mailto:tstellar at redhat.com>
> > > <mailto:tstellar at redhat.com <mailto: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
> > <mailto:rengolin at gmail.com>
> > > <mailto:rengolin at gmail.com <mailto: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 <mailto:llvm-dev at lists.llvm.org>
> > <mailto:llvm-dev at lists.llvm.org <mailto: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 <mailto: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/20201204/e156cadd/attachment-0001.html>
More information about the llvm-dev
mailing list