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

PYTHONPATH fix on macOS actually breaks it depending on configuration #276

Closed
comex opened this issue Mar 19, 2020 · 1 comment
Closed
Assignees
Labels

Comments

@comex
Copy link

comex commented Mar 19, 2020

commit c07206c18992c1dca401b30a01b9f0fe54a71df5
Author: Alberto Sottile <alby128@gmail.com>
Date:   Thu Mar 14 15:45:55 2019 +0100

    Run mpv subprocess with a system PYTHONPATH env on macOS.
    
    youtube-dl relies on system python to run on macOS, but system
    python cannot be executed in a subprocess created from the frozen
    python3 executable. This makes youtube-dl unusable from frozen
    versions of Syncplay on macOS. So, the environment of the mpv
    subprocess is modified with system PYTHONPATH, allowing the
    execution of /usr/bin/python (and hence, of youtube-dl)
    from within the subprocess in frozen executables.

This sets PYTHONPATH to a a value determined by running /usr/bin/python and extracting sys.path, as well as hardcoding PATH to /usr/bin:/usr/local/bin.

It seems that Homebrew does install youtube-dl using /usr/bin/python. But youtube-dl can be installed with any installation of Python, e.g. using pip install youtube-dl or by cloning the repository and running python setup.py install. For that matter, it can also be installed at any path; I'd expect Syncplay to respect the existing PATH variable.

In my case, I have youtube-dl installed as /usr/local/bin/youtube-dl, so Syncplay does see it even with the hardcoded PATH. But the shebang points to /usr/local/bin/python3, the copy of Python I installed it with. When mpv invokes it with PYTHONPATH containing a bunch of paths from the system Python 2.7, it naturally fails to start.

It shouldn't be necessary to set PYTHONPATH at all; after all, it's not like you normally have to manually set PYTHONPATH before running youtube-dl. Instead, I believe the original problem this commit was addressing arose from the fact that Syncplay's frozen Python 3 itself sets PYTHONPATH, and this was inadvertently inherited by youtube-dl. A better approach is to simply unset PYTHONPATH before running youtube-dl. (Or for maximum correctness, figure out what the PYTHONPATH environment variable was originally set to before frozen Python overwrote it, but that may be difficult.)

As for PATH, it doesn't seem like frozen Python modifies it, but I guess the desire is to find youtube-dl in /usr/local/bin even though macOS normally runs apps with PATH set to /usr/bin:/bin:/usr/sbin:/sbin (though it's alterable). The goal makes sense, but some users may use other paths, especially (but not only) when not using frozen Python – I normally run Syncplay from terminal. Instead of replacing the entire PATH, Syncplay should probably just add /usr/local to the end of PATH, so the user's paths take precedence. Also, PATH alteration should probably only happen at all when running under frozen Python; the current code enables it whenever isMacOS() returns true. (I normally run Syncplay directly from a Git checkout using python3 syncplayClient.py.)

comex added a commit to comex/syncplay that referenced this issue Mar 19, 2020
- Only apply when actually running under py2app, as opposed to whenever
  running on macOS.
- Instead of setting PATH to a fully hardcoded value, just append
  /usr/local/bin if not already present.  That way any non-default
  choices by the user are respected.
- Unset PYTHONPATH instead of setting it based on /usr/bin/python's
  sys.path.  mpv or other players could end up invoking any copy of
  Python, so there's no sensible value to set it to, but it's (normally)
  fine to leave it blank.  The important thing is that we don't keep the
  value set by py2app, pointing to our own Contents/Resources directory.
  (It should be safe to change PYTHONPATH at this point without
  affecting Syncplay's own execution, since Python only checks it at
  startup.)
- Unset other Python environment variables instead of just PYTHONPATH,
  because py2app sets a number of them (including
  PYTHONDONTWRITEBYTECODE and PYTHONHOME).
- Do the environment variable patchup when starting Syncplay, so it
  applies to all players rather than just mplayer-based ones.

Fixes Syncplay#276.
@albertosottile
Copy link
Member

albertosottile commented Oct 17, 2020

I am trying to find a fix that would work with:

  • A) homebrew mpv and youtube-dl
  • B) vanilla youtube-dl and any other binary of mpv
  • C) a custom built youtube-dl and any other binary of mpv

What I wrote in the commit message still stands, though it is restricted to scenario A). In this scenario, the "environment variable patchup" is needed also when Syncplay is started from the command line. The issue lies in the homebrew build of youtube-dl entirely. Hence, restricting the alteration of the mpv environment to frozen apps will not solve the issue.

Scenario B) works out of the box with the current Syncplay and system Python, as the shebang is #!/usr/bin/env python (see https://github.com/ytdl-org/youtube-dl/blob/4eda10499e8db831167062b0e0dbc7d10d34c1f9/bin/youtube-dl#L1). I think the "environment variable patchup" is not really relevant, in this case, but certainly it does not break the functionality.

To fix this issue for scenario C), we would need to add something in our code to identify which version of youtube-dl is installed and which version/location of Python is needed for it to function, perhaps by finding the script, reading its shebang, and configuring the mpv environment accordingly. We would need to scan the user's system to find out the Python executables available and which of those are capable of running the installed version of youtube-dl. We would probably need something more that I am missing at the moment. All of this code will probably be not so easy to write and almost impossible to maintain, as it would require a very custom test setup.

In summary, I think that the only viable solution for us is to state this issue among the known issues/limitations of Syncplay and advise to use the homebrew version of mpv or to install youtube-dl using the instructions on the official site. In your specific case, I would advise you to do the latter.

For the reasons stated above, I am closing this issue now. Please feel free to comment here or reopen it anytime.

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

Successfully merging a pull request may close this issue.

2 participants