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

Build additional "streamlinkw" launcher on Windows #2326

Merged
merged 2 commits into from Mar 7, 2019

Conversation

Projects
None yet
3 participants
@bastimeyer
Copy link
Member

bastimeyer commented Feb 25, 2019

This adds an additional streamlink-noconsole.exe "GUI" launcher to the Windows installer and the Streamlink python wheel. This launcher uses pythonw.exe under the hood, so that a new terminal window doesn't get opened if executed from a non-terminal context on Windows.

Resolves #2287, #2308


Motivation of this PR and why this change is needed

A couple of months ago, both pip and pynsist changed their entry point launchers on Windows from setuptools to simple_launcher. This change broke the Streamlink resolver in Streamlink Twitch GUI, as these new launchers are lacking the additional streamlink-script.py file, which is used for parsing its shebang and getting the path to pythonw.exe, which is needed to suppress the terminal window from being opened. Unfortunately, it is impossible to work around the new launchers without parsing them or asking the user to manually set custom paths. That is however not a real solution.

Fortunately though, simple_launcher does provide two different versions of the launchers, a "console" one and a "GUI" one. Setuptools does this as well, but their GUI launcher is useless, as its process terminates immediately and therefore can't be used. The new simple_launcher-based launchers however work exactly as they should and the GUI launcher can be used by GUI applications like Streamlink Twitch GUI without any issues.

That's why I'm submitting this pull request, which adds streamlink-noconsole as a new additional GUI launcher.

I've already submitted a PR to pynist (takluyver/pynsist#179) a couple of days ago which adds a new console=false option to the installer's "command" section, so that a GUI launcher can be built. My PR was merged a couple of hours ago, but since a new release hasn't been published yet, it will be installed from the master branch for now (see the TravisCI config changes).

Regarding the Streamlink python wheel, an additional gui_scripts entry point can be added to setup.py, so that pip will also build a GUI launcher.


Issues / Open questions

  • Since I don't know much about python wheels and python packaging in general, I guess that this change will also add a streamlink-noconsole on Linux and macOS. Is it possible to change that?
  • Is the name streamlink-noconsole okay? Or does anybody have a better idea?
  • Regular installs via python setup.py install will still build the useless GUI launcher.
  • I've noticed some issues with certain plugins while using the GUI launcher, but I don't know if it was because of the build env in my Windows VM or something else. This has to be resolved before this PR can get merged. (I will check this tomorrow)

@bastimeyer bastimeyer referenced this pull request Feb 25, 2019

Open

Missing streamlink-script.py on Windows #618

2 of 2 tasks complete

@bastimeyer bastimeyer force-pushed the bastimeyer:streamlink-noconsole branch from d28e8d8 to bb222eb Feb 25, 2019

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 25, 2019

Codecov Report

Merging #2326 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2326   +/-   ##
=======================================
  Coverage   52.59%   52.59%           
=======================================
  Files         237      237           
  Lines       14797    14797           
=======================================
  Hits         7783     7783           
  Misses       7014     7014
@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 25, 2019

Codecov Report

Merging #2326 into master will decrease coverage by 1.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2326      +/-   ##
==========================================
- Coverage   52.59%   51.37%   -1.22%     
==========================================
  Files         237      237              
  Lines       14797    14469     -328     
==========================================
- Hits         7783     7434     -349     
- Misses       7014     7035      +21
@beardypig

This comment has been minimized.

Copy link
Member

beardypig commented Feb 25, 2019

What does console=false actually do? Can we just set it for the main streamlink binary?

@bastimeyer

This comment has been minimized.

Copy link
Member Author

bastimeyer commented Feb 26, 2019

What does console=false actually do?

What it actually does? See takluyver/pynsist#179
TL;DR it builds the same so-called "GUI"/"no console" launcher as the gui_scripts entry points when a wheel gets installed via pip.
https://packaging.python.org/specifications/entry-points/#use-for-scripts

Again, and I don't want to repeat myself over and over 😩, this is necessary to prevent a terminal window from being opened in a non-terminal context on Windows. This "GUI" launcher uses pythonw.exe instead of python.exe and properly redirects the i/o streams.

Can we just set it for the main streamlink binary?

No, because that would break situations where a terminal window is desired/needed, like for example when launching streamlink.exe from shortcuts. We would also have to switch the console_scripts entry point to gui_scripts.

If python on Windows wouldn't have this stupid distinction between python/pythonw, this all wouldn't be necessary.


I've been trying to solve this mess for the past two weeks now and adding an additional "GUI" launcher is the only way to properly and sensibly fix this. Otherwise, every non-CLI application using Streamlink that doesn't want a terminal window from being shown would have to parse the binary launcher executable for the included shebang, so that the path to the python executable can be found and replaced with pythonw. This is nonsense.

There is no harm in adding an additional executable to the Windows installer. The wheel however is a bit different, because it also affects Linux and macOS. So if a different flavor can't be built and published explicitly for Windows, we can remove the gui_scripts entry point from setup.py again if you want. This isn't too much of a problem for me and the Twitch GUI, because the incompatible launcher can be ignored in pip via --no-binary streamlink and an old regular entry script will be created instead.

