scala_export macro #1790
scala_export macro #1790
Conversation
vinnybod
requested review from
WojciechMazur,
mbland and
simuons
as code owners
mbland
left a comment
mbland
left a comment
There was a problem hiding this comment.
Overall, I think it looks mostly pretty good, modulo the few requests I've made so far, particularly around legacy WORKSPACE support. I also need to study it a bit to make sure I understand the whole thing, and I'll eventually pull the branch and run the tests locally.
That said, I'm going to play the BazelCon card and decline to commit to looking any more closely until about a week from now. (If changes come in, and I'm inclined, I may respond sooner. But no promises until next week.)
mbland
left a comment
mbland
left a comment
There was a problem hiding this comment.
One more thing: there are a number of test failures in CI.
If you haven't already, please run the test scripts locally and fix what you can, ideally getting ./test_all.sh to finish successfully once you're done. I'm happy to dig in and try to help if you get stuck. (After BazelCon.)
And FWIW, I think the Windows failure may be CRLF vs. LF related, but I haven't looked very closely yet. But check out the changes to src/java/io/bazel/rulesscala/worker/WorkerTest.java from #1724, in case it helps.
|
Looks like something in the docs jar is not reproducible. Will need to spend some more time looking at that. |
| @@ -0,0 +1,79 @@ | |||
| load("@aspect_bazel_lib//lib:diff_test.bzl", "diff_test") | |||
There was a problem hiding this comment.
Using bazel_lib instead of skilib because the skylib diff_test fails on windows with this error bazelbuild/bazel-skylib#529
WojciechMazur
left a comment
WojciechMazur
left a comment
There was a problem hiding this comment.
That's a really nice feature, thank you for spending time. I've left some notes, would try to come back to this PR tomorrow and try it out locally
| scala_library( | ||
| name = lib_name, | ||
| tags = tags + maven_coordinates_tags, | ||
| testonly = testonly, | ||
| **kwargs | ||
| ) |
There was a problem hiding this comment.
Shouldn't we set explicit visibility (from input) for all these targets? Seems like it's only set for maven_export
| updated_deploy_env.append(lib) | ||
|
|
||
| scala_library( | ||
| name = lib_name, |
There was a problem hiding this comment.
Note:
I wondered wouldn't it be better if scala_library would be use the input name instead of lib_name, however it seems like both Java and Kotlin variants are also following the same pattern: name for maven_export and lib_name for library, so it might be best to keep it as it is, and follow the convention
As the result depending on :name defined as maven_export which then aliases maven_project_jar we'd get only JavaInfo, but not ScalaInfo in resulting providers.
It might be fine now (it only contains information about defining macros), but we would need to remember about this if we'd like to store additional informations, at that point we should have a dedicated provider to delegate underlying JavaInfo/ScalaInfo/MavenInfo in a default :name target
| updated_deploy_env = [] + deploy_env | ||
| for lib in SCALA_LIBS: | ||
| if lib not in deploy_env: | ||
| updated_deploy_env.append(lib) |
There was a problem hiding this comment.
SCALA_LIB is empty, so that's dead code
| **kwargs | ||
| ) | ||
|
|
||
| if "no-scaladocs" not in tags: |
There was a problem hiding this comment.
The underlying maven_export also checks for no-javadocs tags, I wonder should we should implicitlly add that tag if no-scaladocs was used for consistency?
| inputs = [":" + scaladocs_name] + doc_resources, | ||
| out = name + "-scaladocs.jar", | ||
| ) | ||
| classifier_artifacts["scaladoc"] = scaladocs_jar_name |
There was a problem hiding this comment.
Shouldn't that be a javadoc? scaladoc is definitely a thing, but javadoc is standard classifier for Maven artifacts containing documentation.
As an example all artifacts published to Maven Sonatype need to have a required javadoc artifact, nevertheless if that's a Scala or Java library.
It also seems like maven_export would try to build javadoc, I guess it might fail for Scal sources, so maybe we should prevent it from doing so.
https://github.com/bazel-contrib/rules_jvm_external/blob/a23c85cf0af3a2b10d3d84da7a0160261b38e2bb/private/rules/java_export.bzl#L307-L328
I expect that as result we should have the same outputs, as can be found by sbt (or other Scala dedicated build tools) releases: –javadoc, -sources, actual jar and soruces
| @@ -0,0 +1,132 @@ | |||
| load("@bazel_skylib//rules:run_binary.bzl", "run_binary") | |||
| # Exclude *-docs.jar (javadoc jars from maven_export in rules_jvm_external) | ||
| # because javadoc generates non-deterministic output. |
There was a problem hiding this comment.
Do you maybe remember what was the issue here? Maybe we should fix it upstream in the scaladoc tool or add some options to make the outputs more deterministic
| ) | ||
|
|
||
| bazel_dep(name = "bazel_skylib", version = "1.8.2") | ||
| bazel_dep(name = "aspect_bazel_lib", version = "2.22.0") |
There was a problem hiding this comment.
This one is used only in tests, so can be marked as dev dependency
3 participants
Description
This adds a drop in replacement for
java_exportthat works for scala.We've been using this internally at Confluent for some time now in our rules_jvm_external fork.
https://github.com/confluentinc/rules_jvm_external/blob/master/private/rules/scala_export.bzl
Motivation
We needed this at Confluent and figure other folks might find it useful. Originally I tried to upstream this to rules_jvm_external, but it was decided that it would be more appropriate here and would help to avoid circular dependencies between rules_scala and rules_jvm_external if rules_scala ever chose to adopt rules_jvm_external in the future.