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 #287

Closed
wants to merge 14 commits into from
Closed

Add support for bzlmod #287

wants to merge 14 commits into from

Conversation

keith
Copy link
Contributor

@keith keith commented Mar 27, 2024

Fixes #220

@keith
Copy link
Contributor Author

keith commented Mar 27, 2024

The hardest part of this integration is to pull in the starlark library from bazel, right now some of the bazel build config is duplicated here, although that should be removable.

Open question: would you be open to dropping the non-bzlmod based build? I haven't touched that yet here, I think we could make both work at once, but we could also delete the old one if it's not necessary with this.

@keith
Copy link
Contributor Author

keith commented Mar 27, 2024

It turns out that it's easy enough to keep non-bzlmod working, so I made the minor adjustments required there

@@ -19,15 +19,15 @@ licenses(["notice"])
genrule(
name = "normalised_buildozer",
testonly = 1,
srcs = ["@buildtools//buildozer"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

buildtools isn't in the bcr yet, this repo vendors the downloads directly from that repo's releases

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

We currently automatically vendor buildtools from HEAD because we had needs to be pretty close to the edge. I wonder if this can be treated like io_bazel?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the problem is that pulling in buildtools that way would require pulling rules_go and resolving the go.mod transitive dependencies of that repo as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. I will look into merging this provided we don't end up with too much version skew for build_tools vs the internal monorepo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good, i don't feel like there are a ton of changes there enough to cause major drift

)
maybe(
maven_install,
name = "maven",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

for bzlmod it's better to have a unique name for a repository that might be a dependency of something else, but for building bazel we have to use this name as well, right now these are dup'd but in bzlmod they have a separate set of deps

third_party/bazel/rules_jvm_external_6.0.patch Outdated Show resolved Hide resolved
@hsudhof
Copy link
Collaborator

hsudhof commented Apr 3, 2024

Funnily enough, I have a very similar change in my local repo and ran into exactly the same issue.

@hsudhof hsudhof self-assigned this May 6, 2024
@keith keith marked this pull request as ready for review June 25, 2024 16:50
@keith
Copy link
Contributor Author

keith commented Jun 25, 2024

we've been using this internally for a while, i think it's good to go

@psalaberria002
Copy link

This has been pending for a while, could someone review it? It would be very nice to have it merged.

@sin-ack
Copy link

sin-ack commented Oct 7, 2024

Can confirm this works in combination with #233. I did this:

# MODULE.bazel
bazel_dep(name = "copybara", version = "")

# NOTE: This commit is from a PR with Bzlmod support.
# TODO: Replace with a release once it's cut with this PR merged.
git_override(
    module_name = "copybara",
    commit = "791cd9684e28b3bdb7ab4c54dc2e457d8f0b58eb",
    remote = "https://github.com/google/copybara.git",
)

Then just bazel run @copybara//java/com/google/copybara:copybara.

@hsudhof
Copy link
Collaborator

hsudhof commented Oct 11, 2024

Thank you! I will probably remove the wholesale bazel dependency as we only use a tiny part of the codebase that seems suitable for vendoring - just to avoid the churn of the ever growing dependency list.

@keith keith deleted the ks/add-bzlmod branch October 11, 2024 17:57
@keith
Copy link
Contributor Author

keith commented Oct 11, 2024

thanks!

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 for bzlmod
5 participants