runelite / launcher

Launcher for RuneLite
https://runelite.net
BSD 2-Clause "Simplified" License
65 stars 95 forks source link

Info.plist: account for macports and nix in PATH #111

Closed ylh closed 2 years ago

ylh commented 2 years ago

These are the bin paths used by the other two macOS package managers of note. With this, Macports and Nix users can let RL use their terminal-notifier installs too. In future, acquiring $PATH by running a login shell might be more robust, but for the time being, this should do.

Adam- commented 2 years ago

I am not a fan of adding a bunch of package manager specific paths to the Info.plist path, since it is not something which can be changed easily. I am a little surprised that just removing the path like we had prior to #90 doesn't work. Why doesn't it use the default path? Is there some other way to query for that and change the path of the process?

ylh commented 2 years ago

If you build a simple Cocoa demo app, launch it from the Finder, and make it display what it sees for PATH, all you get is /usr/bin:/bin:/usr/sbin:/sbin. That's even smaller than the PATH in the shell environment on a fresh install, which at the very least tacks /usr/local/bin on the front.

Two possibilities for “proper” solutions that would catch this stuff in all cases:

  1. replace arg lists such as: new String[]{"terminal-notifier", "-foo", "bar"} with: new String[]{"sh", "-lc", "terminal-notifier -foo bar"}

  2. subclass java.lang.ProcessBuilder to run sh -lc 'echo $PATH' then call self.environment().put("PATH", /* stdout from above command */); in the constructor, and use that everywhere

Considering this is only an issue on macOS (on Linux etc, the value of PATH in the gui usually includes wherever their distro's package manager shoves notify-send), I personally think restricting this silly special case to a macOS-specific file is fine, but if you'd prefer one of the above solutions, I can cook up a patch.

Adam- commented 2 years ago

Option 1 seems like the best solution to me.

ylh commented 2 years ago

PR put in for option 1.