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.

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

Merged

Conversation

ianoc-stripe
Copy link
Contributor

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.0
bazelbuild/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

@bazel-io
Copy link
Member

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
Copy link
Contributor Author

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),
Copy link
Member

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"

Copy link
Member

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

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 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).

@ittaiz
Copy link
Member

ittaiz commented Sep 18, 2017 via email

@ianoc-stripe
Copy link
Contributor Author

ianoc-stripe commented Sep 18, 2017

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!

@ianoc-stripe
Copy link
Contributor Author

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?

@johnynek
Copy link
Member

test this please

(ci bot message)

@johnynek
Copy link
Member

test this please

Copy link
Member

@ittaiz ittaiz left a 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 ])
Copy link
Member

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?

Copy link
Member

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)?

@johnynek
Copy link
Member

👍

once @ittaiz is happy.

@ittaiz
Copy link
Member

ittaiz commented Sep 18, 2017

BTW, I hope to merge one of the codepaths and just turn the strict deps on.

@ianoc-stripe
Copy link
Contributor Author

Cool -- the strict deps stuff does sound nice i've not used it yet. But its very nice in the java rules

@ittaiz
Copy link
Member

ittaiz commented Sep 18, 2017

Awesome, thanks! I'll merge once build finishes.

@ittaiz
Copy link
Member

ittaiz commented Sep 18, 2017

test this please

@johnynek
Copy link
Member

huh, bazel ci thinks this failed.

@johnynek
Copy link
Member

does this pass locally for you @ianoc-stripe ?

@ianoc-stripe ianoc-stripe force-pushed the ianoc/fixSourceJarsToClasspath branch from d510bf3 to 361f5a2 Compare September 18, 2017 19:00
@ianoc-stripe
Copy link
Contributor Author

No sorry, just pushed thinking it was tiny. ofc I always forget the need to do return in python. (passes locally now)

@johnynek
Copy link
Member

test this please

@ittaiz
Copy link
Member

ittaiz commented Sep 18, 2017 via email

@ianoc-stripe
Copy link
Contributor Author

Looks like travis hasn't started ? ci from bazel looks green

@ianoc-stripe
Copy link
Contributor Author

Looks like travisci is having a sad day fyi -- https://www.traviscistatus.com/incidents/4qj8t3220jy8

@johnynek
Copy link
Member

seems like this failed on 0.6.0.rc2: https://travis-ci.org/bazelbuild/rules_scala/jobs/276986474

@johnynek
Copy link
Member

test this please

@ianoc-stripe
Copy link
Contributor Author

the bazel ci failure looks transient i think, might need to re-request the test?

@johnynek
Copy link
Member

test this please

@ianoc-stripe
Copy link
Contributor Author

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 :(

@johnynek
Copy link
Member

this this please

@johnynek
Copy link
Member

@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.

@johnynek
Copy link
Member

@ianoc-stripe I'm re-running the travis build.

@damienmg
Copy link
Contributor

We had a downtime yesterday, rerunning should go ok: retest this please.

@ittaiz
Copy link
Member

ittaiz commented Sep 19, 2017

@ianoc-stripe not critical. I can do the change later and cc you so you'll see what I meant, how does that sound?

@ianoc-stripe
Copy link
Contributor Author

@ittaiz That would be great, thanks for reviewing

@ianoc-stripe
Copy link
Contributor Author

This is finally green and should be good to go!

@johnynek johnynek merged commit 85e154c into bazelbuild:master Sep 19, 2017
@johnynek
Copy link
Member

Thanks @ianoc-stripe

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants