[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