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

Add support for bzlmod #43

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Add support for bzlmod #43

merged 6 commits into from
Feb 22, 2024

Conversation

jwnrt
Copy link
Contributor

@jwnrt jwnrt commented Feb 4, 2024

Summary

Hi, this PR adds bzlmod support for this library and resolves #39.

I've had to update some dependencies to match the versions available in the bazel central registry. Where there was no semver-compatible version in the registry, I've updated the dependency to its latest version. The versions in the WORKSPACE and MODULE.bazel should match.

I've also updated some attribute names to use allowlist which extends this library's support to Bazel v7.0, but drops support for Bazel versions pre June 2020.

Help needed

Buildifier built from source isn't available in the BCR right now, but a prebuilt binary is. I hope this change is acceptable, but at the same time it breaks the example and I'm not sure how to resolve the error.

The binary has a default_runfiles file at external/buildifier_prebuilt~6.4.0~buildifier_prebuilt_deps_extension~buildifier_linux_amd64/file/buildifier, but the rlocation call here for this path returns empty.

Should this file just be skipped if it has no rlocation? I'm no Bazel expert so admit I don't know exactly what's going on here.

Thanks!

Copy link
Collaborator

@bttk bttk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this PR!

I'm not sure about the introduction of buildifier_prebuilt
Without it, looks good!

MODULE.bazel Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
@jwnrt
Copy link
Contributor Author

jwnrt commented Feb 6, 2024

I'm not sure about the introduction of buildifier_prebuilt

I understand this being a problem, in that case perhaps we can close this and wait to see if the upstream repo migrates to bzlmod (here's a tracking issue).

Copy link
Member

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Sorry for the delay, github notifications are easy to miss, so I stumbled upon this on accident.

It looks like it makes sense to review some of the dependencies and prune where possible. Less things we depend on the less things can break in the future in weird ways :)

WORKSPACE Outdated Show resolved Hide resolved
WORKSPACE Outdated Show resolved Hide resolved
Bazel moved to using "allowlist" in Jun 2020 and dropped support for the
legacy names in Sep 2023, so newer versions of Bazel don't work.
This version is in the Bazel Central Registry.
@jwnrt
Copy link
Contributor Author

jwnrt commented Feb 21, 2024

I've updated the PR to remove buildifier entirely. Development (and CI) now uses the system buildifier for formatting. I've changed the example which installed buildifier to now install a hello-world C binary.

@hzeller
Copy link
Member

hzeller commented Feb 21, 2024

Less dependencies is good, yay. What do you think @bttk ?

I wonder why the bazelmod.lock files are so gigantic. It makes it essentially impossible to review and look out for potential security risks of whatever it imports. Can these be trimmed to the minimum needed ? (I don't know anything about bzlmod).

@jwnrt
Copy link
Contributor Author

jwnrt commented Feb 21, 2024

The lock files are indeed huge, but I don't see any options for compressing or shortening them. It looks like they include all of the built-in dependencies like bazel_tools which probably contributes to their size quite a lot.

See bazelbuild/bazel#19971 for an open issue about removing built-ins from the lock.

It looks like rules_python has chosen to exclude the lock entirely. What do you think about that approach?

@hzeller
Copy link
Member

hzeller commented Feb 21, 2024

I think I like that approach not to include the *.lock file and consider it an ephemeral artifact.
(In particular since this particular project is not super-sensitive to super-exact versions of what we depend on. Essentially, as long as a shell script can be executed, we're good).

@hzeller
Copy link
Member

hzeller commented Feb 21, 2024

Nice!

I think it would be good if we mention the range of versions supported of bazel with this (in the README somewhere).
This repo should just work from bazel 5 to 7 right ?

Anything you'd like to add @bttk ? Are we ready to merge ?

Copy link
Collaborator

@bttk bttk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@hzeller hzeller merged commit 6001fac into google:main Feb 22, 2024
2 checks passed
@hzeller
Copy link
Member

hzeller commented Feb 22, 2024

Thanks for the PR, merged!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Bzlmod
3 participants