[GR-50220] Allow resources inclusion from jar files in paths containing spaces #7815

zakkak · 2023-11-13T15:07:35Z

Since #7670 it is no longer possible to build projects that include resources from paths with spaces.

How to reproduce:

Build graal master from source
export GRAALVM_HOME=/path/to/graal/master/build
git clone https://github.com/quarkusio/quarkus
cd quarkus
./mvnw -Dquickly
./mvnw -Dnative -pl integration-tests/simple\ with\ space -Dnative.surefire.skip -Dformat.skip -Dno-descriptor-tests clean verify -Dquarkus.native.container-build=false

This PR resolves this issue and adds a set of tests for resource names with spaces (which don't catch the issue at hand, but are still good to test against).

Closes quarkusio/quarkus#36998

jerboaa

I think we should change this to ((JarURLConnection) url.openConnection()).getJarFileURL().toURI().getPath(); instead. See below.

jerboaa · 2023-11-15T13:55:28Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ResourcesFeature.java

+        private JarFile urlToJarFile(URL url) {
            try {
-                return ((JarURLConnection) url.openConnection()).getJarFileURL().getFile();
+                return ((JarURLConnection) url.openConnection()).getJarFile();
            } catch (IOException e) {
                throw new RuntimeException(e);
            }
        }

        private boolean resourceIsDirectory(URL url, boolean fromJar, String resourcePath) throws IOException, URISyntaxException {
            if (fromJar) {
-                try (JarFile jf = new JarFile(urlToJarPath(url))) {
+                try (JarFile jf = urlToJarFile(url)) {


This seems to be the gist of it. Let url be new URL("jar:file:///home/sgehwolf/Documents/mandrel/bugs/quarkus/itest-with-space/test%20with%20space/byteman.jar!/META-INF/MANIFEST.MF")

Then we have this for the old code:

jshell> var x = ((JarURLConnection) url.openConnection()).getJarFileURL().getFile(); x ==> "/home/sgehwolf/Documents/mandrel/bugs/quarkus/it ... 0with%20space/byteman.jar" jshell> JarFile f = new JarFile(x); | Exception java.nio.file.NoSuchFileException: /home/sgehwolf/Documents/mandrel/bugs/quarkus/itest-with-space/test%20with%20space/byteman.jar | at UnixException.translateToIOException (UnixException.java:92) | at UnixException.rethrowAsIOException (UnixException.java:106) | at UnixException.rethrowAsIOException (UnixException.java:111) | at UnixFileAttributeViews$Basic.readAttributes (UnixFileAttributeViews.java:55) | at UnixFileSystemProvider.readAttributes (UnixFileSystemProvider.java:148) | at LinuxFileSystemProvider.readAttributes (LinuxFileSystemProvider.java:99) | at Files.readAttributes (Files.java:1851) | at ZipFile$Source.get (ZipFile.java:1394) | at ZipFile$CleanableResource.<init> (ZipFile.java:716) | at ZipFile.<init> (ZipFile.java:250) | at ZipFile.<init> (ZipFile.java:179) | at JarFile.<init> (JarFile.java:346) | at JarFile.<init> (JarFile.java:317) | at JarFile.<init> (JarFile.java:256) | at (#29:1) jshell> System.out.println(x); /home/sgehwolf/Documents/mandrel/bugs/quarkus/itest-with-space/test%20with%20space/byteman.jar

And the new code:

jshell> URL url = new URL("jar:file:///home/sgehwolf/Documents/mandrel/bugs/quarkus/itest-with-space/test%20with%20space/byteman.jar!/META-INF/MANIFEST.MF") url ==> jar:file:///home/sgehwolf/Documents/mandrel/bugs/ ... .jar!/META-INF/MANIFEST.MF jshell> var x = ((JarURLConnection) url.openConnection()).getJarFile(); x ==> sun.net.www.protocol.jar.URLJarFile@4aa8f0b4

I.e. JarFile created from a String, expects URL decoded paths. OK. But looking at the implementation of getJarFile() I see that it creates a copy of the file, which is probably not what we want.

So I suggest to use:

private String urlToJarFile(URL url) { try { return ((JarURLConnection) url.openConnection()).getJarFileURL().toURI().getPath(); } catch (IOException e) { throw new RuntimeException(e); } }

... which should properly decode %20 to :

jshell> URL url = new URL("jar:file:///home/sgehwolf/Documents/mandrel/bugs/quarkus/itest-with-space/test%20with%20space/byteman.jar!/META-INF/MANIFEST.MF") url ==> jar:file:///home/sgehwolf/Documents/mandrel/bugs/ ... .jar!/META-INF/MANIFEST.MF jshell> var x = ((JarURLConnection) url.openConnection()).getJarFileURL().toURI().getPath(); x ==> "/home/sgehwolf/Documents/mandrel/bugs/quarkus/it ... st with space/byteman.jar" jshell> JarFile f = new JarFile(x); f ==> java.util.jar.JarFile@2a3046da

I see that it creates a copy of the file, which is probably not what we want.

Good catch @jerboaa

So I suggest to use...

Done. Thanks!

Closes quarkusio/quarkus#36998

jerboaa

Would be nice to actually catch this in a test, but this seems fine to me and should be a safe change. Thanks!

zakkak · 2023-11-16T11:34:27Z

Would be nice to actually catch this in a test

One option would be to build graal in a folder containing a space, but unfortunately that's not supported (there are some ninja related failures if you try to do it).

Another option would be to add a space in the jar file itself, the name is derived from

graal/substratevm/mx.substratevm/suite.py

Line 1978 in 35d412e

"SVM_TESTS" : {

so the following patch does the trick:

diff --git a/substratevm/mx.substratevm/suite.py b/substratevm/mx.substratevm/suite.py
index 7c9555bccb5..713abcb06f4 100644
--- a/substratevm/mx.substratevm/suite.py
+++ b/substratevm/mx.substratevm/suite.py
@@ -1975,7 +1975,7 @@ suite = {
             "testDistribution" : True,
         },
 
-        "SVM_TESTS" : {
+        "SVM TESTS" : {
           "subDir": "src",
           "relpath" : True,
           "dependencies" : [

However, it will apply to all tests, i.e. we won't have a separate jar with spaces in the filename only for this test case. Which means that without this PR it results in:

com.oracle.svm.configure.test.config.OmitPreviousConfigTests started (4 of 65)
  testConfigDifference: Fatal error: Unexpected error while locating the configuration files.: java.lang.RuntimeException: java.nio.file.NoSuchFileException: /home/zakkak/code/graal/substratevm/mxbuild/jdk22/dists/jdk21/svm%20tests.jar
    at com.oracle.svm.core.jdk.Resources.get(Resources.java:317)
    at com.oracle.svm.core.jdk.Resources.createURLs(Resources.java:434)
    at com.oracle.svm.core.jdk.Resources.createURL(Resources.java:366)
    at com.oracle.svm.core.jdk.Resources.createURL(Resources.java:358)
    at com.oracle.svm.core.jdk.ResourcesHelper.nameToResourceURL(ResourcesHelper.java:102)
    at jdk.internal.loader.BootLoader.findResource(BootLoader.java:609)
    at java.lang.ClassLoader.getResource(ClassLoader.java:1409)
    at java.lang.ClassLoader.getResource(ClassLoader.java:1407)
    at java.lang.Class.getResource(DynamicHub.java:3160)
    at com.oracle.svm.configure.test.config.OmitPreviousConfigTests.lambda$loadTraceProcessorFromResourceDirectory$0(OmitPreviousConfigTests.java:68)
    at com.oracle.svm.configure.config.ConfigurationFileCollection.addDirectory(ConfigurationFileCollection.java:73)
    at com.oracle.svm.configure.test.config.OmitPreviousConfigTests.loadTraceProcessorFromResourceDirectory(OmitPreviousConfigTests.java:65)
    at com.oracle.svm.configure.test.config.OmitPreviousConfigTests.testConfigDifference(OmitPreviousConfigTests.java:107)
    at java.lang.reflect.Method.invoke(Method.java:580)
    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:376)
    at com.oracle.svm.junit.SVMJUnitRunner.run(SVMJUnitRunner.java:164)
    at com.oracle.svm.junit.SVMJUnitRunner.main(SVMJUnitRunner.java:184)

I will try to create a new jar file (mx distribution) and add it as a dependency to SVM_TESTS to make this test-specific.

zakkak · 2023-11-16T13:32:48Z

I will try to create a new jar file (mx distribution) and add it as a dependency to SVM_TESTS to make this test-specific.

Done in 8e03be8

jerboaa

LGTM. @fniephaus Could you help us get this pushed/reviewed? Thanks!

jerboaa · 2023-11-16T17:22:53Z

substratevm/mx.substratevm/suite.py

          "testDistribution" : True,
        },

+        "SVM_TESTS WITH SPACE" : {


Might warrant a comment that there is intentionally a space in the name. Hard to spot ;-)

Yes indeed. @zakkak please add a comment in suite.py why we need exactly that.

jerboaa · 2023-11-17T10:37:29Z

@olpaw @dnestoro Could you help us getting this reviewed/integrated, please? It's a regression caused by #7670. Thanks!

dnestoro · 2023-11-17T10:42:33Z

@olpaw @dnestoro Could you help us getting this reviewed/integrated, please? It's a regression caused by #7670. Thanks!

Sure, I will take a look on the PR next week (today I have some other priorities). Is that okay (or it is urgent to be done today)?

jerboaa · 2023-11-17T10:46:42Z

@olpaw @dnestoro Could you help us getting this reviewed/integrated, please? It's a regression caused by #7670. Thanks!

Sure, I will take a look on the PR next week (today I have some other priorities). Is that okay (or it is urgent to be done today)?

Next week is fine. We just don't want such issues hanging in the air without knowing if you guys are aware (as it's causing test failures for us). Thanks for the reply!

dnestoro · 2023-11-20T14:06:44Z

Looks good to me! @olpaw can you also take a look on it before it gets merged?

jerboaa · 2023-11-20T14:07:59Z

Thanks for the review!

olpaw · 2023-11-20T14:18:16Z

substratevm/src/com.oracle.svm.hosted/src/com/oracle/svm/hosted/ResourcesFeature.java

-                return ((JarURLConnection) url.openConnection()).getJarFileURL().getFile();
-            } catch (IOException e) {
+                return ((JarURLConnection) url.openConnection()).getJarFileURL().toURI().getPath();
+            } catch (IOException | URISyntaxException e) {


Thanks for the small cleanups as well 😁

zakkak · 2023-11-20T18:15:18Z

@jerboaa @dnestoro @olpaw Thank you for the review

jerboaa · 2023-11-21T09:52:48Z

Am I correct in thinking that this is in the merge queue?

olpaw · 2023-11-21T10:33:53Z

Running in internal CI. Should be in merge queue soon.

jerboaa · 2023-11-21T10:47:59Z

Thanks!

jerboaa · 2023-11-24T09:43:11Z

@olpaw Any update on the merge queue status of this? Did it fail?

zapster · 2023-11-24T09:45:57Z

@jerboaa it is still in the merge queue. There is a lot going on currently, but I'm having an eye on it and make sure that it will go in.

jerboaa · 2023-11-24T09:50:08Z

@zapster Thank you! Appreciate that.

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Nov 13, 2023

zakkak added quarkus quarkus related issue redhat-interest labels Nov 13, 2023

zakkak mentioned this pull request Nov 13, 2023

[GraalVM 24.0] quarkus-integration-test-simple-with-space fails to run with a recent JDK 22-based GraalVM master build quarkusio/quarkus#36998

Closed

Add tests for resources with spaces in their path

9130aa1

zakkak force-pushed the 2023-11-13-resources-with-space-fix branch from c39af7b to 7a31d81 Compare November 14, 2023 07:33

jerboaa reviewed Nov 15, 2023

View reviewed changes

Allow resources inclusion from jar files in paths containing spaces

3eedd42

Closes quarkusio/quarkus#36998

zakkak force-pushed the 2023-11-13-resources-with-space-fix branch from 7a31d81 to 3eedd42 Compare November 16, 2023 11:02

zakkak requested review from dnestoro and jerboaa November 16, 2023 11:03

jerboaa approved these changes Nov 16, 2023

View reviewed changes

zakkak force-pushed the 2023-11-13-resources-with-space-fix branch 2 times, most recently from 436f857 to 8e03be8 Compare November 16, 2023 13:47

jerboaa approved these changes Nov 16, 2023

View reviewed changes

dnestoro approved these changes Nov 20, 2023

View reviewed changes

olpaw self-requested a review November 20, 2023 14:15

olpaw reviewed Nov 20, 2023

View reviewed changes

olpaw approved these changes Nov 20, 2023

View reviewed changes

olpaw changed the title ~~Allow resources inclusion from jar files in paths containing spaces~~ [GR-50220] Allow resources inclusion from jar files in paths containing spaces Nov 20, 2023

Add native unit test to test resource inclusion from path with space

2d4c719

zakkak force-pushed the 2023-11-13-resources-with-space-fix branch from 8e03be8 to 2d4c719 Compare November 20, 2023 18:14

graalvmbot merged commit 50218dc into oracle:master Nov 25, 2023

zakkak deleted the 2023-11-13-resources-with-space-fix branch November 27, 2023 07:31

graalvmbot mentioned this pull request Nov 30, 2023

[GR-50375] Move test resource with space into a jar #7927

Merged

[GR-50220] Allow resources inclusion from jar files in paths containing spaces #7815

[GR-50220] Allow resources inclusion from jar files in paths containing spaces #7815

Uh oh!

Conversation

zakkak commented Nov 13, 2023

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

jerboaa Nov 15, 2023

Choose a reason for hiding this comment

Uh oh!

zakkak Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

zakkak commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zakkak commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jerboaa left a comment

Choose a reason for hiding this comment

Uh oh!

jerboaa Nov 16, 2023

Choose a reason for hiding this comment

Uh oh!

olpaw Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

zakkak Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

jerboaa commented Nov 17, 2023

Uh oh!

dnestoro commented Nov 17, 2023

Uh oh!

jerboaa commented Nov 17, 2023

Uh oh!

dnestoro commented Nov 20, 2023

Uh oh!

jerboaa commented Nov 20, 2023

Uh oh!

olpaw Nov 20, 2023

Choose a reason for hiding this comment

Uh oh!

zakkak commented Nov 20, 2023

Uh oh!

jerboaa commented Nov 21, 2023

Uh oh!

olpaw commented Nov 21, 2023

Uh oh!

jerboaa commented Nov 21, 2023

Uh oh!

jerboaa commented Nov 24, 2023

Uh oh!

zapster commented Nov 24, 2023

Uh oh!

jerboaa commented Nov 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zakkak commented Nov 16, 2023 •

edited

Loading

zakkak commented Nov 16, 2023 •

edited

Loading