Skip to content

Use java_common.run_ijar #538

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

Merged
merged 6 commits into from
Jun 27, 2018
Merged

Use java_common.run_ijar #538

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ee0922a
Use java_common.run_ijar
johnynek Jun 26, 2018
fefefe3
merge with master
johnynek Jun 27, 2018
664cb76
run lint fix
johnynek Jun 27, 2018
c9a70a4
try to fix a test
johnynek Jun 27, 2018
e11597d
fix up the tests
johnynek Jun 27, 2018
adf0cd9
try one more time
johnynek Jun 27, 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
4 changes: 3 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 @@ -329,7 +329,7 @@ def _compile_or_empty(ctx, manifest, jars, srcjars, buildijar,
ctx.attr.print_compile_time, ctx.attr.expect_java_output,
ctx.attr.scalac_jvm_flags)

# compile the java now
# build ijar if needed
if buildijar:
ijar = java_common.run_ijar(
ctx.actions,
Expand All @@ -340,6 +340,8 @@ def _compile_or_empty(ctx, manifest, jars, srcjars, buildijar,
# macro code needs to be available at compile-time,
# so set ijar == jar
ijar = ctx.outputs.jar

# compile the java now
java_jar = try_to_compile_java_jar(
ctx, ijar, all_srcjars, java_srcs,
implicit_junit_deps_needed_for_java_compilation)
Expand Down
4 changes: 1 addition & 3 deletions test_rules_scala.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,9 @@ test_scala_library_expect_failure_on_missing_direct_deps_warn_mode() {

test_scala_library_expect_failure_on_missing_direct_java() {
dependency_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency'
#since bazel 0.12.0 the labels are only emmitted if ijar is in play
dependency_file='test_expect_failure/missing_direct_deps/internal_deps/transitive_dependency_ijar.jar'
test_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency_java_user'

expected_message="$dependency_file.*$test_target"
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't sound like the right approach. Can you share what the error message was when it failed? this expected message is too generic

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it no longer prints the ijar. Which you shouldn't have as far as I can tell. Users shouldn't need to worry about it.

It still fails, and if you follow the recommendation, it still gives the correct next step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PS: I'm pretty convinced we need to move the jdeps approach and away from this custom approach that runs at analysis time (vs build time).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here is the failure though: https://travis-ci.org/bazelbuild/rules_scala/jobs/397490178#L919

Copy link
Contributor

Choose a reason for hiding this comment

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

can you explain why you shouldn't have the ijar?

Re approach:

  1. As far as I understand they are trying to get rid of jdeps since the params passing is too expensive and instead just stamp each jar with a canonical label and if an infraction is spotted pay the cost of extracting the label from the jar.
  2. Our approach is very similar to the jdeps one where we only warn/error in build time (in the compiler plugin); Our difference is in how we collect the list of labels/jars to pass.
  3. We want to align ourselves with the mainstream approach and I've been working with Google people about a shared design for this

Copy link
Contributor

Choose a reason for hiding this comment

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

btw, also the test that fails now was changed in #505 and it looks like that for it too the above assertion should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I see how I goofed. I think this is better. What do you think of the current change?

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrote on the commit. I don’t think it’s enough.
IMHO the point is to show the transitive_target and the test_target since they’re both related

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I pushed one more time.

These tests don't work when run multiple times, which is annoying. It looks like you are relying on the side effect of building, which doesn't happen on the second build with a warn mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it’s annoying. There’s no way to re trigger warnings and this issue on CI only started happening with the caching which is pretty recent. The real solution is bazel integration testing since we have a reproducible sandbox for each scenario

expected_message=".*$test_target"

test_expect_failure_or_warning_on_missing_direct_deps_with_expected_message "${expected_message}" $test_target "--strict_java_deps=error"
}
Expand Down