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

installer: drop Windows installer build config #4405

Merged
merged 1 commit into from
Apr 3, 2022

Conversation

bastimeyer
Copy link
Member

Drop installer build config in favor of the external build config at:
https://github.com/streamlink/windows-installer

  • remove installer build scripts+configs
  • remove Streamlink config file in win32 subdirectory
  • remove windows-installer CI job
    • update steps of the release CI job
  • remove removed-plugins file
    • remove src/streamlink/plugins/.removed
    • update package manifest
    • update tests
    • update developing docs
  • rewrite install docs

Resolves #4390

Merging this will break @scowalt's chocolatey package (due to the new download location) and @beardypig's portable builds (due to the removed config file).

Now the important question, do we already want to remove the build config, or should we wait and do it after the next release? As you can see, I've published the 3.2.0-1 installer release, but we could keep both versions, the one here and the installers at the other repo, for a short while:
https://github.com/streamlink/windows-installer/releases/tag/3.2.0-1

This 3.2.0-1 installer release doesn't mean though that I'm 100% confident with it. I've only tested it in my W10 VM and didn't touch anything W7 related. The custom Python embedded builds are all working, but they are different from the official ones, as mentioned here, so there's lots of potential for unknown issues. And on top of that, the FFmpeg builds are new, the removed plugins file got replaced with a RMDir /r "$INSTDIR\pkgs" command before unpacking/copying files in the installer, and the default install path got changed in the x86_64 installers.

The reviews I've got so far were quick "It's working for me" comments, without any code / build setup reviews. Since I don't want to cause any issues for Windows users in the next release with the switch of the build config, I would really appreciate if one could at least take a deeper look and preferably build it themselves once. Also please see if something's unclear in the installer repo's readme and the docs changes here.

Btw, even though we're not building the nightly installer anymore, I've kept the scheduled CI runs, so that dependency release issues can be caught.

@bastimeyer
Copy link
Member Author

@gravyboat
Copy link
Member

Now the important question, do we already want to remove the build config, or should we wait and do it after the next release? As you can see, I've published the 3.2.0-1 installer release, but we could keep both versions, the one here and the installers at the other repo, for a short while:

Let's see if anyone else you've tagged weighs in, but my vote is to leave it for one more release with a deprecation note then drop it.

I've only tested it in my W10 VM and didn't touch anything W7 related.

I've been using it on Windows 10 since you released it without issue. W7 is not our concern, it's EoL software. If it breaks for someone that's their problem for running an outdated OS. We're not taking on the maintenance burden for an operating system the the creators no longer support.

In terms of other bugs we can add a release note requesting people try the new installer potentially? I'm not sure how many users will do that though.

Since I don't want to cause any issues for Windows users in the next release with the switch of the build config, I would really appreciate if one could at least take a deeper look and preferably build it themselves once. Also please see if something's unclear in the installer repo's readme and the docs changes here.

I will try to make time for this.

Drop installer build config in favor of the external build config at:
https://github.com/streamlink/windows-installer

- remove installer build scripts+configs
- remove Streamlink config file in win32 subdirectory
- remove windows-installer CI job
  - update steps of the release CI job
- remove removed-plugins file
  - remove src/streamlink/plugins/.removed
  - update package manifest
  - update tests
  - update developing docs
- rewrite install docs
@bastimeyer
Copy link
Member Author

A bit sad that there hasn't been a review yet... I'd really like to see this resolved, especially since the current stable release does include an older Python build with a vulnerable OpenSSL version, whereas the ones on streamlink/windows-installer do not. There has been a Python 3.10.4 release already, but this is just a minor bugfix for building on RHEL and is thus irrelevant for the Windows installer builds.

The "Windows installer" CI job is currently set as a requirement for merging, so this will have to be updated first. The rest should be fine now. I forgot to remove one job dependency in my initial push and it didn't trigger a CI run because of that. I thought that this was caused by the GH actions outage at this time, but I was wrong (hence my first force-push without any code changes).

Once again, I'd kindly ask anyone of you to give this a thorough review whenever you have the time.

Remember that this will remove the removed-plugins file, so any other PRs with plugin removals will cause merge conflicts as long as this PR here hasn't been merged yet.

@gravyboat
Copy link
Member

@bastimeyer I didn't have a chance to build the installer this weekend, planning on doing it tonight. After that I will mark as approved.

Copy link
Member

@gravyboat gravyboat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bastimeyer Alright I went through this tonight and it built fine on a linux VM that I spun up. I tried building it on a Mac as well to try and build on some real hardware but it was too big of a pain in the ass to get everything linked up so I gave up. It might be worth updating the docs to note you can run https://github.com/streamlink/windows-installer/blob/master/install-build-dependencies.sh manually if needed.

@bastimeyer
Copy link
Member Author

It might be worth updating the docs to note you can run https://github.com/streamlink/windows-installer/blob/master/install-build-dependencies.sh manually if needed.

It's meant for the CI, because it installs packages via apt, hence why it checks for the CI env var. I didn't put it into /.github, because it also installs pip dependencies, so people who want to build it themselves can see it deps. They should probably be put into a requirements file or pipenv lockfile though.

I tried building it on a Mac as well to try and build on some real hardware but it was too big of a pain in the ass to get everything linked up so I gave up.

All the build dependencies are on homebew:

@gravyboat
Copy link
Member

All the build dependencies are on homebew

I know. Setting up the proper version of Python and getting it linked up properly (pip, virtualenv, etc.) was the worst part. I don't care much about building it on mac any way. I just tried that first since I didn't want to spin up a linux VM. This can be merged if you're ready. Doesn't seem like anyone else is going to weigh in.

@gravyboat
Copy link
Member

@bastimeyer I don't think anyone else is going to look at this. What's left blocking this from a merge if the check is rerun?

@bastimeyer
Copy link
Member Author

bastimeyer commented Apr 2, 2022

There's nothing left (apart from changing the branch protection rule for being able to merge). Merging this will break @scowalt's chocolatey package and @beardypig's portable build, for the reasons mentioned above.

I was hoping for a review from at least @back-to, but there hasn't been any activity in the past days. I'm also not sure if you've reviewed the docs changes here, because of your previous comments.

Also, as said, I would appreicate if the streamlink/windows-installer repo could be added to your (plural) watch list, so I don't have to respond to issues on this repo there alone.

@gravyboat
Copy link
Member

I'm also not sure if you've reviewed the docs changes here, because of your previous comments.

Are you talking about the install.rst doc changes? I already looked at those. They're pretty minimal and seem fine, I wouldn't have approved if I hadn't looked at them.

Also, as said, I would appreicate if the streamlink/windows-installer repo could be added to your (plural) watch list, so I don't have to respond to issues on this repo there alone.

Done.

@bastimeyer
Copy link
Member Author

I've updated the branch protection rules of the master branch and removed the "Windows installer" requirement. It now also requires passing tests for py310 instead of py39.

Going to merge now...

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

Successfully merging this pull request may close these issues.

installer: move to separate repo, build multiple Python versions and archs
3 participants