Use java_common.run_ijar #538
Conversation
|
looks like run_ijar was added in 0.14.1. |
scala/private/rule_impls.bzl
Outdated
|
|
||
| ijar = None | ||
| # compile the java now |
There was a problem hiding this comment.
this comment from the diff looks to be around the ijar rather than the java?
There was a problem hiding this comment.
well, yeah, it is before the ijar which is before java. Due to CI slowness, I'll pick this up in the next PR (the merge of #524 with this).
|
small nit on the comment, otherwise lgtm |
| test_target='//test_expect_failure/missing_direct_deps/internal_deps:transitive_dependency_java_user' | ||
|
|
||
| expected_message="$dependency_file.*$test_target" |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
here is the failure though: https://travis-ci.org/bazelbuild/rules_scala/jobs/397490178#L919
There was a problem hiding this comment.
can you explain why you shouldn't have the ijar?
Re approach:
- 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.
- 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.
- We want to align ourselves with the mainstream approach and I've been working with Google people about a shared design for this
There was a problem hiding this comment.
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
There was a problem hiding this comment.
okay, I see how I goofed. I think this is better. What do you think of the current change?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@or-shachar @natansil fyi re changes to our fix strict deps script |
|
Given the assertion looks to be @ittaiz one, lgtm |
here is a supported function to run ijar:
https://docs.bazel.build/versions/master/skylark/lib/java_common.html#run_ijar
I don't know at the moment when it was introduced because bazel docs are sadly unversioned. It does allow us to delete a fair amount of code, so I think it is worth it.
cc @ittaiz @ianoc