From 564e1bcffd0cdd314fda23c1327a8054d05d8e34 Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Fri, 15 Jun 2018 10:19:10 -1000 Subject: [PATCH 1/8] move scrooge srcjar to srcs --- scala/private/rule_impls.bzl | 4 +++- twitter_scrooge/twitter_scrooge.bzl | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/scala/private/rule_impls.bzl b/scala/private/rule_impls.bzl index 81e823e5b..1b92c5350 100644 --- a/scala/private/rule_impls.bzl +++ b/scala/private/rule_impls.bzl @@ -562,7 +562,9 @@ def _lib(ctx, non_macro_lib): # this information through. extra_information allows passing # this information through, and it is up to the new_targets # to filter and make sense of this information. - extra_information=_collect_extra_information(ctx.attr.deps), + # unfortunately, we need to see this for scrooge and protobuf to work, + # but those are generating srcjar, so they should really come in via srcs + extra_information=_collect_extra_information(ctx.attr.deps + ctx.attr.srcs), jars_to_labels = jars.jars2labels, ) diff --git a/twitter_scrooge/twitter_scrooge.bzl b/twitter_scrooge/twitter_scrooge.bzl index 6dda175d4..1b6904726 100644 --- a/twitter_scrooge/twitter_scrooge.bzl +++ b/twitter_scrooge/twitter_scrooge.bzl @@ -289,12 +289,11 @@ def scrooge_scala_library(name, with_finagle = with_finagle, ) - # deps from macro invocation would come via srcjar - # however, retained to make dependency analysis via aspects easier scala_library( name = name, + srcs = [srcjar], deps = deps + remote_jars + [ - srcjar, "//external:io_bazel_rules_scala/dependency/thrift/libthrift", + "//external:io_bazel_rules_scala/dependency/thrift/libthrift", "//external:io_bazel_rules_scala/dependency/thrift/scrooge_core" ], exports = deps + remote_jars + [ From ad920a07f895b7d0616fe7476f727580d0c9ec07 Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Tue, 19 Jun 2018 16:19:35 -1000 Subject: [PATCH 2/8] require JavaInfo, add a test to avoid thrift jars on classpath --- scala/BUILD | 6 ++++++ scala/private/common.bzl | 15 ++++----------- scala/private/rule_impls.bzl | 4 +--- scala/scala.bzl | 14 ++++++++------ src/java/io/bazel/rulesscala/scala_test/BUILD | 2 +- .../scala/scala/test/sources_jars_in_deps/BUILD | 2 +- .../main/scala/scala/test/twitter_scrooge/BUILD | 10 ++++++++++ .../main/scala/scala/test/twitter_scrooge/grep.sh | 8 ++++++++ 8 files changed, 39 insertions(+), 22 deletions(-) create mode 100755 test/src/main/scala/scala/test/twitter_scrooge/grep.sh diff --git a/scala/BUILD b/scala/BUILD index 06dbacbbf..b2eefcd35 100644 --- a/scala/BUILD +++ b/scala/BUILD @@ -17,3 +17,9 @@ toolchain( toolchain_type = "@io_bazel_rules_scala//scala:toolchain_type", visibility = ["//visibility:public"], ) + +java_import( + name = "bazel_test_runner_deploy", + jars = ["@bazel_tools//tools/jdk:TestRunner_deploy.jar"], + visibility = ["//visibility:public"], +) diff --git a/scala/private/common.bzl b/scala/private/common.bzl index c8049cb19..9ce9288fa 100644 --- a/scala/private/common.bzl +++ b/scala/private/common.bzl @@ -26,15 +26,12 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets): runtime_jars = [] 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] compile_jars.append(java_provider.compile_jars) runtime_jars.append(java_provider.transitive_runtime_jars) - else: - # support http_file pointed at a jar. http_jar uses ijar, - # which breaks scala macros - compile_jars.append(filter_not_sources(dep_target.files)) - runtime_jars.append(filter_not_sources(dep_target.files)) return struct( compile_jars = depset(transitive = compile_jars), @@ -52,17 +49,13 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets): current_dep_compile_jars = None current_dep_transitive_compile_jars = None + # 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] 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) - else: - # support http_file pointed at a jar. http_jar uses ijar, - # which breaks scala macros - current_dep_compile_jars = filter_not_sources(dep_target.files) - current_dep_transitive_compile_jars = filter_not_sources(dep_target.files) - runtime_jars.append(filter_not_sources(dep_target.files)) compile_jars.append(current_dep_compile_jars) transitive_compile_jars.append(current_dep_transitive_compile_jars) diff --git a/scala/private/rule_impls.bzl b/scala/private/rule_impls.bzl index 1b92c5350..3eb030090 100644 --- a/scala/private/rule_impls.bzl +++ b/scala/private/rule_impls.bzl @@ -460,9 +460,7 @@ def _collect_runtime_jars(dep_targets): if JavaInfo in dep_target: runtime_jars.append(dep_target[JavaInfo].transitive_runtime_jars) else: - # support http_file pointed at a jar. http_jar uses ijar, - # which breaks scala macros - runtime_jars.append(filter_not_sources(dep_target.files)) + fail("we expect JavaInfo for runtime jars: " + str(dep_target)) return runtime_jars diff --git a/scala/scala.bzl b/scala/scala.bzl index 2d99e1351..d6fa39382 100644 --- a/scala/scala.bzl +++ b/scala/scala.bzl @@ -12,6 +12,11 @@ load( "@io_bazel_rules_scala//specs2:specs2_junit.bzl", _specs2_junit_dependencies = "specs2_junit_dependencies") +load( + "@io_bazel_rules_scala//scala:scala_import.bzl", + _scala_import = "scala_import" +) + _launcher_template = { "_java_stub_template": attr.label( default = Label("@java_stub_template//file")), @@ -219,13 +224,10 @@ _scala_test_attrs = { "full_stacktraces": attr.bool(default = True), "_scalatest": attr.label( default = Label( - "//external:io_bazel_rules_scala/dependency/scalatest/scalatest"), - allow_files = True), + "//external:io_bazel_rules_scala/dependency/scalatest/scalatest")), "_scalatest_runner": attr.label( - executable = True, cfg = "host", - default = Label("//src/java/io/bazel/rulesscala/scala_test:runner.jar"), - allow_files = True), + default = Label("//src/java/io/bazel/rulesscala/scala_test:runner")), "_scalatest_reporter": attr.label( default = Label("//scala/support:test_reporter")), } @@ -476,7 +478,7 @@ _scala_junit_test_attrs = { "//external:io_bazel_rules_scala/dependency/hamcrest/hamcrest_core") ), "_bazel_test_runner": attr.label( - default = Label("@bazel_tools//tools/jdk:TestRunner_deploy.jar"), + default = Label("@io_bazel_rules_scala//scala:bazel_test_runner_deploy"), allow_files = True), } _scala_junit_test_attrs.update(_launcher_template) diff --git a/src/java/io/bazel/rulesscala/scala_test/BUILD b/src/java/io/bazel/rulesscala/scala_test/BUILD index 4520a832e..34c233909 100644 --- a/src/java/io/bazel/rulesscala/scala_test/BUILD +++ b/src/java/io/bazel/rulesscala/scala_test/BUILD @@ -1,4 +1,4 @@ -java_binary( +java_library( name = "runner", srcs = ["Runner.java"], visibility = ["//visibility:public"], diff --git a/test/src/main/scala/scala/test/sources_jars_in_deps/BUILD b/test/src/main/scala/scala/test/sources_jars_in_deps/BUILD index 29f73c471..8a2d0d7af 100644 --- a/test/src/main/scala/scala/test/sources_jars_in_deps/BUILD +++ b/test/src/main/scala/scala/test/sources_jars_in_deps/BUILD @@ -4,6 +4,6 @@ scala_library( name = "source_jar_not_oncp", srcs = ["ReferCatsImplicits.scala"], deps = [ - "@org_typelevel__cats_core//jar:file", + "@org_typelevel__cats_core//jar", ], ) diff --git a/test/src/main/scala/scala/test/twitter_scrooge/BUILD b/test/src/main/scala/scala/test/twitter_scrooge/BUILD index 17e9ecd44..acf39c6f5 100644 --- a/test/src/main/scala/scala/test/twitter_scrooge/BUILD +++ b/test/src/main/scala/scala/test/twitter_scrooge/BUILD @@ -190,3 +190,13 @@ scala_binary( main_class = "scala.test.twitter_scrooge.BareThrifts", deps = [":bare_thrift_scrooge"], ) + +sh_test( + name = "libthrift2_a_not_on_classpath", + srcs = ["grep.sh"], + args = [ + "libthrift2_a", + "$(location :justscrooges)", + ], + data = [":justscrooges"], +) diff --git a/test/src/main/scala/scala/test/twitter_scrooge/grep.sh b/test/src/main/scala/scala/test/twitter_scrooge/grep.sh new file mode 100755 index 000000000..6d989f2f9 --- /dev/null +++ b/test/src/main/scala/scala/test/twitter_scrooge/grep.sh @@ -0,0 +1,8 @@ +#!/bin/sh + +if grep -q $1 $2 ; then + echo "ERROR: found $1" + exit 1 +else + exit 0 +fi From 8d63397342323381b1cd4458ef4206b34a104a8a Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Tue, 19 Jun 2018 16:55:30 -1000 Subject: [PATCH 3/8] fix strict build, run lint fix --- scala/private/common.bzl | 13 +++++-------- scala/scala.bzl | 6 +++--- 2 files changed, 8 insertions(+), 11 deletions(-) diff --git a/scala/private/common.bzl b/scala/private/common.bzl index 9ce9288fa..089816b23 100644 --- a/scala/private/common.bzl +++ b/scala/private/common.bzl @@ -46,9 +46,6 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets): runtime_jars = [] for dep_target in dep_targets: - current_dep_compile_jars = None - current_dep_transitive_compile_jars = None - # we require a JavaInfo for dependencies # must use java_import or scala_import if you have raw files if JavaInfo in dep_target: @@ -57,11 +54,11 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets): 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) - add_labels_of_jars_to(jars2labels, dep_target, - current_dep_transitive_compile_jars.to_list(), - current_dep_compile_jars.to_list()) + compile_jars.append(current_dep_compile_jars) + transitive_compile_jars.append(current_dep_transitive_compile_jars) + add_labels_of_jars_to(jars2labels, dep_target, + current_dep_transitive_compile_jars.to_list(), + current_dep_compile_jars.to_list()) return struct( compile_jars = depset(transitive = compile_jars), diff --git a/scala/scala.bzl b/scala/scala.bzl index d6fa39382..5acc02adf 100644 --- a/scala/scala.bzl +++ b/scala/scala.bzl @@ -14,8 +14,7 @@ load( load( "@io_bazel_rules_scala//scala:scala_import.bzl", - _scala_import = "scala_import" -) + _scala_import = "scala_import") _launcher_template = { "_java_stub_template": attr.label( @@ -478,7 +477,8 @@ _scala_junit_test_attrs = { "//external:io_bazel_rules_scala/dependency/hamcrest/hamcrest_core") ), "_bazel_test_runner": attr.label( - default = Label("@io_bazel_rules_scala//scala:bazel_test_runner_deploy"), + default = Label( + "@io_bazel_rules_scala//scala:bazel_test_runner_deploy"), allow_files = True), } _scala_junit_test_attrs.update(_launcher_template) From 90dcb05ecc53d8db6df36c90a5729785c2e00b73 Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Tue, 19 Jun 2018 17:11:28 -1000 Subject: [PATCH 4/8] address review comments --- scala/scala.bzl | 4 ---- test/src/main/scala/scala/test/twitter_scrooge/grep.sh | 5 +++++ 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/scala/scala.bzl b/scala/scala.bzl index 5acc02adf..8c5e61620 100644 --- a/scala/scala.bzl +++ b/scala/scala.bzl @@ -12,10 +12,6 @@ load( "@io_bazel_rules_scala//specs2:specs2_junit.bzl", _specs2_junit_dependencies = "specs2_junit_dependencies") -load( - "@io_bazel_rules_scala//scala:scala_import.bzl", - _scala_import = "scala_import") - _launcher_template = { "_java_stub_template": attr.label( default = Label("@java_stub_template//file")), diff --git a/test/src/main/scala/scala/test/twitter_scrooge/grep.sh b/test/src/main/scala/scala/test/twitter_scrooge/grep.sh index 6d989f2f9..880b6e840 100755 --- a/test/src/main/scala/scala/test/twitter_scrooge/grep.sh +++ b/test/src/main/scala/scala/test/twitter_scrooge/grep.sh @@ -1,5 +1,10 @@ #!/bin/sh + +# this is used by a sh_test to test that a string is NOT +# found in a file. This is used for instance to check if +# stray jars are not making it onto the classpath + if grep -q $1 $2 ; then echo "ERROR: found $1" exit 1 From 4dc2edd4853e10552926da2699363e3d9ac9c7eb Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Wed, 20 Jun 2018 08:59:36 -1000 Subject: [PATCH 5/8] merge master, fix tests --- .../scala/{scala => scalarules}/test/twitter_scrooge/grep.sh | 0 test_rules_scala.sh | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename test/src/main/scala/{scala => scalarules}/test/twitter_scrooge/grep.sh (100%) diff --git a/test/src/main/scala/scala/test/twitter_scrooge/grep.sh b/test/src/main/scala/scalarules/test/twitter_scrooge/grep.sh similarity index 100% rename from test/src/main/scala/scala/test/twitter_scrooge/grep.sh rename to test/src/main/scala/scalarules/test/twitter_scrooge/grep.sh diff --git a/test_rules_scala.sh b/test_rules_scala.sh index e2507fdd6..02554b5c8 100755 --- a/test_rules_scala.sh +++ b/test_rules_scala.sh @@ -159,7 +159,7 @@ test_scala_library_expect_failure_on_missing_direct_external_deps_jar() { } test_scala_library_expect_failure_on_missing_direct_external_deps_file_group() { - dependenecy_target='@com_google_guava_guava_21_0_with_file//jar:file' + dependenecy_target='@com_google_guava_guava_21_0_with_file//jar' test_target='test_expect_failure/missing_direct_deps/external_deps:transitive_external_dependency_user_file_group' test_scala_library_expect_failure_on_missing_direct_deps $dependenecy_target $test_target From 2a7862824fa5cd338672369c47242b56975ed3b6 Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Wed, 20 Jun 2018 09:04:30 -1000 Subject: [PATCH 6/8] use JavaInfo directly --- .../missing_direct_deps/internal_deps/custom-jvm-rule.bzl | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test_expect_failure/missing_direct_deps/internal_deps/custom-jvm-rule.bzl b/test_expect_failure/missing_direct_deps/internal_deps/custom-jvm-rule.bzl index d20515454..98fe7bdef 100644 --- a/test_expect_failure/missing_direct_deps/internal_deps/custom-jvm-rule.bzl +++ b/test_expect_failure/missing_direct_deps/internal_deps/custom-jvm-rule.bzl @@ -12,8 +12,7 @@ def _custom_jvm_impl(ctx): def _collect(deps): transitive_compile_jars = depset() for dep_target in deps: - transitive_compile_jars += dep_target[ - java_common.provider].transitive_compile_time_jars + transitive_compile_jars += dep_target[JavaInfo].transitive_compile_time_jars return transitive_compile_jars custom_jvm = rule( From eb50d82305f1ec096861c9e5d585fb26e08572ae Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Wed, 20 Jun 2018 10:22:59 -1000 Subject: [PATCH 7/8] fix tests, take N --- scala/private/common.bzl | 4 ++++ test_expect_failure/missing_direct_deps/external_deps/BUILD | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/scala/private/common.bzl b/scala/private/common.bzl index 089816b23..a97765579 100644 --- a/scala/private/common.bzl +++ b/scala/private/common.bzl @@ -32,6 +32,8 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets): java_provider = dep_target[JavaInfo] compile_jars.append(java_provider.compile_jars) runtime_jars.append(java_provider.transitive_runtime_jars) + else: + print("ignored dependency, has no JavaInfo: " + str(dep_target)) return struct( compile_jars = depset(transitive = compile_jars), @@ -59,6 +61,8 @@ def _collect_jars_when_dependency_analyzer_is_on(dep_targets): add_labels_of_jars_to(jars2labels, dep_target, current_dep_transitive_compile_jars.to_list(), current_dep_compile_jars.to_list()) + else: + print("ignored dependency, has no JavaInfo: " + str(dep_target)) return struct( compile_jars = depset(transitive = compile_jars), diff --git a/test_expect_failure/missing_direct_deps/external_deps/BUILD b/test_expect_failure/missing_direct_deps/external_deps/BUILD index 363e40774..9585f74f2 100644 --- a/test_expect_failure/missing_direct_deps/external_deps/BUILD +++ b/test_expect_failure/missing_direct_deps/external_deps/BUILD @@ -31,5 +31,5 @@ scala_library( srcs = [ "B.scala", ], - deps = ["@com_google_guava_guava_21_0_with_file//jar:file"], + deps = ["@com_google_guava_guava_21_0_with_file//jar"], ) From 026fdb78f1d32550ccb4de587a32d3eef18cc43a Mon Sep 17 00:00:00 2001 From: Oscar Boykin Date: Wed, 20 Jun 2018 11:39:55 -1000 Subject: [PATCH 8/8] fix take N+1, update README, make runtime_deps strict --- README.md | 17 +++++++++++------ scala/private/rule_impls.bzl | 5 +---- scala/scala.bzl | 2 +- test_rules_scala.sh | 2 +- 4 files changed, 14 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 799d44a84..76b27af75 100644 --- a/README.md +++ b/README.md @@ -104,21 +104,23 @@ In order to make a java rule use this jar file, use the `java_import` rule.

List of labels, required

List of Scala .scala source files used to build the - library

+ library. These may be .srcjar jar files that contain source code.

deps

List of labels, optional

-

List of other libraries to linked to this library target

+

List of other libraries to linked to this library target. + These must be jvm targets (scala_library, java_library, java_import, etc...)

runtime_deps

List of labels, optional

-

List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.

+

List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala. + These must be jvm targets (scala_library, java_library, java_import, etc...)

@@ -127,7 +129,8 @@ In order to make a java rule use this jar file, use the `java_import` rule.

List of labels, optional

List of targets to add to the dependencies of those that depend on this target. Similar to the `java_library` parameter of the same name. Use this sparingly as it weakens the - precision of the build graph.

+ precision of the build graph. + These must be jvm targets (scala_library, java_library, java_import, etc...)

@@ -260,14 +263,16 @@ A `scala_binary` requires a `main_class` attribute. deps

List of labels, optional

-

List of other libraries to linked to this binary target

+

List of other libraries to linked to this binary target. + These must be jvm targets (scala_library, java_library, java_import, etc...)

runtime_deps

List of labels, optional

-

List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala.

+

List of other libraries to put on the classpath only at runtime. This is rarely needed in Scala. + These must be jvm targets (scala_library, java_library, java_import, etc...)

