Add support for scala_import #101
Conversation
Switch from using java_library to scala_import for Scala dependencies. Notes: 1. java dependencies still use java_library; ideally we would switch to java_import for consistency but java rules are still in flux so we're sticking to what works 2. the label passed to `jars` attribute might actually not point directly at the :file target (which is a filegroup) but at the synthetic java_import so we're technically not following the contract that says `jars` should have labels pointing at jar files. However, with the current implementation of scala_import that's allowed and it actually helps IntelliJ to resolve the source artifact for a jar. I think this behavior is actually a bug in the scala_import implementation.
The import into IntelliJ works!
3rdparty/jvm/com/chuusai/BUILD
Outdated
| @@ -1,7 +1,7 @@ | |||
| load("@io_bazel_rules_scala//scala:scala.bzl", "scala_library") | |||
There was a problem hiding this comment.
can we change this load line since we aren't using scala_library anymore? This I think is done in the config yaml.
| @@ -186,7 +186,23 @@ object Writer { | |||
| val langFn = language(g, model) | |||
| def replacedTarget(u: UnversionedCoordinate): Option[Target] = | |||
| Label.replaced(u, model.getReplacements).map { case (lab, lang) => | |||
| Target(lang, Label.localTarget(pathInRoot, u, lang), exports = Set(lab)) | |||
| // TODO: converge on using java_import instead of java_library once | |||
There was a problem hiding this comment.
can you add issues for these instead? In my experience TODOs are graveyards.
| case Transitivity.RuntimeDeps => (Set.empty[Label], depLabels) | ||
| } | ||
|
|
||
| // TODO: converge on using java_import instead of java_library once |
There was a problem hiding this comment.
same comment, can we remove and add an issue.
There was a problem hiding this comment.
Sure, created the issue and i'm keeping the comment simply as pointer in case somebody is looking at this code.
tools/build_rules/prelude_bazel
Outdated
| @@ -1 +1,2 @@ | |||
| load("@io_bazel_rules_scala//scala:scala.bzl", "scala_library", "scala_binary", "scala_test", "scala_repl") | |||
| load("@io_bazel_rules_scala//scala:scala_import.bzl", "scala_import") | |||
There was a problem hiding this comment.
I'm getting a bit -1 on prelude_bazel (until 0.9 and I haven't even checked there, it doesn't work great across remote repos). Can we not add this and use the header feature to add scala_import to the 3rdparty targets since it is only used there?
Add `load` for scala_import to buildHeader element and get rid of that load from prelude. The scala_import is only used in 3rdparty so mentioning it in dependencies.yaml file is enough. We want to move away from using prelude_bazel.
Address review feedback
gkk-stripe
force-pushed
the
gkk-scala-import
branch
from
7a9f8ef to
5ec5634
Compare
|
@johnynek review comments addressed |
|
Looks reasonable to me. |
|
🎆 Thanks @gkk-stripe |
|
@gkk-stripe thanks a lot for this PR and @johnynek for merging this. helped us a lot. 👍 |
| kind = Target.Import, | ||
| name = Label.localTarget(pathInRoot, u, lang), | ||
| exports = Set.empty, | ||
| jars = Set(lab)) |
There was a problem hiding this comment.
here is the bug. I don't think we should be changing replacements actually.
@bluedragonx do you want to try reverting just this block here and see if that fixes for you?
Would be nice to add a test that replacements don't get handled with scala_import (and instead just use scala_library export).
There was a problem hiding this comment.
Ah yes. I didn't understand what the replacements are and went too fast over them. Now I see they're described in the README and indeed they should be handled by scala_library.
See the individual commits for details.
Tested on a few internal repos at Stripe. Works great: both bazel build and intellij support.