-
Notifications
You must be signed in to change notification settings - Fork 175
Maven tweaks for usability #4039
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| api/src/main/java-templates | ||
| api/target | ||
| native/target | ||
| wasm/src/main/wasm | ||
| wasm/target | ||
| target | ||
| .idea |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| package org.ruby_lang.prism; | ||
| package org.ruby_lang.prism.jni; | ||
|
|
||
| public abstract class Parser { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This class, which is the only class under Maybe we should actually remove this class? Or import the C file here? OTOH this is an area where usages might want different things (e.g. how the reading is done, maybe from a file/using mmap/etc), and notably truffleruby compiles that .c file together with libprism.a. I'd tend to say the simplest is to just remove this file, but then maybe there is an issue because
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The logical change would be to put the JNI C code in here as well and use Maven to build it. But there's the larger issue of building, packaging, and releasing that JNI library. I have looked briefly at the available Maven tooling for building and releasing native libraries but never attempted to use them: https://www.mojohaus.org/maven-native/native-maven-plugin/ Unfortunately the source links for this plugin appear to be dead. I'm not sure what is considered "standard" for building and releasing JNI libraries at this point.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, the link from the mojohouse page was incorrect. The plugins are here:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking into other projects that publish native libraries, I have found no standard tooling to do this. lwjgl builds and publishes an extensive set of native binaries, but they also have an enormous set of ant build files to do it. MacOS example: https://github.com/LWJGL/lwjgl3/blob/4ef1eebe4af235b2934a165e82aeefcaf8d9b893/config/macos/build.xml#L140 Of course JRuby's JFFI also publishes many native libraries, but we have always built them by hand and committed them to the repository due to the challenges of doing this automatically.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the simplest for now would be to remove this file then, we can always add it back later if useful. It's like 3 lines of code and not useful on its own. It doesn't need to be solved in this PR, but if it removes an extra Maven module and/or simplifies things it seems useful to remove it earlier than later.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FWIW TruffleRuby and more specifically |
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| package org.ruby_lang.prism.wasm; | ||
|
|
||
| public final class WasmResource { | ||
| public static final String absoluteFile = "file://${project.basedir}/src/test/resources/prism.wasm"; | ||
| public static final String absoluteFile = "file://${project.basedir}/src/main/wasm/prism.wasm"; | ||
|
|
||
| private WasmResource() {} | ||
| } |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the thought and effort but it's better to directly copy files from my Prism checkout, that way it also works if e.g. I have some local changes in Prism I'm testing in TruffleRuby.
So this can be removed.