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

Centralize dependency logic #975

Merged
merged 9 commits into from
Feb 6, 2020
Merged
Show file tree
Hide file tree
Changes from 8 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
124 changes: 51 additions & 73 deletions scala/private/common.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -11,44 +11,15 @@ def write_manifest_file(actions, output_file, main_class):

def collect_jars(
dep_targets,
dependency_analyzer_is_off = True,
unused_dependency_checker_is_off = True,
plus_one_deps_is_off = True):
dependency_mode,
need_direct_info,
need_indirect_info):
"""Compute the runtime and compile-time dependencies from the given targets""" # noqa

if dependency_analyzer_is_off:
return _collect_jars_when_dependency_analyzer_is_off(
dep_targets,
unused_dependency_checker_is_off,
plus_one_deps_is_off,
)
else:
return _collect_jars_when_dependency_analyzer_is_on(dep_targets)

def collect_plugin_paths(plugins):
"""Get the actual jar paths of plugins as a depset."""
paths = []
for p in plugins:
if hasattr(p, "path"):
paths.append(p)
elif JavaInfo in p:
paths.extend([j.class_jar for j in p[JavaInfo].outputs.jars])
# support http_file pointed at a jar. http_jar uses ijar,
# which breaks scala macros

elif hasattr(p, "files"):
paths.extend([f for f in p.files.to_list() if not_sources_jar(f.basename)])
return depset(paths)

def _collect_jars_when_dependency_analyzer_is_off(
dep_targets,
unused_dependency_checker_is_off,
plus_one_deps_is_off):
transitive_compile_jars = []
jars2labels = {}
compile_jars = []
plus_one_deps_compile_jars = []
runtime_jars = []
jars2labels = {}

deps_providers = []

