-
Notifications
You must be signed in to change notification settings - Fork 13
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
Migrate to Bzlmod #57
Conversation
MODULE.bazel
Outdated
bazel_dep(name = "rules_pkg", version = "1.0.1") | ||
bazel_dep(name = "rules_scala_annex") | ||
|
||
rules_scala_annex_version = "bzlmod-migration" |
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.
- Wait until we've merged Migrate to Bzlmod rules_scala#74 and update this.
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.
Mostly LGTM. Left a few comments.
@@ -1,5 +1,6 @@ | |||
startup --expand_configs_in_place | |||
|
|||
common --@rules_scala_annex//rules/scala:scala-toolchain=zinc_3 |
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 something all users of Annex will need to do? If so, is it in the README for that ruleset?
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.
Nope, this is only necessary if you want to override the default Scala toolchain. It's explained here:
https://github.com/lucidsoftware/rules_scala/blob/74960c12dfc2487cd257c2a49d5d4e1c6cfb1d44/docs/newdocs/scala_versions.md
Let me know if I can make it clearer in any way.
MODULE.bazel
Outdated
module(name = "rules_play_routes") | ||
|
||
bazel_dep(name = "bazel_skylib", version = "1.7.1") | ||
bazel_dep(name = "buildifier_prebuilt", version = "7.3.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.
Can we mark all of the deps that we can as dev_dependency
? I think this is one.
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.
See my comment here:
lucidsoftware/rules_scala#74 (comment)
MODULE.bazel
Outdated
bazel_dep(name = "buildifier_prebuilt", version = "7.3.1") | ||
bazel_dep(name = "rules_java", version = "7.11.1") | ||
bazel_dep(name = "rules_jvm_external", version = "6.4") | ||
bazel_dep(name = "rules_pkg", version = "1.0.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.
I think this is another dev_dependency
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.
See my comment here:
lucidsoftware/rules_scala#74 (comment)
MODULE.bazel
Outdated
urls = ["https://github.com/lucidsoftware/rules_scala/archive/refs/heads/{}.zip".format(rules_scala_annex_version)], | ||
) | ||
|
||
bazel_dep(name = "stardoc", version = "0.7.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.
Is this dep intentionally below the others or can it be moved up with the others?
Also I think this can be marked as dev_dependency
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 wanted to keep the archive_override
call close to the bazel_dep
call for rules_scala_annex
. That way, it's not confusing where rules_scala_annex
is coming from or what version we're using.
Also, see my comment here:
lucidsoftware/rules_scala#74 (comment)
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 got it. Makes sense.
MODULE.bazel
Outdated
|
||
specs2_version = "4.20.8" | ||
|
||
zinc_version = "1.10.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.
Scope creep, but could we update this to 1.10.4 while we're at it? Annex is already on that version, so it shouldn't require anymore work besides just updating the number and generating the lock file.
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.
Sure thing. I refrained from updating any versions in the lucidsoftware/rules_scala_annex
PR because I considered it scope creep, but this is a simple change.
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.
Thanks
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.
No problem!
bdc6f9f
to
cc66237
Compare
04335c0
to
7edf46f
Compare
No description provided.