Single-jar pack_sources should conform to Bazel behavior #1378
Conversation
| @@ -309,16 +309,26 @@ def _create_scala_compilation_provider(ctx, ijar, source_jar, deps_providers): | |||
| ) | |||
|
|
|||
| def _pack_source_jar(ctx, scala_srcs, in_srcjars): | |||
There was a problem hiding this comment.
was confused a bit at the beginning as in_srcjars implies a bool, but looks like it's meant to be a list of src jars.
so nit possibly to rename it scala_src_jars. feel free to do so as a follow up.
There was a problem hiding this comment.
Agreed, though we should rename to input_srcjars imo (as followup).
There was a problem hiding this comment.
Too lazy for followup. Renamed it and also changed the .-src.jar naming into -src.jar.
|
|
||
| # In line with Bazel's java_common.pack_sources, | ||
| # We return the initial .srcjar file since there are no source files | ||
| # Not sure where the second -src.jar comes from, maybe due to java rules |
There was a problem hiding this comment.
Could you check if adding expect_java_output = False to :source_jar target makes this go away?
There was a problem hiding this comment.
It does! Thanks for the test case. I added one more to test multiple source jars, adding SourceJar2.srcjar to the test resources. Also refactored the test logic since it was insanely repetitive.
| return input_srcjars[0] | ||
| else: | ||
| output_jar = ctx.outputs.jar | ||
| without_ext = output_jar.basename[:-len(output_jar.extension)] |
There was a problem hiding this comment.
Not sure if this would make code look better and probably it's a matter of taste but without_ext could be calculated as output_jar.basename[:-len(output_jar.extension) - 1] if output_jar.extension else output_jar.basename
Please feel free to ignore this comment.
There was a problem hiding this comment.
Good suggestion! I ended up with (ironically) a longer implementation. Your suggestion made me realize that we shouldn't expect that output_jar has an extension. Personally, I think that the ternary if is too long for one line, so a mutating if is fine.
wiwa
force-pushed
the
f/pack-source-1-jar
branch
2 times, most recently
from
0cacb4f to
a319123
Compare
wiwa
force-pushed
the
f/pack-source-1-jar
branch
2 times, most recently
from
0cacb4f to
d0ebff4
Compare
Co-authored-by: Shane Delmore <[email protected]>
wiwa
force-pushed
the
f/pack-source-1-jar
branch
from
d0ebff4 to
151cf8b
Compare
Description
Fix
_pack_source_jarinphase_compile.bzlto return single input source jars if there are no source files and only one source jar.This conforms to the expected behavior of
java_common.pack_sources(at least as of Bazel 5.0.0).The previous PR #1314 did not take into account the behavior of
pack_sourceswith only one input jar.The field
output_source_jaris actually checked inpack_sources, which will onlyoutput the input jar directly if
output_source_jaris null.We also remove any additional
.inoutput_source_jars, i.e. if they look likemy-jars.-src.jar,our change will instead name them
my-jars-src.jar.Motivation
We have a macro which exposes jars with
.srcjarextensions. These are in fact the input jars to-src.jar(bundled) output jars. However, we expect that singular input jars will maintain the.srcjarextension and not be bundled.This is the previous behavior of
rules_scala, and we would like to maintain this for our codebase,since it is also the behavior of Bazel's
java_common.pack_sources.It should be noted that #1314 does not correctly remove the entire extension, which we fix here.
i.e.:
my-jars.extensionwill becomemy-jars.-src.jar.. Presumably, we should be naming itmy-jars-src.jar, without the extra.before the-src.jarsuffix. This is likely due to an off-by-one error in the code:source_jar_name = output_jar.basename[:-len(output_jar.extension)] + "-src.jar"Thus, we remove any trailing
.s before appending-src.jar.