woodruffw / ff2mpv

A Firefox/Chrome add-on for playing URLs in mpv.
https://addons.mozilla.org/en-US/firefox/addon/ff2mpv/
Other
521 stars 52 forks source link

Work around PATH issue on Darwin #38

Closed Atemu closed 3 years ago

Atemu commented 3 years ago

Fixes #13

Tested on both Darwin and Linux with MPV installed to ~/.nix-profile/bin/mpv and /run/current-system/sw/bin/mpv respectively.

Atemu commented 3 years ago

Hmm, black would like me to

-    new_env["PATH"] = f"{new_env['PATH']}:{os.getenv('HOME')}/.nix-profile/bin:/run/current-system/sw/bin:/usr/bin:/usr/local/bin:/opt/bin:/opt/local/bin"
+    new_env[
+        "PATH"
+    ] = f"{new_env['PATH']}:{os.getenv('HOME')}/.nix-profile/bin:/run/current-system/sw/bin:/usr/bin:/usr/local/bin:/opt/bin:/opt/local/bin"

for some reason but that doesn't look right. I'm not very experienced in python, how should this be formatted?

woodruffw commented 3 years ago

Thanks for these changes, but I don't want to handle this on the ff2mpv side: users that want arbitrary processes (Firefox) to spawn other arbitrary processes (MPV and youtube-dl) should make an informed decision and place the latter on their system paths via /etc/paths or /etc/paths.d (or potentially ~/.launchd.conf on macOS).

Doing this for them on an ad-hoc basis violates POLA and introduces maintenance and debugging burdens.

Thanks again, and sorry.

woodruffw commented 3 years ago

for some reason but that doesn't look right. I'm not very experienced in python, how should this be formatted?

The best thing to do would probably be to break it up into smaller expressions. I don't plan on merging this, but something like:

_EXTRA_PATHS = ["foo", "bar", "baz"]

# ... snip ...

new_env["PATH"].append(":".join(_EXTRA_PATHS))

That's roughly how I'd do it.

Atemu commented 3 years ago

I'd love to follow the POLA but, in this case, there isn't much else I could do.

Unfortunately, none of the solutions you mentioned work on modern macOS anymore and the only (supposedly) working solution I could find is to set environment variables imperatively by executing launchctl setenv ENVVNAME value before your GUI application starts which sounds flaky at best.

Atemu commented 3 years ago

Another possible fix I just thought of would be to allow the user to set the environment variables in a config file read by the script. Adding such a config file may not be a bad idea anyways as there might be other things a user would want to configure about ff2mpv (MPV flags for example).

woodruffw commented 3 years ago

Unfortunately, none of the solutions you mentioned work on modern macOS anymore

I wasn't aware of this. Do you happen to have a link or some docs for the change? I thought that /etc/paths and /etc/paths.d at the minimum would work, since /etc doesn't fall under SIP. But it's completely possible that Apple has (once again) changed things underneath me.

Another possible fix I just thought of would be to allow the user to set the environment variables in a config file read by the script. Adding such a config file may not be a bad idea anyways as there might be other things a user would want to configure about ff2mpv (MPV flags for example).

I think a config file would be acceptable. That being said, I think the right approach with a config might just be to have the user configure their exact MPV path, similar to how the wiki currently documents using ff2mpv with MPV when it's installed as an .app.

As far as other configuration goes: I want to minimize the number of knobs and dials that can be turned on ff2mpv itself -- users who need non-default behavior should modify their MPV and youtube-dl configs. That being said, we could allow users to specify a custom MPV profile, which they could then use to customize both MPV and youtube-dl.

Atemu commented 3 years ago

Do you happen to have a link or some docs for the change?

I whish I had docs for any of this. This is me going off of dozens of stackexchange answers and frustrated commenters.

/etc/paths.d

That's for shells though, right? We need those environment variables in whatever launches Firefox (Spotlight? Finder?), not bash.

I think the right approach with a config might just be to have the user configure their exact MPV path, similar to how the wiki currently documents using ff2mpv with MPV when it's installed as an .app.

I think that's also a perfect candidate to be spun out to the config file; asking a user to edit source code is a bit much IMO ;)

Something like

{
  "mpv": "/Applications/mpv.app/Contents/MacOS/mpv"
}

in ~/.config/mpv/ff2mpv.json (or a better config format that doesn't add external dependencies) would be super simple and easy.

we could allow users to specify a custom MPV profile, which they could then use to customize both MPV and youtube-dl

I think allowing custom flags would be simpler. The user can then choose to add flags which launch MPV with a certain profile or even create an implicit profile for ff2mpv by just specifying a few flags it should launch mpv with, whichever they prefer.

The user would be breaking it on their own account it if they added bad flags.

woodruffw commented 3 years ago

That's for shells though, right? We need those environment variables in whatever launches Firefox (Spotlight? Finder?), not bash.

It's been a while, but my recollection was that macOS also loaded /etc/paths.d on graphical login/WindowServer start. But that was about a decade ago, and it looks like that's no longer the case. Oh well.

in ~/.config/mpv/ff2mpv.json (or a better config format that doesn't add external dependencies) would be super simple and easy.

This sounds reasonable to me. JSON is a decent choice, since both Python and Ruby have core/stdlib support for it.