-
-
Notifications
You must be signed in to change notification settings - Fork 9.9k
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
write_jar_script: add java version option #3782
write_jar_script: add java version option #3782
Conversation
As mentioned here as long as it's not using |
Gotcha; I thought your objection was more about adding more DSL features. Thanks. |
Yeah, I'd just as soon get rid of all of them ;) |
Library/Homebrew/extend/pathname.rb
Outdated
mkpath | ||
java_home = if java_version | ||
<<~EOS | ||
export JAVA_HOME="$(#{Language::Java.java_home_cmd(java_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.
Is it worth skipping the export
and just putting the var on the same 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.
Sure. Amended.
join(script_name).write <<~EOS | ||
#!/bin/bash | ||
exec java #{java_opts} -jar #{target_jar} "$@" | ||
#{java_home}exec java #{java_opts} -jar #{target_jar} "$@" |
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.
Utter pedantry I realise (sorry) but does this not end up with yucky indentation in the output file?
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 don't think so? Unless you mean the two spaces between "java" and "-jar" when "java_opts" is empty?
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.
@apjanke np; when it was using EOS
I think the beginning of this line would have had weird indentation.
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 actually think it wouldn't've, because of ~EOS
's de-indentation. (That's the sort of thing that would bother me too, no worries on the pedantry.) But... moot point now. ;)
join(script_name).write <<~EOS | ||
#!/bin/bash | ||
exec java #{java_opts} -jar #{target_jar} "$@" | ||
#{java_home}exec java #{java_opts} -jar #{target_jar} "$@" |
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.
@apjanke np; when it was using EOS
I think the beginning of this line would have had weird indentation.
Thanks again @apjanke! |
👍 |
brew style
with your changes locally?brew tests
with your changes locally?This expands
write_jar_script
to allow specifying the Java version. This will be needed to fix some of the Java 9 breakage in Homebrew/homebrew-core#19696, for those formulae which usewrite_jar_script
instead ofwrite_env_script
.Closes #3656. This PR supersedes it; I'm putting in a new version since there's been no activity there in two weeks.
This is indeed another way to do Homebrew/homebrew-core@39e0b03, and I know ILZ won't like it. But consider this my vote that it's nicer to have
write_jar_script
do it. Let me know what you decide, and if it's a nay, I'll stick with the manual script generation like themetabase
formula does does. (Feel free to just downvote this PR to vote "No"; I won't take it personal.)