Skip to content

feat: add env for write_jar_script again #20010

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

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

Conversation

daeho-ro
Copy link
Member

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?

I think I missed line break from

@daeho-ro
Copy link
Member Author

I believe this cause some error on

@@ -433,7 +433,8 @@ def write_jar_script(target_jar, script_name, java_opts = "", java_version: nil,
mkpath
(self/script_name).write <<~SH
#!/bin/bash
#{env_export}exec "${JAVA_HOME}/bin/java" #{java_opts} -jar "#{target_jar}" "$@"
#{env_export}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These won't be exported if on the line before.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried same as before, add export and line break.

@MikeMcQuaid
Copy link
Member

I believe this cause some error on

It looks like JAVA_HOME isn't ending up set there for some reason.

@daeho-ro
Copy link
Member Author

daeho-ro commented May 26, 2025

Here is the tika script

#!/bin/bash
JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME}/bin/java"  -jar "/opt/homebrew/Cellar/tika/3.2.0/libexec/tika-app-3.2.0.jar" "$@"

JAVA_HOME should be evaluated but it does not, right? How can I?

@daeho-ro daeho-ro force-pushed the write_jar_script-fix branch from 9d5b73a to 84074cb Compare May 26, 2025 16:17
@MikeMcQuaid
Copy link
Member

It maybe needs a preceding env. Let's revert for now.

@MikeMcQuaid
Copy link
Member

#20011

@calvinit
Copy link

Here is the tika script

#!/bin/bash
JAVA_HOME="${JAVA_HOME:-/opt/homebrew/opt/openjdk/libexec/openjdk.jdk/Contents/Home}" exec "${JAVA_HOME}/bin/java"  -jar "/opt/homebrew/Cellar/tika/3.2.0/libexec/tika-app-3.2.0.jar" "$@"

JAVA_HOME should be evaluated but it does not, right? How can I?

Maybe this can solve the problem?

def write_jar_script(target_jar, script_name, java_opts = "", java_version: nil, env: {})
  env[:JAVA_HOME] ||= Language::Java.overridable_java_home_env(java_version)[:JAVA_HOME]

  env_export = env.map do |key, value|
    %(export #{key}="#{value.gsub('"', '\"')}")
  end.join("\n")

  mkpath
  (self/script_name).write <<~EOS
    #!/bin/bash
    #{env_export}
    exec "${JAVA_HOME}/bin/java" #{java_opts} -jar "#{target_jar}" "$@"
  EOS
end

@daeho-ro daeho-ro force-pushed the write_jar_script-fix branch 2 times, most recently from e7ddb49 to d1e040d Compare May 28, 2025 13:37
@daeho-ro daeho-ro changed the title feat: add line break feat: add env for write_jar_script again May 28, 2025
@daeho-ro daeho-ro force-pushed the write_jar_script-fix branch from d1e040d to d23fb25 Compare May 28, 2025 14:04
@daeho-ro daeho-ro force-pushed the write_jar_script-fix branch from d23fb25 to 970b970 Compare May 28, 2025 14:18
#!/bin/bash
export JAVA_HOME="#{Language::Java.overridable_java_home_env(java_version)[:JAVA_HOME]}"
exec "${JAVA_HOME}/bin/java" #{java_opts} -jar "#{target_jar}" "$@"
EOS
#{env_export}exec "${JAVA_HOME}/bin/java" #{java_opts} -jar "#{target_jar}" "$@"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either:

env ENV=var exec

or

export ENV=var
exec

are needed here for valid syntax.

Either way, please ensure you've tested this and found it to be working locally before marking this as non-draft

@MikeMcQuaid MikeMcQuaid marked this pull request as draft May 29, 2025 07:50
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.

3 participants