for dep_target in dep_targets:
Expand All @@ -59,60 +30,67 @@ def _collect_jars_when_dependency_analyzer_is_off(
compile_jars.append(java_provider.compile_jars)
runtime_jars.append(java_provider.transitive_runtime_jars)

if not unused_dependency_checker_is_off:
additional_transitive_compile_jars = _additional_transitive_compile_jars(
java_provider = java_provider,
dep_target = dep_target,
dependency_mode = dependency_mode,
)
transitive_compile_jars.append(additional_transitive_compile_jars)

if need_direct_info or need_indirect_info:
if need_indirect_info:
all_jars = additional_transitive_compile_jars.to_list()
else:
all_jars = []
add_labels_of_jars_to(
jars2labels,
dep_target,
[],
all_jars,
java_provider.compile_jars.to_list(),
)

if (not plus_one_deps_is_off) and (PlusOneDeps in dep_target):
plus_one_deps_compile_jars.append(
depset(transitive = [dep[JavaInfo].compile_jars for dep in dep_target[PlusOneDeps].direct_deps if JavaInfo in dep]),
)

return struct(
compile_jars = depset(transitive = compile_jars),
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),
transitive_compile_jars = depset(transitive = transitive_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
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)

compile_jars.append(current_dep_compile_jars)
transitive_compile_jars.append(current_dep_transitive_compile_jars)
def collect_plugin_paths(plugins):
"""Get the actual jar paths of plugins as a depset."""
paths = []
for p in plugins:
if hasattr(p, "path"):
paths.append(p)
elif JavaInfo in p:
paths.extend([j.class_jar for j in p[JavaInfo].outputs.jars])
# support http_file pointed at a jar. http_jar uses ijar,
# which breaks scala macros

add_labels_of_jars_to(
jars2labels,
dep_target,
current_dep_transitive_compile_jars.to_list(),
current_dep_compile_jars.to_list(),
)
elif hasattr(p, "files"):
paths.extend([f for f in p.files.to_list() if not_sources_jar(f.basename)])
return depset(paths)

return struct(
compile_jars = depset(transitive = compile_jars),
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,
)
def _additional_transitive_compile_jars(
java_provider,
dep_target,
dependency_mode):
if dependency_mode == "transitive":
return java_provider.transitive_compile_time_jars
elif dependency_mode == "plus-one":
# dep_target will not always have a PlusOneDeps provider, such as
# with scala_maven_import_external, hence the need for the fallback.
if PlusOneDeps in dep_target:
plus_one_jars = [dep[JavaInfo].compile_jars for dep in dep_target[PlusOneDeps].direct_deps if JavaInfo in dep]

# plus_one_jars only contains the deps of deps, not the deps themselves.
# Hence the need to include the dep's compile jars anyways
return depset(transitive = plus_one_jars + [java_provider.compile_jars])
else:
return java_provider.compile_jars
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
else: # direct
return java_provider.compile_jars

# When import mavan_jar's for scala macros we have to use the jar:file requirement
# since bazel 0.6.0 this brings in the source jar too
Expand Down
60 changes: 60 additions & 0 deletions scala/private/dependency.bzl
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
# This file contains all computations for what the dependency mode is
# (i.e. transitive, plus-one, direct, etc)
# and what/how dependency analysis is performed (unused deps, strict deps, etc).

def new_dependency_info(
dependency_mode,
unused_deps_mode,
strict_deps_mode,
dependency_tracking_method):
is_strict_deps_on = strict_deps_mode != "off"

# Currently we do not support running both strict and unused deps at the same time
if is_strict_deps_on:
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
unused_deps_mode = "off"

is_unused_deps_on = unused_deps_mode != "off"

need_direct_jars = is_strict_deps_on or is_unused_deps_on
need_direct_targets = is_unused_deps_on

return struct(
dependency_mode = dependency_mode,
need_indirect_info = is_strict_deps_on,
need_direct_jars = need_direct_jars,
need_direct_targets = need_direct_targets,
need_direct_info = need_direct_jars or need_direct_targets,
dependency_tracking_method = "high-level",
unused_deps_mode = unused_deps_mode,
strict_deps_mode = strict_deps_mode,
use_analyzer = is_strict_deps_on or is_unused_deps_on,
)

# TODO(https://github.com/bazelbuild/rules_scala/issues/987): Clariy the situation
def legacy_unclear_dependency_info_for_protobuf_scrooge(ctx):
return new_dependency_info(
dependency_mode = _legacy_unclear_dependency_mode_for_protobuf_scrooge(ctx),
unused_deps_mode = "off",
strict_deps_mode = get_strict_deps_mode(ctx),
dependency_tracking_method = "high-level",
)

# TODO(https://github.com/bazelbuild/rules_scala/issues/987): Clariy the situation
def _legacy_unclear_dependency_mode_for_protobuf_scrooge(ctx):
if is_strict_deps_on(ctx):
return "transitive"
else:
return "direct"

def get_strict_deps_mode(ctx):
if not hasattr(ctx.attr, "_dependency_analyzer_plugin"):
return "off"

# when the strict deps FT is removed the "default" check
# will be removed since "default" will mean it's turned on
if ctx.fragments.java.strict_java_deps == "default":
return "off"
return ctx.fragments.java.strict_java_deps

def is_strict_deps_on(ctx):
return get_strict_deps_mode(ctx) != "off"
7 changes: 6 additions & 1 deletion scala/private/phases/phase_collect_exports_jars.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,9 @@ def phase_collect_exports_jars(ctx, p):
# Add information from exports (is key that AFTER all build actions/runfiles analysis)
# Since after, will not show up in deploy_jar or old jars runfiles
# Notice that compile_jars is intentionally transitive for exports
return collect_jars(ctx.attr.exports)
return collect_jars(
ctx.attr.exports,
"direct",
False,
False,
)
26 changes: 6 additions & 20 deletions scala/private/phases/phase_collect_jars.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
#
# DOCUMENT THIS
#
load(
"@io_bazel_rules_scala//scala/private:rule_impls.bzl",
"is_dependency_analyzer_off",
"is_plus_one_deps_off",
)
load(
"@io_bazel_rules_scala//scala/private:common.bzl",
"collect_jars",
Expand Down Expand Up @@ -46,41 +41,32 @@ def phase_collect_jars_junit_test(ctx, p):
)
return _phase_collect_jars_default(ctx, p, args)

def phase_collect_jars_library_for_plugin_bootstrapping(ctx, p):
args = struct(
unused_dependency_checker_mode = "off",
)
return _phase_collect_jars_default(ctx, p, args)

def phase_collect_jars_common(ctx, p):
return _phase_collect_jars_default(ctx, p)

def _phase_collect_jars_default(ctx, p, _args = struct()):
return _phase_collect_jars(
ctx,
p.dependency,
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind to change it to just p? I know using p.dependency will be less verbose inside the function but just for the purpose of consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was in response to a comment by @ittaiz . I am fine either way.

Copy link
Member

Choose a reason for hiding this comment

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

@borkaehw why do you think this is a better pattern in general?
Let’s ignore consistency for a sec since maybe we’ll decide to change the pattern

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should keep the function signature as simple as possible. By the fact that p should contain all the information we need, we may expand the data structure within the function instead.

In my opinion,

phase_a(ctx, p, a)
phase_b(ctx, p, a, b)
phase_c(ctx, p, a, b, c, d, e)

make the phase architecture messy. When we finalize #965, I imagine the functions should all be consistent like phase_a(ctx, internal_proviers).

Also, functions with a super long list of arguments like compile_scala makes it hard to read. I, in general, prefer to keep arguments as less as possible and no keyword is needed.

Copy link
Member

Choose a reason for hiding this comment

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

@borkaehw I understand the value in what you say but to be honest I actually quite disagree with it. I think that bottom line having similar APIs is less beneficial than having phases explicitly declare which internal providers they need. Something like providers = [JavaInfo] which we have in startlark.
Having said that I'd rather not start this debate here over this change and so for consistency I apologize @Jamie5 and I'll ask you to revert to your original change. Sorry again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks for sharing your idea.

_args.base_classpath if hasattr(_args, "base_classpath") else p.scalac_provider.default_classpath,
_args.extra_deps if hasattr(_args, "extra_deps") else [],
_args.extra_runtime_deps if hasattr(_args, "extra_runtime_deps") else [],
_args.unused_dependency_checker_mode if hasattr(_args, "unused_dependency_checker_mode") else p.unused_deps_checker,
)

# Extract very common code out from dependency analysis into single place
# automatically adds dependency on scala-library and scala-reflect
# collects jars from deps, runtime jars from runtime_deps, and
def _phase_collect_jars(
ctx,
dependency_info,
base_classpath,
extra_deps,
extra_runtime_deps,
unused_dependency_checker_mode):
unused_dependency_checker_is_off = unused_dependency_checker_mode == "off"
dependency_analyzer_is_off = is_dependency_analyzer_off(ctx)

extra_runtime_deps):
deps_jars = collect_jars(
ctx.attr.deps + extra_deps + base_classpath,
dependency_analyzer_is_off,
unused_dependency_checker_is_off,
is_plus_one_deps_off(ctx),
dependency_info.dependency_mode,
dependency_info.need_direct_info,
dependency_info.need_indirect_info,
)

