-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement a toolchain #51
Conversation
a0b4dbd
to
3b0bde2
Compare
Rebased off of |
bc086ff
to
44fd03a
Compare
Rebased off of |
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.
Took a pass through and things mostly looked good to me. I had a few questions/comments.
@@ -8,9 +8,9 @@ load("@rules_scala_annex//rules:providers.bzl", _ScalaConfiguration = "ScalaConf | |||
# SemanticDB compiler plugin being enabled. | |||
# | |||
def phase_semanticdb(ctx, g): | |||
scala_configuration = ctx.attr.scala[_ScalaConfiguration] | |||
toolchain = ctx.toolchains["//rules/scala:toolchain_type"] |
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.
I imagine this string will never change, but it may be worth doing what the rules_go
folks did and instead use a constant that we import. That way if this string ever needs to change, it will be easy to change.
Also, they use the fully qualified name, which is probably not a bad idea either. So @rules_scala_annex//rules/scala:toolchain_type
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 sounds like a great idea, but if we ever need to move that constant, we'll need to update all the imports that reference it, which introduces the same problem.
Also, Is the fully-qualified name unnecessary? I prefer not to use those unless it's necessary. My understanding was that Bazel understood this was in rules_scala_annex
, even if these rules are being used in another 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.
Oh, you meant defining that alias for internal use in this file. That makes a lot of sense; I'll do that.
rules/register_toolchain.bzl
Outdated
if attr.scala_toolchain_name == "": | ||
return {} | ||
|
||
return { |
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.
Do we want to be storing the original scala_toolchain_name like the rules_go
folks did?
Because currently this will always reset to the original value, and I thought that you ran into a scenario where you could override things from the command line, which made that problematic.
I did this in the Play Routes and Twirl rules, in case it is at all helpful
new_settings = {}
new_settings[toolchain_setting_key] = attr.play_routes_toolchain_name
# Store the original toolchain value, so we can reset it
new_settings[original_toolchain_setting_key] = json.encode(settings[toolchain_setting_key])
return new_settings
Then I reset it as follows:
if settings[original_toolchain_setting_key] != "":
new_settings = {}
new_settings[toolchain_setting_key] = json.decode(settings[original_toolchain_setting_key])
new_settings[original_toolchain_setting_key] = ""
return new_settings
else:
return {}
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.
Ah, I completely forgot about that. I'll go ahead and do that.
tests/WORKSPACE
Outdated
scala_register_toolchains( | ||
default_scala_toolchain_name = "test_zinc_2_13", | ||
toolchains = [ | ||
"@@//dependencies/indirect:test_zinc_2_13_direct_deps_off", |
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.
Do we need the @@
for these or can we just use the single @
?
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.
You're right—it can. Do you know the difference between @//...
and @@//...
? I know the difference between @/foo//...
and @@foo//...
, but are @//...
and @@//...
the same?
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.
@@ is canonical name. Was introduced with bzlmod. AFAIK it's intended to be machine readable rather than human readable. https://bazel.build/rules/lib/builtins/Args.html
Label objects are turned into a string representation that resolves back to the same object when resolved in the context of the main repository. If possible, the string representation uses the apparent name of a repository in favor of the repository's canonical name, which makes this representation suited for use in BUILD files. While the exact form of the representation is not guaranteed, typical examples are //foo:bar, @repo//foo:bar and @@canonical_name+//foo:bar.bzl.
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.
Also https://bazel.build/concepts/labels
This link explains it better.
One thing that's been bouncing around in my head and I don't know if I care about or not: we use I wonder if some folks will think that means they're getting that particular label. When in reality they should get a toolchain with that name, which works with the right platform? I don't think |
45c6429
to
7b7115a
Compare
Squashed. |
7b7115a
to
fa16134
Compare
Rebased off of |
Yeah, I remember deliberating about the naming of |
It doesn't seem to test anything meaningful and hasn't been running because the entire test is commented out.
8b99998
to
2739c71
Compare
Removed an outdated comment about generating documentation, since `scripts/gen-docs.sh` no longer exists. I also removed docs/configure_zinc_scala.md`, since the rest of our autogenerated documentation has been deleted.
These tests were looking in the wrong output directory, which has changed now that we're using a configuration setting to determine the Scala compiler.
1fd0fb1
to
80af0ac
Compare
Squashed. |
This PR leverages the Bazel toolchain framework to replace
configure_bootstrap_scala
,configure_zinc_scala
, and thescala
attribute. Those rules were written before the framework existed, so it makes sense to replace it with a more modern, idiomatic alternative.Note that this PR breaks existing repositories that use
lucidsoftware/rules_scala
, so it's necessary to migrate your repository before pulling in these changes. Here are the steps for doing so:default_scala
mapping in yourWORKSPACE
fileconfigure_bootstrap_scala
,configure_zinc_scala
with theirregister_*_toolchain
equivalentsscala_register_toolchains
to provide a list of toolchains and a default toolchain