Skip to content
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

respect JAVA_HOME env in linux/mac launch scripts #6008

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

janAkali
Copy link

Forge should use java version set by JAVA_HOME first and if it's not set - only then use plain "java" invocation.

Also fixed warnings reported by shellcheck utility:

  • SC2164 "cd command can fail.." => "cd ... || exit"
  • SC2155 "local foo=..." => "local foo; foo="
  • SC2086 added double quotes where word-splitting is not needed

@Hanmac
Copy link
Contributor

Hanmac commented Aug 29, 2024

do you have a mac to test the scripts?

can you update the forge.command script too? #5548
it still misses the version checks from the forge.sh

@janAkali
Copy link
Author

janAkali commented Aug 29, 2024

do you have a mac to test the scripts?

No, I don't have a mac to test scripts, but the changes I made are POSIX-compliant. It should work the same with every default shell on any OS (dash, bash, zsh, etc..).

can you update the forge.command

AFAIK there's no difference between ".sh" and ".command" file extensions in MacOS. (see https://apple.stackexchange.com/questions/24262/what-is-the-difference-between-command-tool-and-sh-file-extensions)
Having to support two identical launch scripts seems unnecessary. Maybe it'd be better to remove them? (question is addressed to maintainers)

@Hanmac
Copy link
Contributor

Hanmac commented Aug 29, 2024

do you have a mac to test the scripts?

No, I don't have a mac to test scripts, but the changes I made are POSIX-compliant. It should work the same with every default shell on any OS (dash, bash, zsh, etc..).

can you update the forge.command

AFAIK there's no difference between ".sh" and ".command" file extensions in MacOS. (see https://apple.stackexchange.com/questions/24262/what-is-the-difference-between-command-tool-and-sh-file-extensions) Having to support two identical launch scripts seems unnecessary. Maybe it'd be better to remove them? (question is addressed to maintainers)

I think MacOS reacts differently with the ".command" file extension.

so either copy the .sh to the .command file extension too, or make the .command call the .sh if able

@janAkali
Copy link
Author

janAkali commented Aug 29, 2024

Oh I missed that you was talking about forge.command that launches the desktop client. In my previous message I meant that we can remove two ".command" scripts: forge-gui-mobile-dev/src/main/config/forge-adventure.command and forge-adventure/src/main/config/forge-adventure-editor.command because they have "*_mac.sh" files beside them that are more up to date.

But with "forge.command" it seems like it's possible to copy "forge.sh" script with minimal changes. First, the she-bang (macos uses zsh shell instead of bash):

#!/usr/bin/zsh

I've tested this script both in bash and zsh (on Linux machine) and there's only one line that would work differently in zsh:

elif [[ -n $(type -p java) ]]

type in zsh returns a string "foo not found" even if it can't find the path, but we can fix it to check the exit code instead:

elif (type -p java)

or replace it with whence that's also builtin and doesn't return anything on fail:

elif [[ -n $(whence java) ]]

@Hanmac
Copy link
Contributor

Hanmac commented Aug 29, 2024

it seems we can still force MacOS to use sh for the *.command file, or would that be ignored?

#!/bin/sh
cd $(dirname "${0}")
java -Xmx4096m -Dfile.encoding=UTF-8 -jar $project.build.finalName$

Copy link

This PR has not been updated in a while nad has been marked on stale. Stale PRs will be auto closed

@Hanmac Hanmac added keep no stale and removed no-pr-activity labels Oct 15, 2024
@Hanmac
Copy link
Contributor

Hanmac commented Oct 16, 2024

@tehdiplomat @Agetian @kevlahnota can someone check this MR?

@kevlahnota
Copy link
Contributor

@Hanmac I think it's fine however once my PR is merged along with Java 17 update, the -illegal-access permit shouldn't work and should be removed and we need to use add opens paramater to cleanup the code.

@Agetian
Copy link
Contributor

Agetian commented Oct 16, 2024

Agreed with @kevlahnota 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
keep no stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants