-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Retrieve resource bytes from map as URLStreamHandler can be shared #3336
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
Tested this locally, and it fixes the issue indeed. |
It looks like we are dealing with the same issue. The only difference is that my changes are part of bigger resource refactoring. Take a look at changes in this PR #3315. If you have any suggestions let me know. |
@olpaw or @christianwimmer can you please look at this? |
@jovanstevanovic Building GraalVM from head (so #3315 is included), works (at least related to the resources issues #2291 and gluonhq/substrate#291), but only on Mac and Linux. Windows fails. I've run these two tests:
|
I've also tested some dev builds for Java 11 on Windows:
|
@jovanstevanovic the findings from @jperedadnr indicate that the new resource implementation does not work as expected on Windows. Please fix this on master and also on the soon-to-created 21.2.0-branch. Resource support has to work properly for all our supported platforms. |
@jperedadnr thanks for your testing #3336 (comment) #3336 (comment) Now that @jovanstevanovic s new resource support is on master the changes this PR provides are obsolete and the PR should be closed. Makes sense?
Created #3496 |
Sure, no problem, you can close this PR. However, and if it helps: since this PR fixes it for Windows, if it is useful for #3315, feel free to use all or parts of it. |
Thanks. I will create a new issue and make it refer to this PR for guidance. |
Hey @jperedadnr! Thank you for reporting the issue and testing this out! It turned out to be a problem with the way we pass file paths to resource inclusion matchers. I've opened #3498 to address this and I've tested out gluonhq/substrate#291 (comment):
The reason we need to add the resource manually is because the I've noticed that there were a couple of other Gluon samples, specifically https://github.com/gluonhq/gluon-samples/tree/master/HelloFXML, however, these seem to require a custom GraalVM build on Windows to test. Could you please try out the fix and see if it works, or is there another reproducer that I could run locally? I've tried running it without a custom GraalVM build but I was hitting linker errors due to missing symbols |
I could try to apply the patch from #3498 to do a custom build and then run the tests again on Windows. I'll report back. |
I've tested successfully this patch, the three tests reported that were failing now pass. |
Thank you very much for verifying this! :) |
This PR fixes #2291.
When this approach is used:
the first resource is registered during build time, and the URL is created via
Resources::createURL
(on runtime), where anew URLStreamHandler()
is created based on itsresourceBytes
.However, for the second resource, the
URLStreamHandler
from the context (the first resource) is used, according to theURL
constructor.If that is the case, the
resourceBytes
can't be taken from the first resource, and theresources
map has to be used to retrieve such bytes, if such resource2 exists and was correctly registered during built time.In the test case added to the issue, the
Bogus.class
resource is not found, so a FNFE is thrown, and the test succeeds.Another use case of such approach can be found in the JavaFX
FXMLLoader
class, when an FXML file loads another FXML resource viafx:include
, see issue.