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

feat: support bzlmod #17

Merged
merged 6 commits into from
Nov 21, 2023
Merged

feat: support bzlmod #17

merged 6 commits into from
Nov 21, 2023

Conversation

alexeagle
Copy link
Collaborator

fixes #12

@alexeagle alexeagle requested a review from p0deje November 18, 2023 02:05
Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

There are some failures on CI which I am not sure how to fix.

.bcr/config.yml Show resolved Hide resolved
.bcr/presubmit.yml Show resolved Hide resolved
MODULE.bazel Show resolved Hide resolved

ruby = use_extension("@rules_ruby//ruby:extensions.bzl", "ruby")

ruby.toolchain(name = "rules_ruby", version = "3.2.1")
Copy link
Member

Choose a reason for hiding this comment

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

This should load the version from ruby_version.bzl until multiple toolchains are implemented.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the MODULE.bazel file cannot load(). We can discuss alternative ways to make these line up.

@alexeagle
Copy link
Collaborator Author

The failure is because of the label here: https://github.com/p0deje/rules_ruby/blob/575267f22078de32b7aaecb74a178cc3753ab284/ruby/private/bundle.bzl#L13

The user declares a rules_ruby_dist when the toolchain resolves, but that repository is visible only to them, not to rules_ruby itself. We'll need a bit of refactoring to green it up, I'll look again when I have some fresh brain cells for it.

@alexeagle alexeagle force-pushed the bzlmod branch 2 times, most recently from 5429214 to 7ff169f Compare November 20, 2023 16:07
@alexeagle alexeagle marked this pull request as ready for review November 20, 2023 16:38
@alexeagle
Copy link
Collaborator Author

I've taken the simple answer, which is a small refactor to move that hardcoded label from rules_ruby and instead the user must provide it.

To make it non-breaking for existing WORKSPACE users I've used a macro implicit string->Label conversion to supply a default value.

I'm not sure this is a great answer, but I think the rb_bundle rule could use some redesign thinking anyhow, as it's doing too much in a repository context. So I think we could land this for now, so that we establish bzlmod compatibility, with the tradeoff that it will just slow us down a bit in the future as any refactorings will have to work for both bzlmod and WORKSPACE.

Copy link
Member

@p0deje p0deje left a comment

Choose a reason for hiding this comment

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

LGTM - my only concern is over testing in CI at the moment since we always use MRI 3.2.1 and there is no way to change it for bzlmod.

.bcr/config.yml Show resolved Hide resolved
</pre>


Installs Bundler dependencies and registers an external repository
Copy link
Member

Choose a reason for hiding this comment

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

Shall we move this documentation for the rule to the macro?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My solution for this, at least with current stardoc, is to document both. Described at bazelbuild/stardoc#98 (comment)

@alexeagle
Copy link
Collaborator Author

concern is over testing in CI at the moment since we always use MRI 3.2.1 and there is no way to change it for bzlmod

Filed #23 for a long-term answer. For now I'll change this to just have a bzlmod job with its own matrix that doesn't have a dimension for ruby interpreter.

@alexeagle alexeagle merged commit b9c7691 into bazel-contrib:main Nov 21, 2023
36 checks passed
@alexeagle alexeagle deleted the bzlmod branch November 21, 2023 18:05
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
2 participants