[llvm-dev] RFC: Contributing Bazel BUILD files in the "peripheral" support tier
Tom Stellard via llvm-dev
llvm-dev at lists.llvm.org
Fri Dec 4 20:56:24 PST 2020
On 12/4/20 8:17 PM, Mehdi AMINI wrote:
>
>
> On Fri, Dec 4, 2020 at 8:07 PM Tom Stellard <tstellar at redhat.com
> <mailto: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>
> <mailto: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.
>
My understanding of the policy is that these categories of things still
need to be approved in order to be added to the tree. Am I correct, or
does this policy allow anyone to add an alternative build system as long
as they can satisfy the support requirements.
> 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.
I don't really agree that all the objections were addressed. Maybe we
should directly reach out to people from the original thread and ask them?
I'm not really a fan of having another build system in tree, but I also
don't want to keep devoting a lot of time to arguing about it. I was
hoping that with the pitch process, we could avoid the kind of back and
forth arguing on the list that typically make these RFCs so tiring.
I still don't quite understand why there is so much push back against
pitches, but I think everyone knows my perspective now, so I'm going to
step back and let other people work out what the next steps should be.
-Tom
> 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>>
> > > <mailto: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>>
> > > <mailto: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>>
> > <mailto: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>
> <mailto:llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>>
> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >
>
More information about the llvm-dev
mailing list