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

Fix a regression that breaks scalapb when jvm_flags are set on the toolchain #806

Merged
merged 2 commits into from
Aug 8, 2019
+145 −37
Merged

Fix a regression that breaks scalapb when jvm_flags are set on the toolchain #806

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38bffc8
Fix a regression that breaks scalapb when jvm_flags are set on the to…
beala-stripe Aug 7, 2019
4b4913b
Move passing manual tests to new directory
beala-stripe Aug 7, 2019
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: 5 additions & 1 deletion scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,11 @@ touch {statsfile}
)

def _expand_location(ctx, flags):
return [ctx.expand_location(f, ctx.attr.data) for f in flags]
if hasattr(ctx.attr, "data"):
data = ctx.attr.data
else:
data = []
return [ctx.expand_location(f, data) for f in flags]

def _join_path(args, sep = ","):
return sep.join([f.path for f in args])
Expand Down
18 changes: 18 additions & 0 deletions test_expect_failure/scalac_jvm_opts/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
load("//scala:scala_toolchain.bzl", "scala_toolchain")
load("//scala:scala.bzl", "scala_library")
load(
"//scala_proto:scala_proto.bzl",
"scalapb_proto_library",
)

scala_toolchain(
name = "failing_toolchain_impl",
Expand Down Expand Up @@ -41,3 +45,17 @@ scala_library(
# the `failing_scala_toolchain` is used.
scalac_jvm_flags = ["-Xmx1G"],
)

proto_library(
Copy link
Member

Choose a reason for hiding this comment

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

why is this in expect_failure We expect this to work don't we?

We should only put tests in here that won't build or won't pass with bazel test //...

This seems like it is a positive, not a negative test. It should go in test/...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can move it there. I noticed some tests that require extra arguments (eg, bazel build --extra_toolchains ...) were being put in expect_failure because targets in test/ are all invoked with bazel build //test/... and there's no way to specify extra arguments.

Maybe we need another test directory called test_manual or something for this use case?

Copy link
Member

Choose a reason for hiding this comment

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

yeah, I'd rather have a separate directory. This should be for tests or builds that we are expecting to fail. If we've accidentally allowed others to get in here, we should address that (but not necessarily in this PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. I've moved the passing manual tests to a manual_test directory.

name = "test",
srcs = ["test.proto"],
visibility = ["//visibility:public"],
)

# This is a regression test for a bug that broke compiling scalapb targets when
# `scalac_jvm_flags` was set on the toolchain.
scalapb_proto_library(
name = "proto",
visibility = ["//visibility:public"],
deps = [":test"],
)
8 changes: 8 additions & 0 deletions test_expect_failure/scalac_jvm_opts/test.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
syntax = "proto2";

option java_package = "test.proto";

message TestResponse1 {
optional uint32 c = 1;
optional bool d = 2;
}
5 changes: 5 additions & 0 deletions test_rules_scala.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -837,6 +837,10 @@ test_scalac_jvm_flags_on_target_overrides_toolchain_passes() {
bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:failing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:empty_overriding_build
}

test_scalac_jvm_flags_work_with_scalapb() {
bazel build --extra_toolchains="//test_expect_failure/scalac_jvm_opts:passing_scala_toolchain" //test_expect_failure/scalac_jvm_opts:proto
}

test_scala_test_jvm_flags_from_scala_toolchain_fails() {
action_should_fail test --extra_toolchains="//test_expect_failure/scala_test_jvm_flags:failing_scala_toolchain" //test_expect_failure/scala_test_jvm_flags:empty_test
}
Expand Down Expand Up @@ -1131,6 +1135,7 @@ $runner scala_pb_library_targets_do_not_have_host_deps
$runner test_scalac_jvm_flags_on_target_overrides_toolchain_passes
$runner test_scalac_jvm_flags_from_scala_toolchain_passes
$runner test_scalac_jvm_flags_from_scala_toolchain_fails
$runner test_scalac_jvm_flags_work_with_scalapb
$runner test_scala_test_jvm_flags_on_target_overrides_toolchain_passes
$runner test_scala_test_jvm_flags_from_scala_toolchain_passes
$runner test_scala_test_jvm_flags_from_scala_toolchain_fails