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

Fix sbt/sbt#3736. Recurse into classloader of the class not the root #473

Merged
merged 4 commits into from Dec 22, 2017

Conversation

Projects
None yet
6 participants
@ravwojdyla
Contributor

ravwojdyla commented Dec 20, 2017

dwijnand added some commits Dec 11, 2017

Merge pull request #467 from sbt/fix-blog-link
Fix blog link in CONTRIBUTING
@lightbend-cla-validator

This comment has been minimized.

Show comment
Hide comment
@lightbend-cla-validator

lightbend-cla-validator Dec 20, 2017

Hi @ravwojdyla,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

lightbend-cla-validator commented Dec 20, 2017

Hi @ravwojdyla,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Lightbend Contributors License Agreement:

http://www.lightbend.com/contribute/cla

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Dec 20, 2017

Member

Thanks for the contribution @ravwojdyla ❤️. Could you check if this fixes sbt/sbt#3736? I would like to see some tests here to be 100% sure that this does the right thing.

Member

jvican commented Dec 20, 2017

Thanks for the contribution @ravwojdyla ❤️. Could you check if this fixes sbt/sbt#3736? I would like to see some tests here to be 100% sure that this does the right thing.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Dec 20, 2017

Member

I’ve gone into the details of this patch and the fix looks good to me, by the way. I’m surprised it hasn’t been caught before.

Member

jvican commented Dec 20, 2017

I’ve gone into the details of this patch and the fix looks good to me, by the way. I’m surprised it hasn’t been caught before.

@dwijnand

Given ClasspathFilter's purpose is to verify "that all classes loaded through parent either come from root or classpath" (from its scaladoc) I agree changing from traversing the root classloader's parents to the class's classloader's parents looks right.

And I'm equally surprised this wasn't caught before.

Ideally we'd have a test that accurately reproduces the downstream issue, but I'm happy to merge this without.

@ravwojdyla ravwojdyla changed the title from Fix sbt/sbt#3515. Recurse into classloader of the class not the root to Fix sbt/sbt#3736. Recurse into classloader of the class not the root Dec 20, 2017

@ravwojdyla

This comment has been minimized.

Show comment
Hide comment
@ravwojdyla

ravwojdyla Dec 20, 2017

Contributor

Thanks for reviews @dwijnand, @jvican. Sorry, I have made a mistake by referencing wrong sbt issue, this solves only sbt/sbt#3736. I updated the commit message and title of this PR. Regarding sbt/sbt#3515, this change does not solve it.

Contributor

ravwojdyla commented Dec 20, 2017

Thanks for reviews @dwijnand, @jvican. Sorry, I have made a mistake by referencing wrong sbt issue, this solves only sbt/sbt#3736. I updated the commit message and title of this PR. Regarding sbt/sbt#3515, this change does not solve it.

@jvican

This comment has been minimized.

Show comment
Hide comment
@jvican

jvican Dec 20, 2017

Member

To me, this is good to go IMO. Further investigation can take care of sbt/sbt#3736 😄.

Member

jvican commented Dec 20, 2017

To me, this is good to go IMO. Further investigation can take care of sbt/sbt#3736 😄.

@ravwojdyla

This comment has been minimized.

Show comment
Hide comment
@ravwojdyla
Contributor

ravwojdyla commented Dec 21, 2017

@jvican thanks. This patch also fixes sbt/sbt/issues/3647, sbt/sbt/issues/3608 and sbt/sbt/issues/3733

@ravwojdyla

This comment has been minimized.

Show comment
Hide comment
@ravwojdyla

ravwojdyla Dec 22, 2017

Contributor

FYI I have signed the CLA.

Contributor

ravwojdyla commented Dec 22, 2017

FYI I have signed the CLA.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Dec 22, 2017

Member

@ravwojdyla Hi, thanks for your contribution.

I'm trying to verify this by adding a scripted test locally, and I am unable to confirm using https://github.com/raboof/sbt-run-classloading/blob/master/src/main/scala/Main.scala. Does it run for you?

I am still getting

[error] Caused by: java.lang.ClassNotFoundException: scala.Int
[error] 	at sbt.internal.inc.classpath.ClasspathFilter.loadClass(ClassLoaders.scala:74)
[error] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
[error] 	at java.lang.Class.forName0(Native Method)
[error] 	at java.lang.Class.forName(Class.java:348)
[error] 	at Main$.delayedEndpoint$Main$1(A.scala:7)
Member

eed3si9n commented Dec 22, 2017

@ravwojdyla Hi, thanks for your contribution.

I'm trying to verify this by adding a scripted test locally, and I am unable to confirm using https://github.com/raboof/sbt-run-classloading/blob/master/src/main/scala/Main.scala. Does it run for you?

I am still getting

[error] Caused by: java.lang.ClassNotFoundException: scala.Int
[error] 	at sbt.internal.inc.classpath.ClasspathFilter.loadClass(ClassLoaders.scala:74)
[error] 	at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
[error] 	at java.lang.Class.forName0(Native Method)
[error] 	at java.lang.Class.forName(Class.java:348)
[error] 	at Main$.delayedEndpoint$Main$1(A.scala:7)
@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n

eed3si9n Dec 22, 2017

Member

Never mind. I think there was some internal discrepancy in scripted due to relative path vs absolute path. Let me send a PR on top of your PR.

Member

eed3si9n commented Dec 22, 2017

Never mind. I think there was some internal discrepancy in scripted due to relative path vs absolute path. Let me send a PR on top of your PR.

@eed3si9n

This comment has been minimized.

Show comment
Hide comment
@eed3si9n
Member

eed3si9n commented Dec 22, 2017

@ravwojdyla

This comment has been minimized.

Show comment
Hide comment
@ravwojdyla

ravwojdyla Dec 22, 2017

Contributor

Thanks @eed3si9n . Merged.

Contributor

ravwojdyla commented Dec 22, 2017

Thanks @eed3si9n . Merged.

@eed3si9n eed3si9n changed the base branch from 1.x to 1.1.x Dec 22, 2017

@eed3si9n eed3si9n merged commit 84a5ed1 into sbt:1.1.x Dec 22, 2017

1 check passed

continuous-integration/drone/pr the build was successful
Details

@ravwojdyla ravwojdyla deleted the ravwojdyla:fix_classnotfound branch Dec 22, 2017

@raboof

This comment has been minimized.

Show comment
Hide comment
@raboof

raboof Dec 22, 2017

Contributor

Thanks @ravwojdyla! 🎉

Contributor

raboof commented Dec 22, 2017

Thanks @ravwojdyla! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment