Page MenuHomePhabricator

Introduce a Bazel build configuration
ClosedPublic

Authored by GMNGeoffrey on Oct 28 2020, 4:06 PM.

Details

Summary

This patch introduces configuration for a Bazel BUILD in a side
directory in the monorepo.

This is following the approval of
https://github.com/llvm/llvm-www/blob/main/proposals/LP0002-BazelBuildConfiguration.md

As detailed in the README, the Bazel BUILD is not supported
by the community in general, and is maintained only by interested
parties. It follows the requirements of the LLVM peripheral tier:
https://llvm.org/docs/SupportPolicy.html#peripheral-tier.

This is largely copied from https://github.com/google/llvm-bazel,
with a few filepath tweaks and the addition of the README.

Diff Detail

Event Timeline

GMNGeoffrey created this revision.Oct 28 2020, 4:06 PM
GMNGeoffrey requested review of this revision.Oct 28 2020, 4:06 PM
GMNGeoffrey edited the summary of this revision. (Show Details)Oct 28 2020, 4:23 PM
keith added a subscriber: keith.Oct 28 2020, 5:39 PM

Could you add something similar to https://github.com/llvm/llvm-project/blob/master/llvm/utils/gn/README.rst?

  • How to build something?
  • How to use clang/llvm/mlir as an external dependency?
tschuett added inline comments.Oct 29 2020, 4:12 AM
utils/bazel/.bazelrc
96

This page does not exist.

