Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

FieryCod
Copy link

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.

@FieryCod
Copy link
Author

For completeness, I'm attaching the full stack of what happens:

  1. Server hits the swagger-ui ring handler
    https://github.com/metosin/reitit/blob/master/modules/reitit-swagger-ui/src/reitit/swagger_ui.cljc#L40
  2. Reitit uses response/resource-response under the hood https://github.com/metosin/reitit/blob/master/modules/reitit-ring/src/reitit/ring.cljc#L237
  3. (defn resource-response
  4. (let [response (url-response resource)]
  5. (defn url-response
  6. And here the protocol is different:
    (defmulti resource-data

GraalVM native-image produces the binary with embedded resources from jar:. They substitute the protocol with resource:, so that they can .getResource embedded in the binary.

Why is it important to add the following change to Ring if it's only for GraalVM?

  1. Almost all of the Clojure frameworks are based on Ring. If we add the following change in the Ring we will not have to patch other libraries that use response/resource-response.
  2. Without this change GraalVM returns a cryptic error on runtime.

@weavejester
Copy link
Member

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 ;; GraalVM resource scheme.

@atomist
Copy link

atomist bot commented Sep 15, 2021

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:

  • The commit message should not contain markdown.
  • The commit message body has a line over 72 characters.

GraalVM native-image produces the binary with embedded resources from
jar:. The protocol is then substituted to resource:.
@FieryCod
Copy link
Author

FieryCod commented Sep 15, 2021

Thank you so much @weavejester. I have updated the patch. Hope the changes match Ring standards :)

BTW: Thank you for making Ring!

@FieryCod
Copy link
Author

@weavejester Is there something I can do more to have it merged?

@weavejester
Copy link
Member

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?

@FieryCod
Copy link
Author

FieryCod commented Nov 1, 2021

Don't sorry, it's all good! I'm patient :)

Working demo: here
Source code: here

I have included the patch in my library (see here).

How to test if patch is correct

  1. Install aws, sam, bb commands. Make sure you have a docker, and at least 8GB of RAM. (if you're using mac enable 8GB in Docker Desktop).

  2. Configure you AWS Account via aws configure

  3. Clone the library.

  4. Remove the patch.

  5. Go to the examples/native/

  6. You would need aws, sam, bb CLI's to deploy.

  7. Run the following commands in examples/native/:

    bb hl:compile && bb hl:native:executable && sam deploy --guided
  8. After deployment get the URL of the endpoint and navigate to <URL>/api-docs/index.html. It should not work without the patch.

PS: If you're using the Mac with M1 you would need to change :image in bb.edn to ghcr.io/fierycod/holy-lambda-builder:aarch64-java11-21.3.0.

@weavejester
Copy link
Member

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.

@FieryCod
Copy link
Author

FieryCod commented Dec 3, 2021

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 found this issue: #370, which I was not aware of before.

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.

@FieryCod
Copy link
Author

This PR is not yet ready to merge. @borkdude found a corner case.
When serving a resource directory e.g. public at /public and trying to access the /public the resource-data will return an URL instead of nil.

@weavejester
Copy link
Member

Thanks for the heads up.

@skynet-gh
Copy link

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.

@agorgl
Copy link

agorgl commented Nov 9, 2022

Waiting for this too!

@FieryCod
Copy link
Author

FieryCod commented Nov 9, 2022

@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 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants