-
-
Notifications
You must be signed in to change notification settings - Fork 287
add plus one deps transitive mode for compilation #698
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 all commits
16a6d8b
66ac5aa
b7979ba
a530f8b
a7e96df
55ded3a
cb68338
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,17 @@ | ||
""" | ||
Keeps direct compile dependencies of targets. | ||
This enables targets to pass the to compiler the plus one dependencies in addition to the direct ones. | ||
For motivation of plus one see the e2e tests | ||
""" | ||
PlusOneDeps = provider( | ||
fields = { | ||
'direct_deps' : 'list of direct compile dependencies of a target', | ||
} | ||
) | ||
|
||
def _collect_plus_one_deps_aspect_impl(target, ctx): | ||
return [PlusOneDeps(direct_deps = getattr(ctx.rule.attr,'deps',[]))] | ||
|
||
collect_plus_one_deps_aspect = aspect(implementation = _collect_plus_one_deps_aspect_impl, | ||
attr_aspects = ['deps'], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
load("@io_bazel_rules_scala//scala:jars_to_labels.bzl", "JarsToLabelsInfo") | ||
load("@io_bazel_rules_scala//scala:plusone.bzl", "PlusOneDeps") | ||
|
||
def write_manifest(ctx): | ||
main_class = getattr(ctx.attr, "main_class", None) | ||
|
@@ -22,21 +23,25 @@ def collect_srcjars(targets): | |
def collect_jars( | ||
dep_targets, | ||
dependency_analyzer_is_off = True, | ||
unused_dependency_checker_is_off = True): | ||
unused_dependency_checker_is_off = True, | ||
plus_one_deps_is_off = True): | ||
"""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_jars_when_dependency_analyzer_is_off( | ||
dep_targets, | ||
unused_dependency_checker_is_off): | ||
unused_dependency_checker_is_off, | ||
plus_one_deps_is_off): | ||
compile_jars = [] | ||
plus_one_deps_compile_jars = [] | ||
runtime_jars = [] | ||
jars2labels = {} | ||
|
||
|
@@ -58,11 +63,16 @@ def _collect_jars_when_dependency_analyzer_is_off( | |
else: | ||
print("ignored dependency, has no JavaInfo: " + str(dep_target)) | ||
|
||
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(), | ||
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. huh, this is interesting. This is a change from before. We weren't really tracking transitive compile jars? 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. if strict deps was off we didn't track it |
||
transitive_compile_jars = depset(transitive = compile_jars + plus_one_deps_compile_jars), | ||
) | ||
|
||
def _collect_jars_when_dependency_analyzer_is_on(dep_targets): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -220,6 +220,8 @@ CurrentTarget: {current_target} | |
ignored_targets = ignored_targets, | ||
current_target = current_target, | ||
) | ||
if is_dependency_analyzer_off(ctx) and not _is_plus_one_deps_off(ctx): | ||
compiler_classpath_jars = transitive_compile_jars | ||
|
||
plugins_list = plugins.to_list() | ||
plugin_arg = _join_path(plugins_list) | ||
|
@@ -616,6 +618,9 @@ def is_dependency_analyzer_on(ctx): | |
def is_dependency_analyzer_off(ctx): | ||
return not is_dependency_analyzer_on(ctx) | ||
|
||
def _is_plus_one_deps_off(ctx): | ||
return ctx.toolchains["@io_bazel_rules_scala//scala:toolchain_type"].plus_one_deps_mode == "off" | ||
|
||
# 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 | ||
|
@@ -631,6 +636,7 @@ def _collect_jars_from_common_ctx( | |
ctx.attr.deps + extra_deps + base_classpath, | ||
dependency_analyzer_is_off, | ||
unused_dependency_checker_is_off, | ||
_is_plus_one_deps_off(ctx), | ||
) | ||
|
||
( | ||
|
@@ -985,9 +991,10 @@ def scala_test_impl(ctx): | |
] | ||
unused_dependency_checker_is_off = unused_dependency_checker_mode == "off" | ||
|
||
scalatest_base_classpath = scalac_provider.default_classpath + [ctx.attr._scalatest] | ||
jars = _collect_jars_from_common_ctx( | ||
ctx, | ||
scalac_provider.default_classpath, | ||
scalatest_base_classpath, | ||
extra_runtime_deps = [ | ||
ctx.attr._scalatest_reporter, | ||
ctx.attr._scalatest_runner, | ||
|
@@ -1006,36 +1013,6 @@ def scala_test_impl(ctx): | |
jars.jars2labels, | ||
) | ||
|
||
# _scalatest is an http_jar, so its compile jar is run through ijar | ||
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. why is this all removed now? Is this because we shouldn't depend on a test anyway? 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. Good question. Lastly tests can depend on tests, not a problem also in java. |
||
# however, contains macros, so need to handle separately | ||
scalatest_jars = collect_jars([ctx.attr._scalatest]).transitive_runtime_jars | ||
cjars = depset(transitive = [cjars, scalatest_jars]) | ||
transitive_rjars = depset(transitive = [transitive_rjars, scalatest_jars]) | ||
|
||
if is_dependency_analyzer_on(ctx): | ||
transitive_compile_jars = depset( | ||
transitive = [scalatest_jars, transitive_compile_jars], | ||
) | ||
scalatest_jars_list = scalatest_jars.to_list() | ||
j2l = jars_to_labels.jars_to_labels | ||
add_labels_of_jars_to( | ||
j2l, | ||
ctx.attr._scalatest, | ||
scalatest_jars_list, | ||
scalatest_jars_list, | ||
) | ||
jars_to_labels = JarsToLabelsInfo(jars_to_labels = j2l) | ||
|
||
elif not unused_dependency_checker_is_off: | ||
j2l = jars_to_labels.jars_to_labels | ||
add_labels_of_jars_to( | ||
j2l, | ||
ctx.attr._scalatest, | ||
[], | ||
scalatest_jars.to_list(), | ||
) | ||
jars_to_labels = JarsToLabelsInfo(jars_to_labels = j2l) | ||
|
||
args = " ".join([ | ||
"-R \"{path}\"".format(path = ctx.outputs.jar.short_path), | ||
_scala_test_flags(ctx), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
load("//scala:scala_toolchain.bzl", "scala_toolchain") | ||
scala_toolchain( | ||
name = "plus_one_deps_impl", | ||
plus_one_deps_mode = "on", | ||
visibility = ["//visibility:public"], | ||
) | ||
toolchain( | ||
name = "plus_one_deps", | ||
toolchain = "plus_one_deps_impl", | ||
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type", | ||
visibility = ["//visibility:public"], | ||
) | ||
scala_toolchain( | ||
name = "plus_one_deps_with_unused_error_impl", | ||
unused_dependency_checker_mode = "error", | ||
plus_one_deps_mode = "on", | ||
visibility = ["//visibility:public"], | ||
) | ||
toolchain( | ||
name = "plus_one_deps_with_unused_error", | ||
toolchain = "plus_one_deps_with_unused_error_impl", | ||
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type", | ||
visibility = ["//visibility:public"], | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class A { | ||
println(new B().hi) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class B extends C { | ||
def hi: String = "hi" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
load("//scala:scala.bzl", "scala_library") | ||
scala_library( | ||
name = "a", | ||
srcs = ["A.scala"], | ||
deps = [":b"], | ||
) | ||
scala_library( | ||
name = "b", | ||
srcs = ["B.scala"], | ||
deps = [":c"], | ||
) | ||
scala_library( | ||
name = "c", | ||
srcs = ["C.scala"], | ||
deps = [":d"], | ||
exports = ["d"], | ||
) | ||
scala_library( | ||
name = "d", | ||
srcs = ["D.scala"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class C extends D |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class D { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.external_deps | ||
import org.springframework.dao.DataAccessException | ||
|
||
class A { | ||
println(classOf[DataAccessException]) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
load("//scala:scala.bzl", "scala_library") | ||
scala_library( | ||
name = "a", | ||
srcs = ["A.scala"], | ||
deps = ["@org_springframework_spring_tx"] | ||
|
||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class A { | ||
println(new B().hi) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class B extends C { | ||
def hi: String = "hi" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
load("//scala:scala.bzl", "scala_library") | ||
scala_library( | ||
name = "a", | ||
srcs = ["A.scala"], | ||
deps = [":b"], | ||
) | ||
scala_library( | ||
name = "b", | ||
srcs = ["B.scala"], | ||
deps = [":c"], | ||
) | ||
scala_library( | ||
name = "c", | ||
srcs = ["C.scala"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.internal_deps | ||
|
||
class C |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.with_unused_deps | ||
|
||
class A { | ||
println(new B().hi) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.with_unused_deps | ||
|
||
class B { | ||
def hi: String = { | ||
println(classOf[C]) | ||
"hi" | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
load("//scala:scala.bzl", "scala_library") | ||
scala_library( | ||
name = "a", | ||
srcs = ["A.scala"], | ||
deps = [":b",":c"], | ||
) | ||
scala_library( | ||
name = "b", | ||
srcs = ["B.scala"], | ||
deps = [":c"], | ||
) | ||
scala_library( | ||
name = "c", | ||
srcs = ["C.scala"], | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
package scalarules.test_expect_failure.plus_one_deps.with_unused_deps | ||
|
||
class C |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should have so many default values here. especially with the same type... but we can punt that concern I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely agree...
I also thought about it and the worst thing is that they are all negative case.
I think we still need to do a lot of refactoring here but it's hard to keep up, fix bugs, refactor while also doing all of the other d2d stuff at work...
So thank you also for taking a bigger part of the work of this repo. I hope to remove some of my responsibilities in the next few months so I can give some TLC back here (and maybe also get 1-2 people from Wix to work here some more)