aartbik added inline comments.Oct 29 2020, 10:05 AM
utils/bazel/llvm-project-overlay/mlir/test/BUILD
1 ↗(On Diff #301466)

don't we need a build file for mlir/integration_test as well?

Could you add something similar to https://github.com/llvm/llvm-project/blob/master/llvm/utils/gn/README.rst?

  • How to build something?
  • How to use clang/llvm/mlir as an external dependency?

Yes I'll work on adding such a readme. My intent was to do so based on the discussions in the RFC

utils/bazel/.bazelrc
96

It's an alpha project so it's docs are apparently locked down :-/ I'll investigate further.

utils/bazel/llvm-project-overlay/mlir/test/BUILD
1 ↗(On Diff #301466)

If we want to build the integration_test. The focus here is on usability for dependent projects

  • Add README
  • Pull in new changes from llvm-bazel repository
  • Add coverage info and usage instructions
Ka-Ka added a subscriber: Ka-Ka.Nov 23 2020, 12:08 AM
aartbik removed a reviewer: aartbik.Dec 2 2020, 1:57 PM
aartbik resigned from this revision.Dec 2 2020, 1:59 PM
csigg added a subscriber: csigg.Mar 26 2021, 5:29 AM
csigg added inline comments.
utils/bazel/WORKSPACE
16

Downstream projects need to replicate everything in this WORKSPACE file, and it would therefore be good to keep it small.

My recommendation would be to move loading of external dependencies to llvm_configure, with the exception of

  • load/call bazel_skylib_workspace(): this initializes a unittest toolchain and downstream project do not need it to depend on LLVM (I'm not even sure this repo uses it)
  • load/call rbe_autoconfig(): downstream projects would need to replicate that plus copy and adjust the .bazelrc if they want to use a similar RBE setup.
vnvo added a subscriber: vnvo.Apr 6 2021, 4:40 PM

Pull in new updates from github.com/google/llvm-bazel

GMNGeoffrey added inline comments.May 18 2021, 3:55 PM
utils/bazel/WORKSPACE
16

I actually mostly disagree for this case. All of the things that are configurable here are deliberately configurable. In fact we would not recommend that a user use the *_from_env versions of the rules that provide these repositories, as they likely just want to use a single version. Some of these dependencies are also optional. We also want to make sure that users are conscious of their dependencies (Chandler, in particular, pointed out that some of this has implications for licensing).

LLVM does not pull in new dependencies often, so I don't think this will create much churn for downstream users. As an example of what this looks like, this is what IREE needs to do to configure it's dependency: https://github.com/google/iree/blob/main/WORKSPACE#L42-L69

GMNGeoffrey edited the summary of this revision. (Show Details)May 18 2021, 3:58 PM
csigg added inline comments.May 19 2021, 3:27 AM
utils/bazel/WORKSPACE
16

I think it would be helpful to have IREE's LLVM dependency setup provided here as a repo macro, assuming it's sufficiently useful to get started and has no extra licensing implications.

If you prefer to keep it as is, please document which significant sub-targets/packages require which dependencies. Some of them are optional, and a user shouldn't have to guess.

GMNGeoffrey added inline comments.May 20 2021, 4:25 PM
utils/bazel/WORKSPACE
16

Addressing in https://github.com/google/llvm-bazel/pull/213, which I will then pull into here.

shahms added a subscriber: shahms.May 21 2021, 8:34 AM

In case it wasn't clear from my recent updates, I believe this is now ready for review. PTAL

ormris removed a subscriber: ormris.Thu, Jun 3, 10:26 AM
GMNGeoffrey updated this revision to Diff 351565.EditedFri, Jun 11, 2:44 PM

Pull in new updates from llvm-bazel

Reviewers, could you please take a look at this patch?

keith accepted this revision.Wed, Jun 16, 11:57 AM
keith added inline comments.
utils/bazel/README.md
30

should this mention bazelisk?

utils/bazel/llvm-project-overlay/llvm/binary_alias.bzl
26

skylib has native_binary which appears to do the same thing as this custom rule

This revision is now accepted and ready to land.Wed, Jun 16, 11:57 AM
GMNGeoffrey added inline comments.Wed, Jun 16, 12:34 PM
utils/bazel/README.md
30

I don't want to repeat Bazel docs or be too opinionated about how exactly someone gets this. Personally I don't use bazelisk and instead use the wrapper shell script that's been around for a while that delegates to the appropriate bazel version (but requires that it be installed). Bazelisk is mentioned in the Bazel install docs we link to, so I think it's ok to defer to them. WDYT?

utils/bazel/llvm-project-overlay/llvm/binary_alias.bzl
26

Oh nice! That looks relatively new. Let me look into swapping that in. I think currently the main repo doesn't have a dependency on skylib because our only usage in the project was for the config diff tests, which dependent users don't need and it seemed like an unnecessary dependency. This would maybe be worth tipping us over the edge.

keith added inline comments.Wed, Jun 16, 12:38 PM
utils/bazel/README.md
30

Yea I think that's fine, it just might mean folks end up here using the wrong version because without bazelisk installing an old version takes more steps

kuhar added a subscriber: kuhar.Wed, Jun 16, 1:03 PM
kuhar added inline comments.
utils/bazel/WORKSPACE
77

Is there a reason for using this particular version? I think the most recent release is 4.x.

utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/llvm-config.h
75

This seems outdated. Could we have a script that generates this config file based on one produced with Cmake?

GMNGeoffrey added inline comments.Wed, Jun 16, 2:03 PM
utils/bazel/WORKSPACE
77

It was the newest version when we originally wrote these and there hasn't been a major reason to upgrade. I've bumped it to 4.0.0 in https://github.com/google/llvm-bazel/pull/229, which I think is the latest LTS release, if I understand https://blog.bazel.build/2020/11/10/long-term-support-release.html correctly.

utils/bazel/llvm-project-overlay/llvm/include/llvm/Config/llvm-config.h
75

We've looked at various scripting ideas but it's pretty non-trivial. The best we came up with was the current approach of manually specifying everything and then having a test that checks when the config .cmake files change. It looks like this is an example of something that falls through for that because it's only updated in the CMakeLists.txt :-/ For now I'll update this to 13

GMNGeoffrey added inline comments.Wed, Jun 16, 2:42 PM
utils/bazel/llvm-project-overlay/llvm/binary_alias.bzl
26

Hmmm it seems like that requires an explicit out name for some reason, which is pretty annoying. Also does a copy instead of a symlink, which it claims is necessary. Right now I think we get the rule name to be the same as the binary name, which is desirable. Not a huge deal, but at least gives me pause for a moment.

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Or if there's some part of this review that could be extracted (is there sort of a "null" implementation - maybe literally just the top level utils/bazel directory, empty, or with a README, etc) and committed with that approval, and then invested bazel-using folks could use this review thread for further discussion on the design of the bazel build files?

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Or if there's some part of this review that could be extracted (is there sort of a "null" implementation - maybe literally just the top level utils/bazel directory, empty, or with a README, etc) and committed with that approval, and then invested bazel-using folks could use this review thread for further discussion on the design of the bazel build files?

I'm happy to restructure this patch in that way if others would prefer (check in the README first). I did think about something like that, but I thought that non-Bazel folks might want to review some of the actual substance. I figure that the approval of the proposal already means that just the directory existing is no longer at issue, so there's not much gained by splitting it out. One thing to note is that these files have already been reviewed by at least one other LLVM community member as they've been incrementally created. Chandler wrote (ported) the bulk of them with my review and subsequent updates have all had review.

It's not really clear to me what is required to land here. The proposal for adding these has been approved and I now have another community member "accepting" the patch as well as a few others commenting. It's been around for plenty of time to allow sufficient review. I think LLVM has some norms around this and for "normal" patches I think I have a good feel for what those are, but on this one I'm a little less sure :-)

Pull in latest changes from llvm-bazel

dblaikie accepted this revision.Thu, Jun 17, 2:00 PM

Might make sense to have someone from the committee that approved the proposal sign off on these files being checked into this location (with no review of the content/quality of the files) - and perhaps to commit at that point (regardless of the content/quality of the files) - then do the actual semantic review separately.

Or if there's some part of this review that could be extracted (is there sort of a "null" implementation - maybe literally just the top level utils/bazel directory, empty, or with a README, etc) and committed with that approval, and then invested bazel-using folks could use this review thread for further discussion on the design of the bazel build files?

I'm happy to restructure this patch in that way if others would prefer (check in the README first). I did think about something like that, but I thought that non-Bazel folks might want to review some of the actual substance.

They might, but I think owing to the nature of this proposal (adding files that are mostly unrelated to/meant to be ignorable by contributors) there's not going to be a lot of folks who have an opinion on the bazel file structure/contents themselves & so I doubt anyone will likely feel responsible/informed enough to sign off on their contents. You're the code owner & the code doesn't share much with the rest of the project (in terms of code style, design, etc) so there's not really much else to discuss, I think.

I figure that the approval of the proposal already means that just the directory existing is no longer at issue, so there's not much gained by splitting it out. One thing to note is that these files have already been reviewed by at least one other LLVM community member as they've been incrementally created. Chandler wrote (ported) the bulk of them with my review and subsequent updates have all had review.

It's not really clear to me what is required to land here. The proposal for adding these has been approved and I now have another community member "accepting" the patch as well as a few others commenting. It's been around for plenty of time to allow sufficient review. I think LLVM has some norms around this and for "normal" patches I think I have a good feel for what those are, but on this one I'm a little less sure :-)

Given the nature of this, I think there's not much approval required beyond the proposal approval - beyond that, it's basically yours (& anyone else who stands up to say they care deeply about bazel build files) to do with as you please.

kuhar accepted this revision.Thu, Jun 17, 2:03 PM

Given the nature of this, I think there's not much approval required beyond the proposal approval - beyond that, it's basically yours (& anyone else who stands up to say they care deeply about bazel build files) to do with as you please.

+1 to what David said and an LGTM from me.

It will be much easier to iterate on the implementation details once the initial version is landed.

echristo accepted this revision.Thu, Jun 17, 4:13 PM

As part of the group reviewing here's an explicit ack. Happy to do post commit review on the rest.

Pull in changes from llvm-bazel up to llvm commit b3634d3e88b7

Ok I'm going to go ahead and land this

This revision was landed with ongoing or failed builds.Tue, Jun 22, 12:48 PM
This revision was automatically updated to reflect the committed changes.