-
Notifications
You must be signed in to change notification settings - Fork 281
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.
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
Strip -source's jars from scala's compiler classpath for dependencies #279
Strip -source's jars from scala's compiler classpath for dependencies #279
Conversation
Can one of the admins verify this patch? |
@@ -14,18 +14,25 @@ env: | |||
# we want to test the most recent few releases | |||
- V=0.5.3 | |||
- V=0.5.4 | |||
- V=0.6.0rc2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this seemed like the best way to avoid possibly regressing in future since the 0.5.x's don't have this behavior.
scala/scala.bzl
Outdated
@@ -487,7 +498,10 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets): | |||
compile_jars += dep_target.files | |||
runtime_jars += dep_target.files | |||
|
|||
return struct(compile_jars = compile_jars, transitive_runtime_jars = runtime_jars, jars2labels = {}, transitive_compile_jars = depset()) | |||
return struct(compile_jars = filter_not_sources(compile_jars), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not remove them from runtime jars as well? Essentially I think this filter should be where we do "dep.files"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or rather only in the branch of "http file" this encapsulates the "mess" better and when we'll introduce scala import we can remove it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if someone would want to do runtime reflection and access them tbh was the reason not to do it. Its mostly been accidental, and the likely hood of using/needing them at compile time seems amazingly low. But I don't mind dropping them from runtime, this just seemed smaller possible wake of unexpected behavior.
I don't think http_file branch would do it for us alone since we refer to maven_jar, which generates a build file. I'm not sure how easily we'll be able to remove it without saying that scala rules are incompatible with the native maven jar? (though this will only hit on some libraries where the sources path layout causes this path to fire too of course which is probably relatively uncommon -- this particular case cats are patching to remove).
When you use maven_jar //jar:file it translates in rules scala to the http
file branch. It was created for this purpose.
…On Mon, 18 Sep 2017 at 20:11 ianoc-stripe ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In scala/scala.bzl
<#279 (comment)>
:
> @@ -487,7 +498,10 @@ def _collect_jars_when_dependency_analyzer_is_off(dep_targets):
compile_jars += dep_target.files
runtime_jars += dep_target.files
- return struct(compile_jars = compile_jars, transitive_runtime_jars = runtime_jars, jars2labels = {}, transitive_compile_jars = depset())
+ return struct(compile_jars = filter_not_sources(compile_jars),
I don't know if someone would want to do runtime reflection and access
them tbh was the reason not to do it. Its mostly been accidental, and the
likely hood of using/needing them at compile time seems amazingly low. But
I don't mind dropping them from runtime, this just seemed smaller possible
wake of unexpected behavior.
I don't think http_file branch would do it for us alone since we refer to
maven_jar, which generates a build file. I'm not sure how easily we'll be
able to remove it without saying that scala rules are incompatible with the
native maven jar? (though this will only hit on some libraries where the
sources path layout causes this path to fire too of course which is
probably relatively uncommon -- this particular case cats are patching to
remove).
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#279 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFyew4grbMLNE9j6Jb_ZdB1E7D2DJks5sjqRCgaJpZM4PabSy>
.
|
Ahhh! didn't realize that -- i'll change this around to move the filter there, it would be much cleaner if that just works. Thanks! |
I moved it to where we do the files dance, we do it 3 places right now though. So it doesn't look much different to before. But probably a bit more tidy? |
test this please (ci bot message) |
test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! looks good. Added one small comment
scala/scala.bzl
Outdated
# import cats.implicits._ | ||
|
||
def filter_not_sources(deps): | ||
return depset([dep for dep in deps.to_list() if "-sources.jar" not in dep.basename ]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might sound picky but any chance you can factor out the if so that it's shared between the two places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah maybe:
def _is_source_jar(filename):
return "-sources.jar" in filename
then we call that twice? if not _is_source_jar(dep.basename)
?
👍 once @ittaiz is happy. |
BTW, I hope to merge one of the codepaths and just turn the strict deps on. |
Cool -- the strict deps stuff does sound nice i've not used it yet. But its very nice in the java rules |
Awesome, thanks! I'll merge once build finishes. |
test this please |
huh, bazel ci thinks this failed. |
does this pass locally for you @ianoc-stripe ? |
d510bf3
to
361f5a2
Compare
No sorry, just pushed thinking it was tiny. ofc I always forget the need to do |
test this please |
Forgetting the return happens to me every time :)
…On Mon, 18 Sep 2017 at 22:12 P. Oscar Boykin ***@***.***> wrote:
test this please
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#279 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABUIFx797TATKym85U7ZvJJXe0UPiOEBks5sjsCSgaJpZM4PabSy>
.
|
Looks like travis hasn't started ? ci from bazel looks green |
Looks like travisci is having a sad day fyi -- https://www.traviscistatus.com/incidents/4qj8t3220jy8 |
seems like this failed on |
test this please |
the bazel ci failure looks transient i think, might need to re-request the test? |
test this please |
I'm not sure there is a single point here unless i'm missing it? We refer to files in a few places to add them to different parts of the struct. I could make a function to operate on the struct and then do it further out too. The failures for both travis and bazel look transient and not code related :( |
this this please |
@damienmg looks like bazel ci is not working for us. Fails immediately with a channel closed exceptions. You probably already know, but I wanted to share. |
@ianoc-stripe I'm re-running the travis build. |
We had a downtime yesterday, rerunning should go ok: retest this please. |
@ianoc-stripe not critical. I can do the change later and cc you so you'll see what I meant, how does that sound? |
@ittaiz That would be great, thanks for reviewing |
This is finally green and should be good to go! |
Thanks @ianoc-stripe |
This won't effect compiling source jars, but they shouldn't be in the dependency tree.
The issue here is that for maven_jar type imports we need to use the
jar:file
to avoid the ijar behavior for importing scala macros. But since scala 0.6.0bazelbuild/bazel@7a7c41d
Means that the source jars are also being included into this filegroup. Either we need a custom maven fetching rule, or this will strip
-sources.jar
from the dependencies tree. Since we wouldn't expect class files to be in sources jars already compiled this seems safe?(First push here should fail build with error all going well, then the new code can be activated to pass build)
r? @johnynek