-
Notifications
You must be signed in to change notification settings - Fork 115
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
Allow to run on jre9 #409
Allow to run on jre9 #409
Conversation
project.clj
Outdated
@@ -27,6 +27,8 @@ | |||
:source-paths ["src/main/clojure"] | |||
:resource-paths [] | |||
:test-paths ["src/test/clojure" "src/test/common"] | |||
:jvm-opts ["-XX:+IgnoreUnrecognizedVMOptions" | |||
"--add-modules=java.xml.bind"] |
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.
We could add a new profile named something like :jdk9+
that added this "--add-modules=java.xml.bind" to :jvm-opts
.
If that were done, then there wouldn't be a need to make this top-level :jvm-opts
have to "-XX:+IgnoreUnrecognizedVMOptions" for all JVM versions.
We could also probably autodetect if "--add-modules=java.xml.bind" should be added via some lein project load time-eval (if worthwhile).
Is there a particular advantage to setting it here at the root of the project for all JVMs instead that you see? I guess it is mostly fine, except if there a re more options, it hides the fact that they may be specified incorrectly.
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 agree that it would be better to add an explicit additional profile than risk having wrong arguments. Also, some quick Googling suggests this is coming from a ClojureScript dependency: https://dev.clojure.org/jira/browse/CLJS-2377 . Is that the case or are we getting this from somewhere else? Also, will this have any impact on the jar built on the JVM or is it just for the JVM running during the build?
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 changed the PR by adding java9 as a lein profile.
Be sure to check out the https://github.com/cerner/clara-rules/blob/master/CONTRIBUTING.md too |
482a774
to
851e570
Compare
My thinking here is to merge this PR and log a followup issue to detect the Java version automatically in Leiningen/investigate what such options exists. This can't be the only project that has encountered this issue. @mrrodriguez thoughts? |
@WilliamParker That seems fine to me. I believe you could get that to work with some dynamic project eval stuff, but doesn't need to be done in this PR. |
@sunilgunisetty I've merged this and logged #412 as discussed above for looking into other ways to address this later, such as the setup automatically determining what JVM is running and adjusting accordingly. Thanks for bringing this up and sorry for the lengthy delay. |
@WilliamParker No problem. Thanks for the great work on Clara Rules ! |
No description provided.