[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 21:28:17 PST 2020
On Fri, Dec 4, 2020 at 8:56 PM Tom Stellard <tstellar at redhat.com> wrote:
> 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.
>
I agree with you, but that does not make it a justification for a pitch
automatically.
The policy gives us a framework to discuss and avoids to rehash the same
set of arguments over and over: for example we don't object to a specific
proposal because we don't agree with the policy, but because of the
specifics of a particular proposal.
Similarly, an RFC or a pitch shouldn't have to re-evaluate what has been
codified as OK in a policy, otherwise what is the point of codifying it in
the first place if everything is always revisited?
> > 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?
>
Seems like we agree in the end: we need the objections to be restated :)
>
> 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.
>
Can only speak for myself: I believe pitches should be more of a "last
resort" than "the normal way of driving any proposal". I object to what I
see using too easily a "pitch" as a way to "work around discussions".
--
Mehdi
>
> -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
> > >
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201204/3aa2fdbf/attachment.html>
More information about the llvm-dev
mailing list