(
Expand Down
31 changes: 9 additions & 22 deletions scala/private/phases/phase_compile.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,6 @@ def phase_compile_library(ctx, p):

def phase_compile_library_for_plugin_bootstrapping(ctx, p):
args = struct(
unused_dependency_checker_ignored_targets = [
ittaiz marked this conversation as resolved.
Show resolved Hide resolved
target.label
for target in p.scalac_provider.default_classpath + ctx.attr.exports
],
unused_dependency_checker_mode = "off",
buildijar = ctx.attr.build_ijar,
)
return _phase_compile_default(ctx, p, args)
Expand Down Expand Up @@ -114,8 +109,7 @@ def _phase_compile_default(ctx, p, _args = struct()):
_args.srcjars if hasattr(_args, "srcjars") else depset(),
_args.buildijar if hasattr(_args, "buildijar") else True,
_args.implicit_junit_deps_needed_for_java_compilation if hasattr(_args, "implicit_junit_deps_needed_for_java_compilation") else [],
_args.unused_dependency_checker_ignored_targets if hasattr(_args, "unused_dependency_checker_ignored_targets") else [],
_args.unused_dependency_checker_mode if hasattr(_args, "unused_dependency_checker_mode") else p.unused_deps_checker,
unused_dependency_checker_ignored_targets = _args.unused_dependency_checker_ignored_targets if hasattr(_args, "unused_dependency_checker_ignored_targets") else [],
)

def _phase_compile(
Expand All @@ -125,8 +119,7 @@ def _phase_compile(
buildijar,
# TODO: generalize this hack
implicit_junit_deps_needed_for_java_compilation,
unused_dependency_checker_ignored_targets,
unused_dependency_checker_mode):
unused_dependency_checker_ignored_targets):
manifest = ctx.outputs.manifest
jars = p.collect_jars.compile_jars
rjars = p.collect_jars.transitive_runtime_jars
Expand All @@ -144,10 +137,10 @@ def _phase_compile(
transitive_compile_jars,
jars2labels,
implicit_junit_deps_needed_for_java_compilation,
unused_dependency_checker_mode,
unused_dependency_checker_ignored_targets,
p.dependency,
deps_providers,
default_classpath,
unused_dependency_checker_ignored_targets,
)

# TODO: simplify the return values and use provider
Expand All @@ -169,10 +162,10 @@ def _compile_or_empty(
transitive_compile_jars,
jars2labels,
implicit_junit_deps_needed_for_java_compilation,
unused_dependency_checker_mode,
unused_dependency_checker_ignored_targets,
dependency_info,
deps_providers,
default_classpath):
default_classpath,
unused_dependency_checker_ignored_targets):
# 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)
Expand All @@ -190,11 +183,6 @@ def _compile_or_empty(
in_srcjars = _get_files_with_extension(ctx, _srcjar_extension)
all_srcjars = depset(in_srcjars, transitive = [srcjars])

# We are not able to verify whether dependencies are used when compiling java sources
# Thus we disable unused dependency checking when java sources are found
if len(java_srcs) != 0:
unused_dependency_checker_mode = "off"

sources = scala_srcs + java_srcs
_compile_scala(
ctx,
Expand All @@ -216,9 +204,8 @@ def _compile_or_empty(
ctx.attr.expect_java_output,
ctx.attr.scalac_jvm_flags,
ctx.attr._scalac,
unused_dependency_checker_ignored_targets =
unused_dependency_checker_ignored_targets,
unused_dependency_checker_mode = unused_dependency_checker_mode,
dependency_info,
unused_dependency_checker_ignored_targets,
)

# build ijar if needed
Expand Down
Loading