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.

Sign up for GitHub

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

Jump to bottom

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Merged
merged 9 commits into from
Jun 20, 2018
Merged

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
564e1bc
move scrooge srcjar to srcs
johnynek Jun 15, 2018
ad920a0
require JavaInfo, add a test to avoid thrift jars on classpath
johnynek Jun 20, 2018
8d63397
fix strict build, run lint fix
johnynek Jun 20, 2018
90dcb05
address review comments
johnynek Jun 20, 2018
d1a3d5e
Merge branch 'master' into oscar/put_scrooge_into_src
johnynek Jun 20, 2018
4dc2edd
merge master, fix tests
johnynek Jun 20, 2018
2a78628
use JavaInfo directly
johnynek Jun 20, 2018
eb50d82
fix tests, take N
johnynek Jun 20, 2018
026fdb7
fix take N+1, update README, make runtime_deps strict
johnynek Jun 20, 2018
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
6 changes: 6 additions & 0 deletions scala/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
)
30 changes: 10 additions & 20 deletions scala/private/common.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Why keep the “if”? I think you can require the divider on the signature of the attribute and so throw all of the conditionals away

Copy link
Member Author

Choose a reason for hiding this comment

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

because scrooge puts everything in the deps sadly... we want to fix it, but one step at a time.

Copy link
Member Author

Choose a reason for hiding this comment

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

scrooge macro puts all the thrift dependencies on the deps. :( we need #510

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),
Expand All @@ -49,26 +46,19 @@ 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:
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:
Copy link
Member

Choose a reason for hiding this comment

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

Again the whole conditional as above but even if I’m wrong why silently ignore and not fail?

# 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)
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),
Expand Down
8 changes: 4 additions & 4 deletions scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -562,7 +560,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),
Copy link
Member

Choose a reason for hiding this comment

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

I’m sorry but I don’t understand this. Where does scroog euse this?

Copy link
Member Author

Choose a reason for hiding this comment

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

search for all the extra information in the file. For instance: https://github.com/bazelbuild/rules_scala/blob/master/twitter_scrooge/twitter_scrooge.bzl#L85

Really this is something that should be replaced with #510 but that is staged behind a few layers of tech debt removal (a supported skylark function to compile scala and get a JavaInfo, and a toolchain that sets up all the things we need for scala so we don't have to copy-paste the scala setup for the ctx).

jars_to_labels = jars.jars2labels,
)

Expand Down
10 changes: 4 additions & 6 deletions scala/scala.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,13 +219,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")),
}
Expand Down Expand Up @@ -476,7 +473,8 @@ _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)
Expand Down
2 changes: 1 addition & 1 deletion src/java/io/bazel/rulesscala/scala_test/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
java_binary(
java_library(
name = "runner",
srcs = ["Runner.java"],
visibility = ["//visibility:public"],
Expand Down
2 changes: 1 addition & 1 deletion test/src/main/scala/scala/test/sources_jars_in_deps/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
10 changes: 10 additions & 0 deletions test/src/main/scala/scala/test/twitter_scrooge/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
)
13 changes: 13 additions & 0 deletions test/src/main/scala/scala/test/twitter_scrooge/grep.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
#!/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"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add what was expected? Either in the assertion or in the test name since 6 months from now I won’t remember :)

exit 1
else
exit 0
fi
5 changes: 2 additions & 3 deletions twitter_scrooge/twitter_scrooge.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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],
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? i think the scala_library rule is restricted to only allow actual files in the srcs not targets?

Copy link
Contributor

Choose a reason for hiding this comment

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

(or does it intelligently realize the output file?... interesting)

Copy link
Member Author

Choose a reason for hiding this comment

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

it does use the output file here. We could be more precise and just list the output filename. That might actually solve the problem.... Maybe by putting the whole target we are sucking up all the deps.

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 + [
Expand Down