Skip to content

Conversation

@zakkak
Copy link
Collaborator

@zakkak zakkak commented Feb 15, 2022

Fix #4326

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 15, 2022

Note: this draft allows resolving files from resourceNames with trailing slashes which is not desirable.

Update: this is now fixed.

Update2: testing this branch with latest Quarkus seems to resolve the Quarkus issues that appeared after #4281 (see https://github.com/graalvm/mandrel/actions/runs/1848087712)

I am keeping this as draft till I get some tests in place to ensure this doesn't break again in the future.

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 16, 2022

I am keeping this as draft till I get some tests in place to ensure this doesn't break again in the future.

It appears that mx does not create JarEntrys for directories when generating the svm-tests.jar and thus I cannot register a directory as resource in order to implement the tests. @olpaw do you have any suggestions on how to work around this issue? Is there any way to force mx to create JarEntrys for directories in the svm-tests.jar?

@zakkak zakkak marked this pull request as ready for review February 16, 2022 21:14
@zakkak
Copy link
Collaborator Author

zakkak commented Feb 16, 2022

This is now ready for review. In order for the new tests to work mx needs to be patched with

diff --git a/mx_jardistribution.py b/mx_jardistribution.py
index 42e6c3f..d60f504 100755
--- a/mx_jardistribution.py
+++ b/mx_jardistribution.py
@@ -998,6 +998,8 @@ class _ArchiveStager(object):
             def add_classes(archivePrefix, includeServices):
                 for root, _, files in os.walk(outputDir):
                     reldir = root[len(outputDir) + 1:]
+                    dirEntry = reldir.replace(os.sep, '/') + '/'
+                    self.bin_archive.entries[dirEntry] = dirEntry
                     for f in files:
                         relpath = join(reldir, f)
                         self.add_file(dep, outputDir, relpath, archivePrefix, arcnameCheck=overlay_check, includeServices=includeServices)

I don't know if this change would be welcome on mx since it will affect all jars. Please advice on how to proceed.

@olpaw
Copy link
Member

olpaw commented Feb 21, 2022

@olpaw do you have any suggestions on how to work around this issue? Is there any way to force mx to create JarEntrys for directories in the svm-tests.jar?

After spending way too much time to find something appropriate I now think the best would be to add this to mx. But not like suggested in #4327 (comment)

Instead I would make the mentioned code conditional of a special filename (e.g. .empty_dir). If we have such an entry, the archiver should not add the file but instead the directory that contains file. @gilles-duboscq are you ok with such a change in mx?

@gilles-duboscq
Copy link
Member

i think that's fine

@olpaw
Copy link
Member

olpaw commented Feb 21, 2022

@zakkak please prepare the mx PR that we need for this PR and add me and @gilles-duboscq as reviewers.

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 22, 2022

Thanks for the input @olpaw !

@zakkak please prepare the mx PR that we need for this PR and add me and @gilles-duboscq as reviewers.

Done. This PR is now blocked by graalvm/mx#256

@olpaw
Copy link
Member

olpaw commented Feb 23, 2022

@zakkak your mx PR got merged. Now you need to update this PR to make substratevm depend on the version of mx that contains your change. I.e. adjust

"mxversion": "5.316.15",
accordingly.

@zakkak
Copy link
Collaborator Author

zakkak commented Feb 23, 2022

@olpaw mxversion is now up to date.

@olpaw olpaw changed the title Fix handling of resources with trailing slashes [GR-37154] Fix handling of resources with trailing slashes Feb 24, 2022
@olpaw olpaw self-assigned this Feb 24, 2022
- Adds .mxkeep file to create the necessary jar entry
- Bumps requires mx version to work with .mxkeep
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GraalVM's native-image substitute for ClassLoader.getResource normalizes resources' names while original doesn't

4 participants