-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Don't prepend resource URLs with '/' #4316
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
Conversation
|
@olpaw can you please have a look at this? |
In PR oracle#4281 resource URLs started being prefixed with '/', which results in failures to retrieve the resource in some cases. E.g. quarkusio/quarkus#23604
4e52779 to
2042745
Compare
| try { | ||
| String refPart = index != 0 ? '#' + Integer.toString(index) : ""; | ||
| return new URL(JavaNetSubstitutions.RESOURCE_PROTOCOL, moduleName, -1, '/' + resourceName + refPart); | ||
| return new URL(JavaNetSubstitutions.RESOURCE_PROTOCOL, moduleName, -1, resourceName + refPart); |
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.
@zakkak, we cannot simply omit the '/' separator. Otherwise we will end up with URLs like e.g.
resource://jdk.compilermodule-info.class
where it actually should be
resource://jdk.compiler/module-info.class
which will break the ability to create resource URLs from their string representations. Something that works fine with URLs created from resource access when running on the JVM. We therefore have to support that as well.
Consequently this change causes our gates to fail with
1) com.oracle.svm.test.NativeImageResourceFileSystemProviderTest#testURLExternalFormEquivalence
java.lang.AssertionError: Contents of original URL and one created from originals ExternalForm must be the same: java.io.FileNotFoundException: resource://jdk.compilermodule-info.class
at org.junit.Assert.fail(Assert.java:88)
at com.oracle.svm.test.NativeImageResourceFileSystemProviderTest.testURLExternalFormEquivalence(NativeImageResourceFileSystemProviderTest.java:631)
at java.lang.reflect.Method.invoke(Method.java:568)
at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:50)
at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:47)
at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:325)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:78)
at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:57)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runners.Suite.runChild(Suite.java:128)
at org.junit.runners.Suite.runChild(Suite.java:27)
at org.junit.runners.ParentRunner$3.run(ParentRunner.java:290)
at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:71)
at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:288)
at org.junit.runners.ParentRunner.access$000(ParentRunner.java:58)
at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:268)
at org.junit.runners.ParentRunner.run(ParentRunner.java:363)
at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
at com.oracle.mxtool.junit.MxJUnitWrapper.runRequest(MxJUnitWrapper.java:368)
at com.oracle.svm.junit.SVMJUnitRunner.run(SVMJUnitRunner.java:217)
at com.oracle.svm.junit.SVMJUnitRunner.main(SVMJUnitRunner.java:237)
FAILURES!!!
Tests run: 61, Failures: 1
You can test this locally with
mx build
mx gate --omit-clean -t 'native unittests'
from the substratevm subdir.
Since our resource URL support was actually completely broken before merging e71f13e could it be that the failing quarkus test needs adjustment?
Can you create a small reproducer (or a concrete example) of what actually fails on your side?
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 for the detailed explanation @olpaw . Further investigation revealed that the breakage was indeed not caused by the leading slash (at least for one of the cases), but it was unveiled by it :).
I opened #4326 for the underlying issue and prepared a draft PR for a fix (but I was not able to write a unit test for it yet, I have some trouble registering a directory as a resource) #4327.
|
Closing as invalid. |
In PR #4281 resource URLs started being prefixed with '/', which results
in failures to retrieve the resource in some cases.
E.g. quarkusio/quarkus#23604