-
Notifications
You must be signed in to change notification settings - Fork 60
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
Move VM options to jvm.config #18
Conversation
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.
LGTM, minor comment.
.mvn/jvm.config
Outdated
@@ -0,0 +1 @@ | |||
--add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.net=ALL-UNNAMED |
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.
if you could get rid of that eye sore end of line.
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.
haha, done 🖖
391dd12
to
151a45f
Compare
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.
LGTM
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 should take the related setting in the pom out as part of this PR as well
@@ -281,8 +281,7 @@ if [ -n "$wrapperSha256Sum" ]; then | |||
fi | |||
|
|||
#This option is required by the javalite db-migrator-maven-plugin, which does not support adding VM options in its config | |||
REQUIRED_BUILD_OPTIONS='--add-opens=java.base/java.net=ALL-UNNAMED' | |||
MAVEN_OPTS="$(concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $REQUIRED_BUILD_OPTIONS $MAVEN_OPTS" | |||
MAVEN_OPTS="$(concat_lines "$MAVEN_PROJECTBASEDIR/.mvn/jvm.config") $MAVEN_OPTS" |
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.
do NOT modify the generated mvnw wrapper script .. this will be wiped whenever we upgrade the wrapper
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.
this changes it back to the generated version
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.
except for the comment - let me regenerate the wrapper
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.
oh .. good
151a45f
to
77440d9
Compare
77440d9
to
2fb5e28
Compare
This fails to build locally for me .. you want output @willmostly ? |
Is this ready now @willmostly ? I don't think you pushed the regenerated wrapper.. |
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.
Looks good now. Thank you for updating and cofirming.
This allows the build to succeed using
mvn install
instead of requiring./mvnw install