@beardypig

This comment has been minimized.

Copy link
Member

beardypig commented Feb 26, 2019

Yeah, I got all that from the previous issues, I was just wondering if a regular python.exe console script was even required - sounds like it is.

If we build separate wheels for each platform we could probably include it just for Windows, but as we are building universal wheels I don't think we can do it as-is. Maybe @back-to knows more.

I'm fine with the name, but if you want another suggestion streamlinkw :p

@bastimeyer

This comment has been minimized.

Copy link
Member Author

bastimeyer commented Feb 26, 2019

https://packaging.python.org/guides/distributing-packages-using-setuptools/#platform-wheels

We just need to build an additional Windows specific wheel. That will be selected first on Windows, so there's no need to change the current config.

Would it make sense copying setup.py to a temporary location, modifying it and renaming the output wheel name? Or is there an easier/better way for building platform specific wheels from one location?

streamlinkw

That's probably better :)

@bastimeyer

This comment has been minimized.

Copy link
Member Author

bastimeyer commented Feb 27, 2019

Looks like we need to build Windows specific wheels for win32 and win-amd64 (python setup.py bdist_wheel --plat-name=XYZ). Probably also cygwin. 😕

https://www.python.org/dev/peps/pep-0425/#id1

https://github.com/pypa/wheel/blob/0.33.1/wheel/bdist_wheel.py#L49-L51
https://github.com/pypa/wheel/blob/0.33.1/wheel/bdist_wheel.py#L111-L119
https://github.com/pypa/wheel/blob/0.33.1/wheel/bdist_wheel.py#L148-L156
https://github.com/python/cpython/blob/v3.7.2/Lib/distutils/util.py#L18-L91

Regarding the actual build procedure, would it make sense adding a diff/patch file for setup.py that gets applied after building the non-platform-specific wheel and then building the specific ones? Before I update this pull request, I'd like to get some feedback first whether this is the right path to take here.

Btw, the scripts/sdistsign.sh script name doesn't make sense if a wheel also gets built 😅

@beardypig

This comment has been minimized.

Copy link
Member

beardypig commented Feb 27, 2019

I'm not too fussed having the streamlink-noconsole console script in the universal wheel, I imagine no one will notice :)

Update: I checked and they behave identically on linux and macOS. I don't know of debian/arch/etc would have an issue with the extra binary though...

Build additional "streamlinkw" launcher on Windows
1/2: Windows installer (using a pre-release version of pynsist)
@bastimeyer

This comment has been minimized.

Copy link
Member Author

bastimeyer commented Feb 28, 2019

Let's not add this to Linux and macOS. Remember that this will add a duplicate entry script to the user's PATH and will make tab-completion a bit annoying.

Regarding my patch file approach, this unfortunately won't work because of versioneer. The resulted version string will have a dirty tag added because of the modified setup file. This means that the file either has to be duplicated (which is kinda stupid), or modified with custom logic. Since the --plat-name parameter has to be set for the bdist_wheel command, it would make sense making use of it in the entry_points field in order to see whether the wheel gets built for Windows or not.

Build additional "streamlinkw" launcher on Windows
2/2: Windows specific wheels

@bastimeyer bastimeyer force-pushed the bastimeyer:streamlink-noconsole branch from bb222eb to 6869254 Feb 28, 2019

@bastimeyer bastimeyer changed the title Build additional "streamlink-noconsole" launcher Build additional "streamlinkw" launcher on Windows Feb 28, 2019

@bastimeyer bastimeyer requested a review from beardypig Mar 1, 2019

@bastimeyer

This comment has been minimized.

Copy link
Member Author

bastimeyer commented Mar 5, 2019

@beardypig Could you please review these changes when you get the time? Since you're the one who wrote the sdistsign script, I would like to have it reviewed by you.

I could split the PR into two, one for the installer and one for the wheels if that's better or if you don't have the time right now. The installer changes should be a no-brainer and could get merged immediately.

I would really like to have this merged and a new release published, because the Twitch GUI is in an awkward situation on Windows since Streamlink's 1.0.0 release.

@beardypig
Copy link
Member

beardypig left a comment

Looks fine, I couldn't see a better way to do is_wheel_for_windows :)

@gravyboat

This comment has been minimized.

Copy link
Member

gravyboat commented Mar 7, 2019

Looks fine to me as well on a quick review so I'm going to merge, thanks @bastimeyer.

@gravyboat gravyboat merged commit e96c725 into streamlink:master Mar 7, 2019

3 checks passed

codecov/project 52.59% (target 30%)
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bastimeyer bastimeyer referenced this pull request Mar 9, 2019

Open

1.1.0 release #2349

jackyzy823 added a commit to jackyzy823/streamlink that referenced this pull request Mar 20, 2019

Build additional "streamlinkw" launcher on Windows (streamlink#2326)
* Build additional "streamlinkw" launcher on Windows

1/2: Windows installer (using a pre-release version of pynsist)

* Build additional "streamlinkw" launcher on Windows

2/2: Windows specific wheels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.