Skip to content

Commit

Permalink
Add more tests which use ast_plus_one_deps_strict_deps_unused_deps_er…
Browse files Browse the repository at this point in the history 
…ror toolchain (#1030)

* move ast_plus_one_deps_strict_deps_unused_deps_error to //scala package
also mention it in readme

* add e2e tests to use ast_plus_one_deps_strict_deps_unused_deps_error

* fix lint

* rename ast_plus_one_deps_strict_deps_unused_deps_error to minimal_direct_source_deps
  • Loading branch information
@ittaiz
ittaiz authored Apr 13, 2020
1 parent 30b4529 commit c567dab
Show file tree
Hide file tree
Showing 9 changed files with 30 additions and 18 deletions.
2 changes: 2 additions & 0 deletions README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,8 @@ With these settings, we also will error on dependencies which are unneeded, and
The dependency tracking method `ast` is experimental but so far proves to be better than the default for computing the direct dependencies for `plus-one` mode code. In the future we hope to make this the default for `plus-one` mode and remove the option altogether.
To try it out you can use the following toolchain: `//scala:minimal_direct_source_deps`.
### [Experimental] Dependency mode
There are three dependency modes. The reason for the multiple modes is that often `scalac` depends on jars which seem unnecessary at first glance. Hence, in order to reduce the need to please `scalac`, we provide the following options.
Expand Down
16 changes: 16 additions & 0 deletions scala/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,22 @@ toolchain(
visibility = ["//visibility:public"],
)

scala_toolchain(
name = "ast_plus_one_deps_strict_deps_unused_deps_error_impl",
dependency_mode = "plus-one",
dependency_tracking_method = "ast",
strict_deps_mode = "error",
unused_dependency_checker_mode = "error",
visibility = ["//visibility:public"],
)

toolchain(
name = "minimal_direct_source_deps",
toolchain = "ast_plus_one_deps_strict_deps_unused_deps_error_impl",
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
visibility = ["//visibility:public"],
)

scala_toolchain(
name = "code_coverage_toolchain_impl",
enable_code_coverage_aspect = "on",
Expand Down
3 changes: 3 additions & 0 deletions src/java/io/bazel/rulesscala/specs2/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ scala_library(
"Specs2RunnerBuilder.scala",
"package.scala",
],
unused_dependency_checker_ignored_targets = [
"//external:io_bazel_rules_scala/dependency/scala/scala_xml",
],
visibility = ["//visibility:public"],
deps = [
"//external:io_bazel_rules_scala/dependency/junit/junit",
Expand Down
3 changes: 3 additions & 0 deletions src/scala/scripts/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ scala_library(
scala_library(
name = "scalapb_generator_lib",
srcs = ["ScalaPBGenerator.scala"],
unused_dependency_checker_ignored_targets = [

This comment has been minimized.

Sign in to view

Copy link
@Jamie5

Jamie5 Apr 15, 2020

Contributor

@ittaiz here is what I think is happening

So when using plus one mode, then removing this dependency from deps works. But when using direct mode, it gives a compile error that something is missing. So +1 must be bringing in some dependency.

I suspect this is via the //src/java/io/bazel/rulesscala/worker which then depends on //third_party/bazel/src/main/protobuf:worker_protocol_java_proto but am not 100% sure. (mainly I think this because I couldn't find any other deps of deps)

I could not find a reference in the source code to com.google stuff which makes me suspect this is a case where we correctly identify the dep as unused in source code, and that when using +1 things just work (-ish, because not sure that we are getting the dependency correctly).

Am not fully sure why scalac needs the dependency but possibly it is the mention of ScalaPbCodeGenerator in ScalaPBGenerator.scala, which then here https://github.com/scalapb/ScalaPB/blob/master/compiler-plugin/src/main/scala/scalapb/ScalaPbCodeGenerator.scala has references com.google.protobuf.ExtensionRegistry in the interface, which would be provided by //external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java (is my guess).

So maybe the "//external:io_bazel_rules_scala/dependency/proto/scalapb_plugin" (if that is the right dep which gives ScalaPbCodeGenerator) should declare a deps on //external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java.

Though that doesn't solve the problem which is that when using direct we need the dep but when using +1 we don't need it and it is unused.

This comment has been minimized.

Sign in to view

Copy link
@ittaiz

ittaiz Apr 17, 2020

Author Member

Your analysis sounds relevant (haven't validated but it sounds about right).
My question to you- isn't this a good example to our discussion over at #867 ? Where we need to identify that something is needed transitively? Can you maybe somehow identify in ast mode that scalac needs it and not the source?
I'm waving my hands but I have a feeling you'll catch my drift

This comment has been minimized.

Sign in to view

Copy link
@Jamie5

Jamie5 Apr 17, 2020

Contributor

Added a mention to #867 (comment) . Though this isn't quite a "need" in this case because +1 mode already brings in the dependency.

This comment has been minimized.

Sign in to view

Copy link
@ittaiz

ittaiz Apr 17, 2020

Author Member

that's right but couldn't it happen also with another "hop"?

This comment has been minimized.

Sign in to view

Copy link
@Jamie5

Jamie5 Apr 17, 2020

Contributor

Potentially yes would need to look at the situation with more detail/etc. Just saying that this current specific situation isn't an example of +1 failing.
"//external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java",
],
visibility = ["//visibility:public"],
runtime_deps = [
"//external:io_bazel_rules_scala/dependency/com_google_protobuf/protobuf_java",
Expand Down
2 changes: 1 addition & 1 deletion test/shell/test_deps.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ test_plus_one_ast_analyzer_strict_deps() {
expected_message_error="error: Target '$dependenecy_target' is used but isn't explicitly declared, please add it to the deps"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message_error}" ${test_target} "--extra_toolchains=//test/toolchains:ast_plus_one_deps_strict_deps_error" "eq"
test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message_error}" ${test_target} "--extra_toolchains=//test/toolchains:ast_plus_one_deps_strict_deps_unused_deps_error" "eq"
test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message_error}" ${test_target} "--extra_toolchains=//scala:minimal_direct_source_deps" "eq"
test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message_warn}" ${test_target} "--extra_toolchains=//test/toolchains:ast_plus_one_deps_strict_deps_warn" "ne"
}

Expand Down
2 changes: 1 addition & 1 deletion test/shell/test_unused_dependency.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ test_plus_one_ast_analyzer_unused_deps_error() {
}

test_plus_one_ast_analyzer_unused_deps_strict_deps_error() {
action_should_fail build --extra_toolchains="//test/toolchains:ast_plus_one_deps_strict_deps_unused_deps_error" //test_expect_failure/plus_one_deps/with_unused_deps:a
action_should_fail build --extra_toolchains="//scala:minimal_direct_source_deps" //test_expect_failure/plus_one_deps/with_unused_deps:a
}

test_plus_one_ast_analyzer_unused_deps_warn() {
Expand Down
2 changes: 2 additions & 0 deletions test/src/main/scala/scalarules/test/srcjars_with_java/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ scala_library(
scala_library(
name = "mixed_language_dependent",
srcs = ["MixedLanguageDependent.scala"],
unused_dependency_checker_ignored_targets = [":mixed_language_source_jar"],
deps = [":mixed_language_source_jar"],
)

Expand All @@ -23,5 +24,6 @@ scala_library(
scala_library(
name = "java_dependent",
srcs = ["JavaDependent.scala"],
unused_dependency_checker_ignored_targets = [":java_source_jar"],
deps = [":java_source_jar"],
)
16 changes: 0 additions & 16 deletions test/toolchains/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,19 +59,3 @@ toolchain(
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
visibility = ["//visibility:public"],
)

scala_toolchain(
name = "ast_plus_one_deps_strict_deps_unused_deps_error_impl",
dependency_mode = "plus-one",
dependency_tracking_method = "ast",
strict_deps_mode = "error",
unused_dependency_checker_mode = "error",
visibility = ["//visibility:public"],
)

toolchain(
name = "ast_plus_one_deps_strict_deps_unused_deps_error",
toolchain = "ast_plus_one_deps_strict_deps_unused_deps_error_impl",
toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type",
visibility = ["//visibility:public"],
)
2 changes: 2 additions & 0 deletions test_rules_scala.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@ $runner bazel test test/...
$runner bazel test third_party/...
# UnusedDependencyChecker doesn't work with strict_java_deps
$runner bazel build "--strict_java_deps=ERROR -- test/... -test:UnusedDependencyChecker"
$runner bazel build "--extra_toolchains=//scala:minimal_direct_source_deps -- test/... -test:UnusedDependencyChecker"
#$runner bazel build "--strict_java_deps=ERROR --all_incompatible_changes -- test/... -test:UnusedDependencyChecker"
$runner bazel test "--strict_java_deps=ERROR -- test/... -test:UnusedDependencyChecker"
$runner bazel test "--extra_toolchains=//scala:minimal_direct_source_deps -- test/... -test:UnusedDependencyChecker"
$runner bazel build "test_expect_failure/missing_direct_deps/internal_deps/... --strict_java_deps=warn"
$runner bazel build //test_expect_failure/proto_source_root/... --strict_proto_deps=off
$runner bazel test //test/... --extra_toolchains="//test_expect_failure/plus_one_deps:plus_one_deps"
Expand Down

0 comments on commit c567dab

Please sign in to comment.