Allow user to provide scalac source jar for "ast-plus" dependency tracking feature

[thirtyseven]

Description

Allow users to pass a label for the scala-compiler srcjar for use in the new ast-plus dep tracking feature, e.g. if they have configured rules_jvm_external to fetch it.

Motivation

Enable using the ast-plus feature while allowing for reproducible builds and user-controlled dependencies. An attempt at fixing #1492.

[thirtyseven]

This doesn't seem like a very satisfying solution, to use it with rules_jvm_external, you need to pass the very unwieldy

--repo_env=SCALA_COMPILER_SRCJAR=@org_scala_lang_scala_compiler_jar_sources_2_11_12//file:org/scala-lang/scala-compiler/2.11.12/scala-compiler-2.11.12-sources.jar

on the CLI or to scala_config. Better than nothing, though. Also, I just realized that ast-plus tracking hasn't even been included in a tagged release yet, so maybe I'm jumping the gun with this PR and you have something else in mind.

[liucijus]

Thanks, @thirtyseven, for looking into this.

I think passing sources label via rules_scala_setup to dt_patched_compiler_setup can be easier than adding it to scala_config, which by itself is a hack and shouldn't be used when there are alternative ways to solve the problem. WDYT?

[thirtyseven]

That would work, as long as it’s possible to pass in a dict of Scala version to srcjar labels. Our repo and probably others depend on being able to change the Scala version per-build with --repo_env.

[thirtyseven]

I've updated this with an API that I think is reasonable, it allows the user to pass in either a URL and a hash, or a label, for multiple Scala versions. See here for an example.

[liucijus]

That would work, as long as it’s possible to pass in a dict of Scala version to srcjar labels. Our repo and probably others depend on being able to change the Scala version per-build with --repo_env.

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

[thirtyseven]

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

I think the current iteration of the API in this PR solves this while also adding the option of passing a custom URL/SHA, PTAL. Passing a custom URL is probably more useful than a label since only direct labels for files work in repository rules and it can be hard to find a label that works for this. My approach uses rules_scala_setup not scala_config, I'm not sure if I can move it back to scala_config since it involves some nested structures.

[liucijus]

That would work, as long as it’s possible to pass in a dict of Scala version to srcjar labels. Our repo and probably others depend on being able to change the Scala version per-build with --repo_env.

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

I think the current iteration of the API in this PR solves this while also adding the option of passing a custom URL/SHA, PTAL. Passing a custom URL is probably more useful than a label since only direct labels for files work in repository rules and it can be hard to find a label that works for this. My approach uses rules_scala_setup not scala_config, I'm not sure if I can move it back to scala_config since it involves some nested structures.

I was not clear with what I meant with scala_config - I wanted to say that a user can look at SCALA_VERSION and decide which data needs to be passed to rules_scala_setup. I want this macro to contain bare minimum to create scala_compiler-source external without Scala version selection.

I'm OK with the changes in this PR, just looking for opportunities to simplify it as much as possible. Also, in the future, we will have to find a proper way to inject this dependency on the source via proper indirection (toolchain?).

@thirtyseven would you be ok to change rules_scala_setup to accept information just for a single version. Eg.:

rules_scala_setup(scala_compiler_srcjar =  {
        "url": "https://repo1.maven.org/maven2/org/scala-lang/scala-compiler/2.12.13/scala-compiler-2.12.13-sources.jar",
        "label": "foo",
        #  "integrity": "sha384-yKJTudaHM2dA+VM//elLxhEfOmyCYRHzbLlQcf5jlrR+G5FEW+fBW/b794mQLMOX",
        "sha256": "65f783f1fbef7de661224f607ac07ca03c5d19acfdb7f2234ff8def1e79b5cd8",
    })

[thirtyseven]

That would work, as long as it’s possible to pass in a dict of Scala version to srcjar labels. Our repo and probably others depend on being able to change the Scala version per-build with --repo_env.

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

@thirtyseven, I totally missed the fact that you want to do multiversion builds. I think it's a hard problem to give a good API for every external loader (not everyone is using rules_jvm_external). So, I'd like to leave version selection to the user, which they can do using scala_config. WDYT?

I think the current iteration of the API in this PR solves this while also adding the option of passing a custom URL/SHA, PTAL. Passing a custom URL is probably more useful than a label since only direct labels for files work in repository rules and it can be hard to find a label that works for this. My approach uses rules_scala_setup not scala_config, I'm not sure if I can move it back to scala_config since it involves some nested structures.

I was not clear with what I meant with scala_config - I wanted to say that a user can look at SCALA_VERSION and decide which data needs to be passed to rules_scala_setup. I want this macro to contain bare minimum to create scala_compiler-source external without Scala version selection.

I'm OK with the changes in this PR, just looking for opportunities to simplify it as much as possible. Also, in the future, we will have to find a proper way to inject this dependency on the source via proper indirection (toolchain?).

@thirtyseven would you be ok to change rules_scala_setup to accept information just for a single version. Eg.:

rules_scala_setup(scala_compiler_srcjar =  {
        "url": "https://repo1.maven.org/maven2/org/scala-lang/scala-compiler/2.12.13/scala-compiler-2.12.13-sources.jar",
        "label": "foo",
        #  "integrity": "sha384-yKJTudaHM2dA+VM//elLxhEfOmyCYRHzbLlQcf5jlrR+G5FEW+fBW/b794mQLMOX",
        "sha256": "65f783f1fbef7de661224f607ac07ca03c5d19acfdb7f2234ff8def1e79b5cd8",
    })

Thanks for clarifying, that makes a million times more sense than what I had. I've updated the PR with that API. The test cases aren't quite ready yet, they don't test the error handling or verify that the user jar is actually used, but I can update them once you've signed off on the current approach.

[liucijus]

The test cases aren't quite ready yet, they don't test the error handling or verify that the user jar is actually used, but I can update them once you've signed off on the current approach.

Looks good!

[thirtyseven]

The test cases aren't quite ready yet, they don't test the error handling or verify that the user jar is actually used, but I can update them once you've signed off on the current approach.

Looks good!

Thanks, I've updated the test cases.