Reintegrate the compiler CLI into this repo, implement toolchains, and update all the things #55

jjudd · 2024-09-24T17:06:20Z

This brings the Compiler CLI from https://github.com/lucidsoftware/play_routes_compiler_cli back into this repo. Having it split across repos was causing a lot of code duplication. We still will publish the CLI to maven, but it's all in one repo to avoid people copy pasting tests, etc. across the repos.

It also implements toolchains to replace our use of bind for choosing which Play routes compiler CLI to use.

It also updates nearly all the dependencies.

With how many changes there are in this PR it may just be easier to review the codebase as whole than the diff.

Heads up that there's a circular dependency between the cli repo and this repo, so things will be broken until the new cli gets published and then another commit to this repo is made.

jadenPete

I ended up just reviewing the entire repository, as you suggested.

jadenPete · 2024-09-25T16:40:04Z

README.md


 ```python
 # update version as needed
-rules_play_routes_version = "bfaca5f186f2c3b989c80fd00f37a53b84406b3d"
+rules_play_routes_version = "TODO"


Rather than putting TODO here or updating this commit manually, should we instruct users to insert the latest commit ID? This applies to the other repositories pulled in here.

Oh, sorry, I should have left a comment.

I intend to update this as soon as I have a commit with everything working. I'm not too concerned about doing much more than that as we'll be moving to bzlmod very soon and Bazel 8 will disable workspace support by default.

jadenPete · 2024-09-25T16:43:42Z

README.md


-This installs `rules_play_routes` to your `WORKSPACE` and binds the default play routes compiler cli the rules will use. Update the commit as needed.
+play_routes_compiler_cli_3_repositories()


Don't this and play_routes_compiler_cli_2_13_repositories need to be imported?

Yep. Good catch.

jadenPete · 2024-09-25T17:05:15Z

workspace.bzl

+    maven_install(
+        name = "play_routes_compiler_cli_3",
+        artifacts = [
+            "com.github.scopt:scopt_3:4.1.0",


Nit: Might we also want to deduplicate the version of scopt?

jadenPete · 2024-09-25T17:09:43Z

test_workspace.bzl

+play_version = "3.0.4"
+specs2_version = "4.20.8"
+
+def play_routes_test_3_repositories():


Nit: There's a lot of overlap between this and play_routes_test_2_13_repositories. Could we merge them into a single macro?

Yep. I'll change it to have a single underlying private function with two top level public functions to keep calling it simple.

jadenPete · 2024-09-25T17:10:49Z

BUILD.bazel

+    configuration = DEFAULT_TOOLCHAIN_CONFIGURATION,
+    java_runtime = "@rules_java//toolchains:remotejdk_21",
+    javac_supports_worker_multiplex_sandboxing = True,
+    # some of the default options make scala compilation fail in the test package


Could you elaborate on what these options are?

I believe it's the error prone compiler options. The -Xep ones

I'm honestly not too familiar with this as I believe I found this like this and copied it over from the other repo https://github.com/google/error-prone

Ah, gotcha. I was just curious, since I wasn't familiar with these.

jadenPete · 2024-09-25T17:53:59Z

play-routes-toolchain/transitions.bzl

+    last play_routes_toolchain_transition.
+    """
+
+    if settings[original_toolchain_setting_key] != "":


Wouldn't it be much simpler to just reset the setting to its default value? In what situation would resetting it to its previous value not be equivalent?

Is it possible to end up in a state in which it wouldn't be equivalent? If not, then we can use the default value.

I was defensively programming here. Are there downsides to doing it this way? If not, then I say we keep it this way to be safe. If so, then I say we go default to avoid the downsides.

Actually, now that I think about it, you could supply a command-line argument to override the default value, in which case they wouldn't be equivalent. This makes the most sense to me.

jadenPete · 2024-09-25T17:54:19Z

play-routes-toolchain/transitions.bzl

+    """
+    if attr.play_routes_toolchain_name == "":
+        return {}
+    elif settings[toolchain_setting_key] == attr.play_routes_toolchain_name:


Nit: This can be an if instead of an elif.

I'll just combine the if and elif into a single if

Even better!

jadenPete · 2024-09-25T17:56:07Z

play-routes-toolchain/transitions.bzl

+        # we store the original value in
+        return {}
+
+    new_settings = {}


Nit:

return { toolchain_setting_key: attr.play_routes_toolchain_name original_toolchain_setting_key: json.encode(settings[toolchain_setting_key]) }

FWIW I like the way it is even if it is more verbose as it makes it clearer to those unfamiliar with toolchains that these are new settings because of the variable name.

jadenPete · 2024-09-25T17:57:34Z

play-routes/play-routes.bzl

    attrs = {
        "srcs": attr.label_list(
            doc = "Play routes files",
            allow_files = True,
            mandatory = True,
+            cfg = reset_play_routes_toolchain_transition,


We may want to add a comment explaining that this outgoing transition should be added to every attr.label or attr.label_list attribute, lest the configuration change and things be re-built unnecessarily.

jadenPete · 2024-09-25T18:00:24Z

test/play-routes-compiler-test.bzl

+        play_routes_toolchain_name = play_routes_toolchain_name,
+    )
+
+    # TOOD: Figure out what this does and add the test for it


Nit: TODO

This has been here since the dawn of time for this repo. I don't think anyone's ever figured this one out. I'ma leave it as it's just copy pasted from the CLI repo and was written forever ago.

Ah, okie dokie. There's no problem preserving relics of the past.

…lchains

jjudd added 2 commits September 9, 2024 12:16

Update all the things, update to annex latest, and add worker support

22a30c6

Heads up that there's a circular dependency between the cli repo and this repo, so things will be broken until the new cli gets published and then another commit to this repo is made.

Use toolchains instead of bind

f83f9dc

jjudd requested review from jadenPete and SrodriguezO September 24, 2024 17:06

jadenPete approved these changes Sep 25, 2024

View reviewed changes

jjudd force-pushed the compiler-cli-and-toolchains branch 3 times, most recently from 55ce187 to 21adb38 Compare October 12, 2024 22:57

jjudd added 3 commits October 12, 2024 17:21

Move the Play Routes Compiler CLI back into this repo and improve too…

658961a

…lchains

Drop Bazel 6 support and update Bazel 7 version

43e2a76

Support path mapping and better support cancellation

65f5417

jjudd force-pushed the compiler-cli-and-toolchains branch from 21adb38 to 65f5417 Compare October 12, 2024 23:21

SrodriguezO mentioned this pull request Oct 15, 2024

Update all the things, move the CLI back into this repo, use toolchains instead of bind, and support new Bazel worker options lucidsoftware/rules_twirl#45

Merged

jjudd merged commit a3048c2 into master Oct 15, 2024
2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reintegrate the compiler CLI into this repo, implement toolchains, and update all the things #55

Reintegrate the compiler CLI into this repo, implement toolchains, and update all the things #55

jjudd commented Sep 24, 2024

jadenPete left a comment

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 26, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 26, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 26, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 25, 2024

jjudd Sep 25, 2024

jadenPete Sep 26, 2024


		This installs `rules_play_routes` to your `WORKSPACE` and binds the default play routes compiler cli the rules will use. Update the commit as needed.
		play_routes_compiler_cli_3_repositories()

Reintegrate the compiler CLI into this repo, implement toolchains, and update all the things #55

Reintegrate the compiler CLI into this repo, implement toolchains, and update all the things #55

Conversation

jjudd commented Sep 24, 2024

jadenPete left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment