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.

Sign up for GitHub

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

Migrate from java_common.create_provider to JavaInfo() #781

Merged
merged 17 commits into from
Jul 12, 2019
Merged

Migrate from java_common.create_provider to JavaInfo() #781

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
597e84d
Migrate from java_common.create_provider to JavaInfo()
iirina Jul 8, 2019
31f9abd
Fix scala/private/common.bzl
iirina Jul 8, 2019
0cd63ef
Fix some build failures.
iirina Jul 8, 2019
819fba3
Fix some more builds.
iirina Jul 9, 2019
e782e99
Remove scala/private/common:create_java_provider
iirina Jul 10, 2019
f412410
Remove unused _create_provider.
iirina Jul 10, 2019
56b7294
Remove unused load statement
iirina Jul 10, 2019
c5b3d88
Also propagate deps and runtime_deps in scala_import with no jars.
iirina Jul 10, 2019
ac7887d
Address review comments.
iirina Jul 10, 2019
483928f
Update custom-jvm-rule.bzl
ittaiz Jul 10, 2019
d4a6f45
Update BUILD
ittaiz Jul 10, 2019
0edf49b
typo
ittaiz Jul 10, 2019
408f51d
Removed implicit value for deps_providers.
iirina Jul 11, 2019
86a229e
Add dummy intermediate jar file for scala_import with no jars.
iirina Jul 11, 2019
549f2e0
Cleanup code.
iirina Jul 12, 2019
c27ca9b
Replace + with lists extend.
iirina Jul 12, 2019
99a3f7e
Revert enabling --all_incompatible_changes test by mistake.
iirina Jul 12, 2019
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
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,4 @@ hash1
hash2
.DS_store
.bazel_cache
.ijwb
6 changes: 6 additions & 0 deletions scala/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,9 @@ _declare_scalac_provider(
],
visibility = ["//visibility:public"],
)

java_library(
name = "PlaceHolderClassToCreateEmptyJarForScalaImport",
srcs = ["PlaceHolderClassToCreateEmptyJarForScalaImport.java"],
visibility = ["//visibility:public"],
)
1 change: 1 addition & 0 deletions scala/PlaceHolderClassToCreateEmptyJarForScalaImport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
public class PlaceHolderClassToCreateEmptyJarForScalaImport { }
19 changes: 7 additions & 12 deletions scala/private/common.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,11 +62,14 @@ def _collect_jars_when_dependency_analyzer_is_off(
runtime_jars = []
jars2labels = {}

deps_providers = []

for dep_target in dep_targets:
# we require a JavaInfo for dependencies
# must use java_import or scala_import if you have raw files
if JavaInfo in dep_target:
java_provider = dep_target[JavaInfo]
deps_providers.append(java_provider)
compile_jars.append(java_provider.compile_jars)
runtime_jars.append(java_provider.transitive_runtime_jars)

Expand All @@ -90,19 +93,22 @@ def _collect_jars_when_dependency_analyzer_is_off(
transitive_runtime_jars = depset(transitive = runtime_jars),
jars2labels = JarsToLabelsInfo(jars_to_labels = jars2labels),
transitive_compile_jars = depset(transitive = compile_jars + plus_one_deps_compile_jars),
deps_providers = deps_providers,
)

def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
transitive_compile_jars = []
jars2labels = {}
compile_jars = []
runtime_jars = []
deps_providers = []

for dep_target in dep_targets:
# we require a JavaInfo for dependencies
# must use java_import or scala_import if you have raw files
if JavaInfo in dep_target:
java_provider = dep_target[JavaInfo]
deps_providers.append(java_provider)
current_dep_compile_jars = java_provider.compile_jars
current_dep_transitive_compile_jars = java_provider.transitive_compile_time_jars
runtime_jars.append(java_provider.transitive_runtime_jars)
Expand All @@ -124,6 +130,7 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets):
transitive_runtime_jars = depset(transitive = runtime_jars),
jars2labels = JarsToLabelsInfo(jars_to_labels = jars2labels),
transitive_compile_jars = depset(transitive = transitive_compile_jars),
deps_providers = deps_providers,
)

# When import mavan_jar's for scala macros we have to use the jar:file requirement
Expand Down Expand Up @@ -163,15 +170,3 @@ def _provider_of_dependency_label_of(dependency, path):
return dependency[JarsToLabelsInfo].jars_to_labels.get(path)
else:
return None

# TODO this seems to have limited value now that JavaInfo has everything
def create_java_provider(scalaattr, transitive_compile_time_jars):
return java_common.create_provider(
use_ijar = False,
compile_time_jars = scalaattr.compile_jars,
runtime_jars = scalaattr.transitive_runtime_jars,
transitive_compile_time_jars = depset(
transitive = [transitive_compile_time_jars, scalaattr.compile_jars],
),
transitive_runtime_jars = scalaattr.transitive_runtime_jars,
)
58 changes: 46 additions & 12 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ load(
"collect_jars",
"collect_plugin_paths",
"collect_srcjars",
"create_java_provider",
"not_sources_jar",
"write_manifest",
)
Expand Down Expand Up @@ -318,10 +317,10 @@ StatsfileOutput: {statsfile_output}
)

def _interim_java_provider_for_java_compilation(scala_output):
Copy link
Member

Choose a reason for hiding this comment

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

I think I need to read the code a bit more in depth but from what I remember this won't work because this way we won't have the java code present at runtime. I think the whole "interim" approach is now wrong and was needed before.
I might be mistaken but my hunch is that a bit more of a refactor is needed to have the scala compilation propagate out the JavaInfo of the java (or a different refactoring)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the reason why runtime_jars was previously empty?

IIUC this provider is needed to compile Java sources with scala dependencies. Does scala also need this provider?

Copy link
Member

Choose a reason for hiding this comment

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

Context:
We call java_common.compile with all inputs to the rule (deps/runtime_deps/etc). In addition we need to give it a provider representing the output of scalac we ran a second before (javac compilation depends on scalac compilation). This solution was a hack meant to generate a provider which will be added on top of the other deps

Copy link
Member

Choose a reason for hiding this comment

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

Also now that I think about it a bit more then maybe I'm wrong and your solution does work. This will mean that javac will get an ijar of the scalac code and it won't propagate it onwards since it's neverlink. Sounds good actually.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly. This only works because it's neverlink, otherwise we need to pass in the full jar as well (output_jar=full_jar, compile_jar=ijar) to avoid having an ijar at runtime.

return java_common.create_provider(
use_ijar = False,
compile_time_jars = [scala_output],
runtime_jars = [],
return JavaInfo(
output_jar = scala_output,
compile_jar = scala_output,
neverlink = True
)

def _scalac_provider(ctx):
Expand Down Expand Up @@ -371,10 +370,12 @@ def try_to_compile_java_jar(
host_javabase = find_java_runtime_toolchain(ctx, ctx.attr._host_javabase),
strict_deps = ctx.fragments.java.strict_java_deps,
)

return struct(
ijar = provider.compile_jars.to_list().pop(),
jar = full_java_jar,
source_jars = provider.source_jars,
java_compilation_provider = provider
)

def collect_java_providers_of(deps):
Expand All @@ -394,11 +395,14 @@ def _compile_or_empty(
jars2labels,
implicit_junit_deps_needed_for_java_compilation,
unused_dependency_checker_mode,
unused_dependency_checker_ignored_targets):
unused_dependency_checker_ignored_targets,
deps_providers):
# We assume that if a srcjar is present, it is not 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)

