Support not bundling SemanticDB files in the output JAR #50
Conversation
jadenPete
force-pushed
the
support-generating-semanticdb-files-separately
branch
from
d5315ac
to
a698dd2
Compare
|
Formatted code. |
| @@ -30,24 +35,22 @@ phase_coverage_jacoco = _phase_coverage_jacoco | |||
|
|
|||
| phase_ijinfo = _phase_ijinfo | |||
|
|
|||
| phase_noop = _phase_noop | |||
| phase_javainfo = _phase_javainfo | |||
There was a problem hiding this comment.
Looks like these were sorted alphabetically. Is that what is going on here?
| return struct(outputs = [], scalacopts = []) | ||
|
|
||
| outputs = [] | ||
| semanticdb_directory = paths.join("_semanticdb/", ctx.label.name) |
There was a problem hiding this comment.
Is it possible for the name to contain characters that are illegal to use as file names?
There was a problem hiding this comment.
From Bazel's documentation on labels:
Allowed characters in package names are the lowercase letters
athroughz, the uppercase lettersAthroughZ, the digits0through9, the characters! \"#$%&'()*+,-.;<=>?@[]^_`{|}(yes, there's a space character in there!), and of course forward slash/(since it's the directory separator).
I assume legal target names can't include slashes because that'd make the package and name indistinguishable.
I think the only illegal characters in Unix filenames are / and the null character, both of which Bazel doesn't allow, so no, it shouldn't be possible. Windows is a different story, but I think it's highly unlikely that special characters other than - and _ will be used anyways, and even if they are, an error should be reported.
| @@ -55,7 +55,7 @@ def phase_bootstrap_compile(ctx, g): | |||
| jar_creator = ctx.executable._jar_creator.path, | |||
| main_class = main_class, | |||
| output_jar = g.classpaths.jar.path, | |||
| scalacopts = " ".join(ctx.attr.scalacopts), | |||
| scalacopts = " ".join(ctx.attr.scalacopts + g.semanticdb.scalacopts), | |||
There was a problem hiding this comment.
Do we need semanticdb for the bootstrap compiler? It's only used for a small number of internal targets.
There was a problem hiding this comment.
We don't. I didn't understand the full purpose of the bootstrap compiler when I wrote this. I'll fix it.
|
|
||
| outputs.append(ctx.actions.declare_file(output_filename)) | ||
|
|
||
| if scala_configuration.version.startswith("2"): |
There was a problem hiding this comment.
I'm not sure how much we care about this, but how does this work for things like Scalajs or Scala Native?
There was a problem hiding this comment.
Hmm, it depends on what version is set to. The only other place in which we use it is phase_bootstrap_compile, where we also check if we're dealing with Scala 2 or 3.
Now that I think about it, is SemanticDB even a concept in Scala.js and Scala Native? I think both Scala.js and Scala Native use the same compiler, so it should be. For now, I'll leave this as is and dig into it further if folks want Scala.js and/or Scala Native support (assuming that's ok with you).
| @@ -37,6 +37,7 @@ def phase_zinc_compile(ctx, g): | |||
| ] | |||
|
|
|||
| zincs = [dep[_ZincInfo] for dep in ctx.attr.deps if _ZincInfo in dep] | |||
| scalacopts = ctx.attr.scalacopts + g.semanticdb.scalacopts | |||
There was a problem hiding this comment.
Nit: should we name this something other than scalacopts, so it's obvious that it's the combination of the regular scalacopts + the semanticdb opts? Perhaps combined_scalacopts?
| ), | ||
| "semanticdb_bundle": attr.bool( | ||
| default = True, | ||
| doc = "Whether to bundle SemanticDB files in the resulting JAR. Note that in Scala 2, this requires the SemanticDB compiler plugin.", |
There was a problem hiding this comment.
Should we provide some default for Scala 2? Is that even possible?
There was a problem hiding this comment.
Would we even want to do that if we could?
There was a problem hiding this comment.
What do you mean by this? The default is set to true, which matches the default for the compiler plugin (bundling SemanticDB files in the JAR).
| bazel build :semanticdb-2_13 | ||
| check_for_semanticdb_files 2_13 | ||
| bazel build :semanticdb-3 | ||
| check_for_semanticdb_files '3' |
There was a problem hiding this comment.
Nit: This has single quotes around it and the 2_13 version does not.
There was a problem hiding this comment.
Oh, I did that for syntax highlighting. It has no effect on the script. Bash differentiates numbers and strings, so it was being highlighted differently from 2_13.
This is an attempt to recreate these changes in this repository. Unlike
bazelbuild/rules_scala's SemanticDB support, these changes don't check that the SemanticDB compiler plugin is loaded if we're working with Scala 2. Instead, it's assumed that if the user sets thesemanticdb_bundleScalaConfigurationparameter toFalse, they'll add the plugin. If they don't, the build will fail.