-
Notifications
You must be signed in to change notification settings - Fork 519
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
Add support for GraalVM ResourceURLConnection #447
base: master
Are you sure you want to change the base?
Conversation
For completeness, I'm attaching the full stack of what happens:
GraalVM
Why is it important to add the following change to Ring if it's only for GraalVM?
|
Thanks for the patch. I think explanations of why this is included should be mainly confined to the commit message rather than as part of a comment. We can limit the comment to something like |
Commit messages of this repository should follow the seven rules of a great Git commit message, as mentioned in the project's contributing guidelines. It looks like there's a few issues with the commit messages in this pull request:
|
GraalVM native-image produces the binary with embedded resources from jar:. The protocol is then substituted to resource:.
Thank you so much @weavejester. I have updated the patch. Hope the changes match BTW: Thank you for making Ring! |
@weavejester Is there something I can do more to have it merged? |
Apologies for the delay. I haven't had the time to setup a GraalVM environment to test this properly. Do you have an example project that uses Ring and GraalVM? |
Don't sorry, it's all good! I'm patient :) Working demo: here I have included the patch in my library (see here). How to test if patch is correct
PS: If you're using the Mac with M1 you would need to change |
Thanks for the link, but I'm looking for something a little easier to set up, and something we can keep around to test this in future (or even automate). For example, a small project that uses the Leiningen GraalVM plugin. |
Hi @weavejester! Sorry for not responding for so long. Here is the repro/example. Let me know if it's minimal enough. I've also prepared the configuration necessary to run Ring apps with GraalVM CE 21.3.0. See here. If you're interested in pushing it upstream, I'm happy to help with this. |
This PR is not yet ready to merge. @borkdude found a corner case. |
Thanks for the heads up. |
Any updates on this? It seems for now that I need to add these lines manually to every project using ring (or perhaps reitit with ring) that I want to build a native image. |
Waiting for this too! |
@agorgl @skynet-gh I forgot to create an issue on the GraalVM repo. I will try to find some time this week to address it, but I don't promise anything 😂 |
0d7f29a
to
0103527
Compare
Dear @weavejester please consider adding the following patch to
ring
.Why this is important?
I'm trying to patch as many libraries as it's possible to make the GraalVM Clojure applications easier to configure.
With the following change, I would be able to deploy a full Clojure application with
metosin/swagger-ui
on AWS Lambda.