# no need to build ijar when empty
return struct(
class_jar = ctx.outputs.jar,
Expand All @@ -408,6 +412,7 @@ def _compile_or_empty(
ijars = [ctx.outputs.jar],
java_jar = False,
source_jars = [],
merged_provider = scala_compilation_provider
)
else:
in_srcjars = [
Expand Down Expand Up @@ -471,6 +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)

# compile the java now
java_jar = try_to_compile_java_jar(
ctx,
Expand All @@ -490,6 +497,11 @@ def _compile_or_empty(

coverage = _jacoco_offline_instrument(ctx, ctx.outputs.jar)

if java_jar:
merged_provider = java_common.merge([scala_compilation_provider, java_jar.java_compilation_provider])
else:
merged_provider = scala_compilation_provider

return struct(
class_jar = ctx.outputs.jar,
coverage = coverage,
Expand All @@ -498,8 +510,24 @@ def _compile_or_empty(
ijars = ijars,
java_jar = java_jar,
source_jars = source_jars,
merged_provider = merged_provider
)

def _create_scala_compilation_provider(ctx, ijar, deps_providers):
exports = []
if hasattr(ctx.attr, "exports"):
exports = [dep[JavaInfo] for dep in ctx.attr.exports]
runtime_deps = []
if hasattr(ctx.attr, "runtime_deps"):
runtime_deps = [dep[JavaInfo] for dep in ctx.attr.runtime_deps]
return JavaInfo(
Copy link
Member

Choose a reason for hiding this comment

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

I just realized that this doesn’t pass the source_jar.
Any chance you can add that? If not I’ll add it in a separate PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure which is the source jar here. Is it _pack_source_jars(ctx)? Note that JavaInfo() takes only one source jar, so probably there needs to be some more packing before.

output_jar = ctx.outputs.jar,
compile_jar = ijar,
deps = deps_providers,
exports = exports,
runtime_deps = runtime_deps,
)

def _build_deployable(ctx, jars_list):
# This calls bazels singlejar utility.
# For a full list of available command line options see:
Expand Down Expand Up @@ -725,11 +753,13 @@ def _collect_jars_from_common_ctx(
transitive_rjars,
jars2labels,
transitive_compile_jars,
deps_providers
) = (
deps_jars.compile_jars,
deps_jars.transitive_runtime_jars,
deps_jars.jars2labels,
deps_jars.transitive_compile_jars,
deps_jars.deps_providers
)

transitive_rjars = depset(
Expand All @@ -742,6 +772,7 @@ def _collect_jars_from_common_ctx(
jars2labels = jars2labels,
transitive_compile_jars = transitive_compile_jars,
transitive_runtime_jars = transitive_rjars,
deps_providers = deps_providers,
)

def _lib(
Expand Down Expand Up @@ -781,6 +812,7 @@ def _lib(
unused_dependency_checker_ignored_targets
],
unused_dependency_checker_mode = unused_dependency_checker_mode,
deps_providers = jars.deps_providers,
)

transitive_rjars = depset(outputs.full_jars, transitive = [transitive_rjars])
Expand Down Expand Up @@ -817,13 +849,11 @@ def _lib(
transitive_runtime_jars = transitive_rjars,
)

java_provider = create_java_provider(scalaattr, jars.transitive_compile_jars)

return struct(
files = depset([ctx.outputs.jar] + outputs.full_jars), # Here is the default output
instrumented_files = outputs.coverage.instrumented_files,
jars_to_labels = jars.jars2labels,
providers = [java_provider, jars.jars2labels] + outputs.coverage.providers,
providers = [outputs.merged_provider, jars.jars2labels] + outputs.coverage.providers,
runfiles = runfiles,
scala = scalaattr,
)
Expand Down Expand Up @@ -877,6 +907,7 @@ def _scala_binary_common(
java_wrapper,
unused_dependency_checker_mode,
unused_dependency_checker_ignored_targets,
deps_providers,
implicit_junit_deps_needed_for_java_compilation = [],
runfiles_ext = []):
write_manifest(ctx)
Expand All @@ -892,6 +923,7 @@ def _scala_binary_common(
unused_dependency_checker_ignored_targets =
unused_dependency_checker_ignored_targets,
unused_dependency_checker_mode = unused_dependency_checker_mode,
deps_providers = deps_providers,
) # no need to build an ijar for an executable
rjars = depset(outputs.full_jars, transitive = [rjars])

Expand All @@ -918,14 +950,12 @@ def _scala_binary_common(
transitive_runtime_jars = rjars,
)

java_provider = create_java_provider(scalaattr, transitive_compile_time_jars)

return struct(
executable = executable,
coverage = outputs.coverage,
files = depset([executable, ctx.outputs.jar]),
instrumented_files = outputs.coverage.instrumented_files,
providers = [java_provider, jars2labels] + outputs.coverage.providers,
providers = [outputs.merged_provider, jars2labels] + outputs.coverage.providers,
runfiles = runfiles,
scala = scalaattr,
transitive_rjars =
Expand Down Expand Up @@ -991,6 +1021,7 @@ def scala_binary_impl(ctx):
ctx.attr.unused_dependency_checker_ignored_targets
],
unused_dependency_checker_mode = unused_dependency_checker_mode,
deps_providers = jars.deps_providers,
)
_write_executable(
ctx = ctx,
Expand Down Expand Up @@ -1054,6 +1085,7 @@ trap finish EXIT
ctx.attr.unused_dependency_checker_ignored_targets
],
unused_dependency_checker_mode = unused_dependency_checker_mode,
deps_providers = jars.deps_providers,
)
_write_executable(
ctx = ctx,
Expand Down Expand Up @@ -1140,6 +1172,7 @@ def scala_test_impl(ctx):
unused_dependency_checker_ignored_targets,
unused_dependency_checker_mode = unused_dependency_checker_mode,
runfiles_ext = [argsFile],
deps_providers = jars.deps_providers,
)

rjars = out.transitive_rjars
Expand Down Expand Up @@ -1265,6 +1298,7 @@ def scala_junit_test_impl(ctx):
unused_dependency_checker_ignored_targets =
unused_dependency_checker_ignored_targets,
unused_dependency_checker_mode = unused_dependency_checker_mode,
deps_providers = jars.deps_providers,
)

if ctx.attr.tests_from:
Expand Down
68 changes: 22 additions & 46 deletions scala/scala_import.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,64 +24,36 @@ def _scala_import_impl(ctx):
ctx.label,
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:
iirina marked this conversation as resolved.
Show resolved Hide resolved
current_target_providers = [_new_java_info(ctx, ctx.file._placeholder_jar)]

return struct(
scala = struct(
outputs = struct(jars = intellij_metadata),
),
providers = [
_create_provider(
current_jars,
transitive_runtime_jars,
jars,
exports,
ctx.attr.neverlink,
ctx.file.srcjar,
intellij_metadata,
),
java_common.merge(current_target_providers),
DefaultInfo(
files = current_jars,
),
JarsToLabelsInfo(jars_to_labels = jars2labels),
],
)

def _create_provider(
current_target_compile_jars,
transitive_runtime_jars,
jars,
exports,
neverlink,
source_jar,
intellij_metadata):
transitive_runtime_jars = [
transitive_runtime_jars,
jars.transitive_runtime_jars,
exports.transitive_runtime_jars,
]

if not neverlink:
transitive_runtime_jars.append(current_target_compile_jars)

source_jars = []

if source_jar:
source_jars.append(source_jar)
else:
for metadata in intellij_metadata:
source_jars.extend(metadata.source_jars)

return java_common.create_provider(
use_ijar = False,
compile_time_jars = depset(
transitive = [current_target_compile_jars, exports.compile_jars],
),
transitive_compile_time_jars = depset(transitive = [
jars.transitive_compile_jars,
current_target_compile_jars,
exports.transitive_compile_jars,
]),
transitive_runtime_jars = depset(transitive = transitive_runtime_jars),
source_jars = source_jars,
def _new_java_info(ctx, jar):
return JavaInfo(
output_jar = jar,
compile_jar = jar,
exports = [target[JavaInfo] for target in ctx.attr.exports],
deps = [target[JavaInfo] for target in ctx.attr.deps],
runtime_deps = [target[JavaInfo] for target in ctx.attr.runtime_deps],
source_jar = ctx.file.srcjar,
neverlink = ctx.attr.neverlink,
)

def _add_labels_of_current_code_jars(code_jars, label, jars2labels):
Expand Down Expand Up @@ -169,5 +141,9 @@ scala_import = rule(
"exports": attr.label_list(),
"neverlink": attr.bool(),
"srcjar": attr.label(allow_single_file = True),
"_placeholder_jar": attr.label(
allow_single_file = True,
default = Label("@io_bazel_rules_scala//scala:libPlaceHolderClassToCreateEmptyJarForScalaImport.jar"),
),
},
)
Loading