search

google / bazel_rules_install

Bazel rules for installing build results. Similar to `make install`.
Apache License 2.0
37 stars 14 forks source link

Add support for bzlmod #43

Closed jwnrt closed 9 months ago

jwnrt commented 9 months ago

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!

jwnrt commented 9 months ago

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).

jwnrt commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 https://github.com/bazelbuild/bazel/issues/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 commented 9 months ago

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 commented 9 months ago

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 ?

hzeller commented 9 months ago

Thanks for the PR, merged!