Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bazel] Add support for bzlmod module system #55924

Open
aaronmondal opened this issue Jun 7, 2022 · 17 comments
Open

[bazel] Add support for bzlmod module system #55924

aaronmondal opened this issue Jun 7, 2022 · 17 comments
Assignees
Labels
bazel "Peripheral" support tier build system: utils/bazel

Comments

@aaronmondal
Copy link
Member

aaronmondal commented Jun 7, 2022

We are currently working on making the Clang/LLVM Bazel overlay available via the bzlmod system. We noticed that parts of our code may be desirable to have in the Clang/LLVM overlay in this repo.

What works

  • We have a working MODULE.bazel file here (ignore the rules_ll specific stuff).
  • An extensions.bzl file to apply the overlay etc. is here, mostly copied from configure.bzl.

What doesn't work

I would expect that we would need to fix the following issues before something like this would be acceptable for upstreaming:

Conclusion

Please let me know whether bzlmod support is something that would have a place in the bazel overlay in this repository. If it is, please also let me know which issues, apart from those already mentioned we would have to address.

@fmeum
Copy link
Contributor

fmeum commented Jun 7, 2022

@aaronmondal If you test with recent rolling releases of Bazel, local overrides of modules also get a repo name with a suffix, which means that you can more easily reproduce issues caused by that locally.

bazelbuild/bazel#15553 contains some ideas on how to handle bzlmod repo names in copts.

@aaronmondal
Copy link
Member Author

@fmeum Thanks for the infos! The first one should make testing a lot easier. The second one may be able to fix the issue with //clang:ast, and may help with //clang:builtin_headers_gen. I'll post an update as soon as I implemented it on our side 😊

@aaronmondal
Copy link
Member Author

Ok things seem to be working now. I updated the initial post and will send a patch for review.

@aaronmondal
Copy link
Member Author

Some groundwork proposed in https://reviews.llvm.org/D136496. Also opened an issue in the BCR: bazelbuild/bazel-central-registry#206

For those interested, I am upstreaming from https://github.com/eomii/bazel-eomii-registry/tree/llvm-project-overlay/modules/llvm-project-overlay.

@fmeum
Copy link
Contributor

fmeum commented Oct 21, 2022

@aaronmondal Maybe I just haven't figured out Phabricator, but I think I'm unable to comment on the LLVM revision, so I'm going to comment here.

You should indeed be able to drop the Label constructors except for the one line where you pass the label to repository_ctx.path. Other than that, the PR looks good to me and should be a no-op in WORKSPACE builds.

The way you patch a repo through a symlink looks slightly "adventurous" and I'm not 100% convinced it won't break some assumptions Bazel makes about external repos, but if it's only going to be used by LLVM devs, I think that would be acceptable.

@aaronmondal
Copy link
Member Author

@fmeum

You should indeed be able to drop the Label constructors except for the one line where you pass the label to repository_ctx.path. Other than that, the PR looks good to me and should be a no-op in WORKSPACE builds.

Yeah that was indeed an oversight. I keep losing track of all the changes locally in all the different repos 😅

The way you patch a repo through a symlink looks slightly "adventurous" and I'm not 100% convinced it won't break some assumptions Bazel makes about external repos, but if it's only going to be used by LLVM devs, I think that would be acceptable.

The easiest variant is for sure to use a http_archive. Patching the symlinked copy works with the initial build, but breaks rebuilds since the original sources will be modified. This is especially annoying when one wants to test changes via a local_override for the llvm-project. The symlinked copy should only be available when one uses no patches. This is an error in the current implementation in the demo registry where I accidentally swapped some if-else statements that check for this.

@aaronmondal aaronmondal self-assigned this Apr 16, 2023
aaronmondal added a commit that referenced this issue May 7, 2023
Prefix occurrences of `//utils/bazel` with an explicit `@llvm-raw`.

This change lets us reuse code from `configure.bzl` in future compatibility patches for the bzlmod
module system.

The llvm-project overlay will be made available as an `@llvm-project-overlay` (name WIP) module in
the Bazel Central Registry. This means that we will have an `@llvm-project-overlay` workspace in
addition to the `@llvm-raw` and `@llvm-project` workspaces currently involved in the build. To keep
future patches to the existing build files as small as possible, the explicit naming proposed in this
change appears to be the simplest way to not confuse the module workspace resolution.

This is not a functional change to the current WORKSPACE build. It is a foundation for future patches.

GitHub Issue in the BCR: bazelbuild/bazel-central-registry#206
GitHub Issue in LLVM: #55924

Reviewed By: csigg

Differential Revision: https://reviews.llvm.org/D136496
@arsenm arsenm added bazel "Peripheral" support tier build system: utils/bazel and removed new issue labels Aug 17, 2023
@keith
Copy link
Member

keith commented Mar 21, 2024

@aaronmondal where did you leave this one, are you interested in working on this still? I might be interested in picking it up

@aaronmondal
Copy link
Member Author

@keith Whoops we had to delete and recreate the registry I posted previously for various reasons. The patches are slightly outdated but should still be mostly functional: https://github.com/eomii/bazel-eomii-registry/tree/main/modules/llvm-project-overlay/17-init-bcr.3

@aaronmondal
Copy link
Member Author

aaronmondal commented Mar 22, 2024

Hmm unless we want to add root-level MODULE.bazel, WORKSPACE.bazel and BUILD.bazel files I think we need to wait for a release with bazelbuild/bazel@6f254ce

That PR adds new_local_repository to MODULE.bazel which lets us create these files on the fly.

@keith
Copy link
Member

keith commented Mar 25, 2024

I think for now we could call the original APIs with a module_extension to get the same behavior, but I haven't tried

@aaronmondal
Copy link
Member Author

Tested this again. Still won't work. We can't use native.* rules in module extensions and the newly added starlark implementations for local_repository and new_local_repository appear to be broken:

bazelbuild/bazel#18285 (comment)

@keith
Copy link
Member

keith commented Apr 16, 2024

instead of doing that could we use a fake bazel_dep(name="foo") and then a local_path_override?

@aaronmondal
Copy link
Member Author

That would work, if we could add a MODULE.bazel, WORKSPACE.bazel and BUILD.bazel to the root of the project. TBH I've played around with this idea for a while now, but since it's not just one but three files (and a fourth MODULE.bazel.lock if we want to secure against supply chain vulns) I'm not sure how feasible this is. It would unblock this though and I'd be in favor of doing this.

@keith
Copy link
Member

keith commented Apr 16, 2024

oh i was thinking there would be some way to make that work with it in the overlay still. i agree that would be ideal but am not sure how much pushback there would be. if we only supported bzlmod you only need the MODULE.bazel and BUILD.bazel (no WORKSPACE* required). if you wanted to keep supporting both i think we'd also need a WORKSPACE.bzlmod to make bazel ignore the WORKSPACE.bazel when bzlmod is enabled

@aaronmondal
Copy link
Member Author

Ha! It seems like I messed something up with the local_* rules. I believe I have something that works. Will send a PR later 🚀

@aaronmondal
Copy link
Member Author

aaronmondal commented Apr 16, 2024

@nickdesaulniers
Copy link
Member

bazel 8.0.0 now prints a warning that "WARNING: WORKSPACE support will be removed in Bazel 9 (late 2025), please migrate to Bzlmod, see https://bazel.build/external/migration."

As a heads up, it sounds like "late 2025" might be a deadline for this bug that's been actively worked on+open for years. See also #119425.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel
Projects
None yet
Development

No branches or pull requests

5 participants