-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
There was a problem hiding this 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.
examples/gem/MODULE.bazel
Outdated
|
||
ruby = use_extension("@rules_ruby//ruby:extensions.bzl", "ruby") | ||
|
||
ruby.toolchain(name = "rules_ruby", version = "3.2.1") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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 |
5429214
to
7ff169f
Compare
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 |
There was a problem hiding this 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.
</pre> | ||
|
||
|
||
Installs Bundler dependencies and registers an external repository |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)
This satisfies the strict-repo-deps constraint that comes with 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. |
fixes #12