-
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
Various fixes and improvements #87
base: lucid-master
Are you sure you want to change the base?
Conversation
--incompatible_allow_tags_propagation defaults to true and has for a while
.bazelrc_shared
Outdated
@@ -15,6 +15,7 @@ build --verbose_failures | |||
build --worker_max_instances=4 | |||
build --worker_sandboxing | |||
|
|||
common --incompatible_modify_execution_info_additive |
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.
Nit: Could we put this with the other --incompatible_*
options?
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.
Yep. Will do.
rules/scalafmt/private/test.bzl
Outdated
@@ -32,7 +32,7 @@ scala_non_default_format_attributes = { | |||
def build_format(ctx): | |||
files = [] | |||
manifest_content = [] | |||
config = ctx.toolchains["//rules/scalafmt:toolchain_type"].scalafmt_config.config | |||
config = ctx.toolchains["@rules_scala_annex//rules/scalafmt:toolchain_type"].scalafmt_config.config |
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 this necessary? Won't this label be evaluated in the context of this workspace?
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 believe you are correct. Revisiting this I'm pretty sure there was a bug with toolchain handling in the internal repo and I worked around it by doing this. Now that I've fixed the bug this is no longer necessary. I'll remove this change and the other change.
@@ -15,7 +15,7 @@ _scalafmt_config = rule( | |||
attrs = { | |||
"config": attr.label( | |||
allow_single_file = [".conf"], | |||
default = "//:.scalafmt.conf", | |||
default = "@rules_scala_annex//:.scalafmt.conf", |
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.
Wouldn't the correct thing to do be Label("//:.scalafmt.conf")
? We can't guarantee the user of this module will name it rules_scala_annex
.
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.
Same here
# We set `original_scala_toolchain_setting` so we can reset the toolchain to its | ||
# original value in `scala_outgoing_transition`. That way, we can ensure every target is | ||
# built under a single toolchain, thus preventing duplicate builds. | ||
# | ||
# We do not do this work when the toolchain name is set, but is no different than what is |
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.
It may be worth adding a comment explaining that this is a temporary fix.
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.
Added a blurb.
Things I ran into while upgrading a large repo to Bazel 8 + bzlmod