Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Passing source-jar into JavaInfo #783

Merged
merged 6 commits into from
Jul 13, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 12 additions & 9 deletions scala/private/rule_impls.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ def _compile_or_empty(
if len(ctx.files.srcs) + len(srcjars.to_list()) == 0:
_build_nosrc_jar(ctx)

scala_compilation_provider = _create_scala_compilation_provider(ctx, ctx.outputs.jar, deps_providers)
scala_compilation_provider = _create_scala_compilation_provider(ctx, ctx.outputs.jar, None, deps_providers)

# no need to build ijar when empty
return struct(
Expand Down Expand Up @@ -476,7 +476,8 @@ def _compile_or_empty(
# so set ijar == jar
ijar = ctx.outputs.jar

scala_compilation_provider = _create_scala_compilation_provider(ctx, ijar, deps_providers)
source_jar = _pack_source_jar(ctx)
scala_compilation_provider = _create_scala_compilation_provider(ctx, ijar, source_jar, deps_providers)

# compile the java now
java_jar = try_to_compile_java_jar(
Expand Down Expand Up @@ -513,7 +514,7 @@ def _compile_or_empty(
merged_provider = merged_provider
)

def _create_scala_compilation_provider(ctx, ijar, deps_providers):
def _create_scala_compilation_provider(ctx, ijar, source_jar, deps_providers):
exports = []
if hasattr(ctx.attr, "exports"):
exports = [dep[JavaInfo] for dep in ctx.attr.exports]
Expand All @@ -523,6 +524,7 @@ def _create_scala_compilation_provider(ctx, ijar, deps_providers):
return JavaInfo(
output_jar = ctx.outputs.jar,
compile_jar = ijar,
source_jar = source_jar,
deps = deps_providers,
exports = exports,
runtime_deps = runtime_deps,
Expand Down Expand Up @@ -962,9 +964,7 @@ def _scala_binary_common(
rjars, #calling rules need this for the classpath in the launcher
)

def _pack_source_jars(ctx):
source_jars = []

def _pack_source_jar(ctx):
# collect .scala sources and pack a source jar for Scala
scala_sources = [
f
Expand All @@ -986,10 +986,13 @@ def _pack_source_jars(ctx):
java_toolchain = find_java_toolchain(ctx, ctx.attr._java_toolchain),
host_javabase = find_java_runtime_toolchain(ctx, ctx.attr._host_javabase),
)
if scala_source_jar:
source_jars.append(scala_source_jar)

return source_jars
return scala_source_jar

def _pack_source_jars(ctx):
source_jar = _pack_source_jar(ctx)
#_pack_source_jar may return None if java_common.pack_sources returned None (and it can)
return [source_jar] if source_jar else []
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't source_jar always defined now? I'm confused how this can ever return []. Can we add a comment if I am missing something?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: if I am right, can we remove _pack_source_jars and and just use _pack_source_jar?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the java_common.pack_sources may return None. @iirina maybe you know in which cases?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnynek just add a comment saying that java_common.pack_sources may return None? Honestly I want to do some refactoring here as I'd like us to drop the scala attribute or at least significantly trim down what we're passing there to rely on JavaInfo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, adding that comment would be fine to me, but I wonder if we really need two methods, maybe just put this logic at the end of the first call?

I would also love to remove the scala attribute and just use JavaInfo. If it doesn't break intellij, I think we should be able to do that. We should have an issue (maybe already do) to track that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just now saw your comment about removing _pack_source_jars completely. I preferred to focus on this change since (like I wrote above) I want to consider greatly limiting the scala attribute and so probably also will be able to remove _pack_source_jars.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no problem.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

java_common.pack_sources returns None if there's nothing to pack, meaning if both source_jars and sources are empty.


def scala_binary_impl(ctx):
scalac_provider = _scalac_provider(ctx)
Expand Down
9 changes: 4 additions & 5 deletions scala/scala_import.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -25,11 +25,10 @@ def _scala_import_impl(ctx):
jars2labels,
) #last to override the label of the export compile jars to the current target

current_target_providers = [_new_java_info(ctx, jar) for jar in current_target_compile_jars]

# Handle the case with no jars.
# TODO(#8867): Migrate away from the placeholder jar hack when #8867 is fixed.
if not current_target_providers:
if current_target_compile_jars:
current_target_providers = [_new_java_info(ctx, jar) for jar in current_target_compile_jars]
else:
# TODO(#8867): Migrate away from the placeholder jar hack when #8867 is fixed.
current_target_providers = [_new_java_info(ctx, ctx.file._placeholder_jar)]

return struct(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,6 @@ scala_library(

custom_jvm(
name = "direct_java_provider_dependency",
main = "//test/src/main/resources/scalarules/test:hellos-and-byes.jar", #just any jar would do
deps = ["transitive_dependency"],
)

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#This rule is an example for a jvm rule that doesn't support Jars2Labels
def _custom_jvm_impl(ctx):
jar = ctx.file.main
# TODO(#8867): Migrate away from the placeholder jar hack when #8867 is fixed.
jar = ctx.file._placeholder_jar
provider = JavaInfo(output_jar = jar,
compile_jar = jar,
deps = [target[JavaInfo] for target in ctx.attr.deps]
Expand All @@ -10,7 +11,10 @@ def _custom_jvm_impl(ctx):
custom_jvm = rule(
implementation = _custom_jvm_impl,
attrs = {
"main": attr.label(allow_single_file = True), #just used so we'll be able to build the JavaInfo with a "main" jar
"deps": attr.label_list(),
"_placeholder_jar": attr.label(
allow_single_file = True,
default = Label("@io_bazel_rules_scala//scala:libPlaceHolderClassToCreateEmptyJarForScalaImport.jar"),
),
},
)
3 changes: 2 additions & 1 deletion twitter_scrooge/twitter_scrooge.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,8 @@ def _scrooge_scala_library_impl(ctx):
)
if ctx.attr.exports:
exports = [exp[JavaInfo] for exp in ctx.attr.exports]
all_java = java_common.merge(_concat_lists(exports, [aspect_info.java_info]))
exports.append(aspect_info.java_info)
all_java = java_common.merge(exports)
else:
all_java = aspect_info.java_info

Expand Down