po5 / mpv_sponsorblock

mpv script to skip sponsored segments of YouTube videos
GNU General Public License v3.0
530 stars 29 forks source link

`python` invokes Python 2 on some systems. #6

Closed rydalt closed 4 years ago

rydalt commented 4 years ago

That's not how windows executables work at all, you don't have to specify the .exe.

Please don't try to lecture others when you've got no idea how something works.

Will implement the solution discussed earlier as it's seemingly the best we'll ever come up with.

_Originally posted by @po5 in https://github.com/po5/mpv_sponsorblock/pull/4#issuecomment-584981127_

Since you locked that I couldn't properly reply, so I apologize for opening a new issue. I literally set this up on Windows using @jgkamat's patch. It worked.

While I cannot find official documentation, Windows will run commands without the .exe extension as long as they are in the PATH. It even works when you invoke external programs programmatically.

The most proof outside of my own word I could find was this superuser issue where adding an item to a path is one of the solutions offered to run a binary.


Also an actual issue that just occurred to me, the lua script needs to detect if python is installed, else it just fails. I learned this due to the fact Python actually installs a shim these days that just opens the Windows Store and tells you to download Python 3 from there.

po5 commented 4 years ago

I tried @jgkamat's patch on my own Windows installation. Case in point, it didn't work. I've never had complaints from Windows users, so it must be working as it is now, right? I don't want to be flooded with issues stating a recent update broke everything, I will not be merging this patch.

The script not working when requirements aren't met is intended behavior.

jgkamat commented 4 years ago

I've never had complaints from Windows users, so it must be working as it is now, right?

This logic dosen't make sense to me. Case in point, I tried this script, and it wasn't working on my system. I was going to ignore it as a broken script and move on but I happened to recognize the error.

Reading some more docs: https://docs.python.org/3/using/windows.html

If no version qualifiers are found in a command, the environment variable PY_PYTHON can be set to specify the default version qualifier. If it is not set, the default is “3”. The variable can specify any value that may be passed on the command line, such as “3”, “3.7”, “3.7-32” or “3.7-64”. (Note that the “-64” option is only available with the launcher included with Python 3.7 or newer.)

So on an unconfigured system (on windows, with the official python package (?)) python should probably (?) be fine. But users setting PY_PYTHON will probably still have this script broken. You might be able to use the py launcher with -3 as an argument to ensure python3 as well. I don't mind personally what you do as I still don't have a windows box, but just something to keep in mind.

However, the docs are a little contradictory, and offer this example saying that python should link to python2 on windows (?)

If no relevant options are set, the commands python and python2 will use the latest Python 2.x version installed and the command python3 will use the latest Python 3.x installed.

(but it later says)

If PY_PYTHON=3, the commands python and python3 will both use the latest installed Python 3 version.

And the default of PY_PYTHON is 3 per the first section, so...

po5 commented 4 years ago

I've never had complaints from Windows users, so it must be working as it is now, right?

This logic dosen't make sense to me. Case in point, I tried this script, and it wasn't working on my system.

You're not a windows user from what I gathered, so you were experiencing an actual issue that I've acknowledged and fixed.

The py launcher is optional, I can't expect users to have it installed.

Is the fix working for you?

jgkamat commented 4 years ago

Is the fix working for you?

Yes, it seems to be. I'm getting a couple errors regarding sqlite but I don't have the time to look into that at the minute.

po5 commented 4 years ago

Interested in the sqlite stuff, please open a new issue if you can figure out what's going on.

Gonna lock this one since the fix is working.