diff --git a/scala/private/rule_impls.bzl b/scala/private/rule_impls.bzl index 3eb030090..4163f6c76 100644 --- a/scala/private/rule_impls.bzl +++ b/scala/private/rule_impls.bzl @@ -457,10 +457,7 @@ def _collect_runtime_jars(dep_targets): runtime_jars = [] for dep_target in dep_targets: - if JavaInfo in dep_target: - runtime_jars.append(dep_target[JavaInfo].transitive_runtime_jars) - else: - fail("we expect JavaInfo for runtime jars: " + str(dep_target)) + runtime_jars.append(dep_target[JavaInfo].transitive_runtime_jars) return runtime_jars diff --git a/scala/scala.bzl b/scala/scala.bzl index 8c5e61620..dd5873dd0 100644 --- a/scala/scala.bzl +++ b/scala/scala.bzl @@ -102,7 +102,7 @@ _common_attrs_for_plugin_bootstrapping = { "srcs": attr.label_list(allow_files = [".scala", ".srcjar", ".java"]), "deps": attr.label_list(), "plugins": attr.label_list(allow_files = [".jar"]), - "runtime_deps": attr.label_list(), + "runtime_deps": attr.label_list(providers = [[JavaInfo]]), "data": attr.label_list(allow_files = True, cfg = "data"), "resources": attr.label_list(allow_files = True), "resource_strip_prefix": attr.string(), diff --git a/test_rules_scala.sh b/test_rules_scala.sh index 02554b5c8..f1ff88e28 100755 --- a/test_rules_scala.sh +++ b/test_rules_scala.sh @@ -159,7 +159,7 @@ test_scala_library_expect_failure_on_missing_direct_external_deps_jar() { } test_scala_library_expect_failure_on_missing_direct_external_deps_file_group() { - dependenecy_target='@com_google_guava_guava_21_0_with_file//jar' + dependenecy_target='@com_google_guava_guava_21_0_with_file//jar:jar' test_target='test_expect_failure/missing_direct_deps/external_deps:transitive_external_dependency_user_file_group' test_scala_library_expect_failure_on_missing_direct_deps $dependenecy_target $test_target