-
-
Notifications
You must be signed in to change notification settings - Fork 287
Centralize dependency logic #975
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
Changes from 8 commits
e746893
8b0da8a
927e8c0
7ea430a
9baabe4
9d60be2
f442b26
fe91373
4a897f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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" |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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", | ||
|
@@ -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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would you mind to change it to just There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @borkaehw why do you think this is a better pattern in general? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 In my opinion,
make the phase architecture messy. When we finalize #965, I imagine the functions should all be consistent like Also, functions with a super long list of arguments like There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
) | ||
|
||
( | ||
|
Uh oh!
There was an error while loading. Please reload this page.