Skip to content

installer: external assets #2916

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 20, 2020
Merged

installer: external assets #2916

merged 3 commits into from
Apr 20, 2020

Conversation

bastimeyer
Copy link
Member

@bastimeyer bastimeyer commented Apr 17, 2020 •
edited
Loading

Resolves #2872
and upgrades FFMpeg to 4.2.2 (Zeranoe)

Changes

  1. Rewrite makeinstaller.sh
    • Move removed plugins list into a text file and read from it
    • Add log and err functions
    • Define build dependencies
    • Set repo root directory instead of assuming it
    • Clean up build directory before building the installer
    • Set dist dir to same STREAMLINK_DIST_DIR
    • Suppress warning/error messages from inkscape and imagemagick
    • Replace some strings in pynsist config with variables
    • Update python license text according to the currently used version
  2. Download assets from streamlink/streamlink-assets
    • Download metadata from latest assets release
    • Download assets into cache dir (useful for re-building locally)
    • Compare checksums
    • Extract specific files following the metadata
  3. Finally remove binary files from the repo

beardypig
beardypig reviewed Apr 17, 2020
View reviewed changes
beardypig
beardypig reviewed Apr 17, 2020
View reviewed changes
beardypig
beardypig reviewed Apr 17, 2020
View reviewed changes
beardypig
beardypig reviewed Apr 17, 2020
View reviewed changes
beardypig
beardypig reviewed Apr 17, 2020
View reviewed changes
@bastimeyer bastimeyer force-pushed the installer/external-assets branch 2 times, most recently from e041a13 to 8c4d65d Compare April 17, 2020 16:35
@bastimeyer
Copy link
Member Author

The log output on Github actions is so useless. It's scrambling the lines for some reason, even in the raw log.

@bastimeyer
Copy link
Member Author

What about the python license file? Do we need to include it? I feel like this is pynsist's job.

@beardypig
Copy link
Member

It is pynsist's job. But it probably should be included, depends on what the licence says :)

@bastimeyer
Copy link
Member Author

https://github.com/streamlink/streamlink/pull/2916/files#diff-6871e707e71cec9606ed3e5534cc4516

Ok, that should probably not be deleted, but instead fixed up.

@bastimeyer bastimeyer force-pushed the installer/external-assets branch from 8c4d65d to 9fde3ce Compare April 17, 2020 17:28
@bastimeyer
Copy link
Member Author

Restored win32/LICENSE.txt, fixed typo, fixed python version (would probably be wise turning it into a template in the future when the python version gets bumped again) and removed the redundant RTMPDump license text.

Still not sure how to feel about the removal of the license files of the libs bundled by FFmpeg. The zeranoe build archive should include these, but it doesn't. Maybe include this?
https://ffmpeg.zeranoe.com/builds/readme/win64/static/ffmpeg-4.2.2-win64-static-readme.txt
Would require a quick update of the assets repo and a fix here, as it always expects zip archives.

back-to
back-to reviewed Apr 17, 2020
View reviewed changes
@bastimeyer bastimeyer force-pushed the installer/external-assets branch from 9fde3ce to 5b2165c Compare April 17, 2020 18:39
@bastimeyer
Copy link
Member Author

I've submitted streamlink/streamlink-assets#2 earlier today which should resolve the ffmpeg license issue. This needs to be merged first before I can push my changes here.

Btw, also did a bit of research regarding the Python license and from what it looks like, it's simply missing from the python-3.6.6-embed-win32.zip file pynsist downloads + extracts. The 3.8.2 zip archive includes a license file, so once the installer's python version gets bumped (not in this PR), the custom license text can be removed.

@bastimeyer bastimeyer force-pushed the installer/external-assets branch from 5b2165c to 37e7eec Compare April 19, 2020 10:54
@bastimeyer
Copy link
Member Author

The ffmpeg build info is now included and the assets related log output a bit more verbose:
https://github.com/streamlink/streamlink/runs/599289704?check_suite_focus=true#step:7:308

That should be it regarding the external assets and their licenses.

Btw, pynsist 2.5 - we're currently using 2.4 - has some optimizations regarding the assembly of the executable launchers. Those are now done at build time and the result is that the install directory is much cleaner. Also includes high-DPI scaling fixes. Could be explored in another PR.

@beardypig
Copy link
Member

Have you made all the changes you want to make for this PR @bastimeyer?

@bastimeyer
Copy link
Member Author

Unless you've spotted another issue, I think it's ready to be merged.

@beardypig
Copy link
Member

Looks good to me, let's see if @back-to can spot anything :D

@bastimeyer
Copy link
Member Author

Ehm, do we even include Streamlink's license in the installer? I don't think so...

Redistributions in binary form must reproduce the above copyright notice,
this list of conditions and the following disclaimer in the documentation
and/or other materials provided with the distribution.

@bastimeyer bastimeyer force-pushed the installer/external-assets branch from 37e7eec to 5e64f29 Compare April 19, 2020 16:55
back-to
back-to reviewed Apr 19, 2020
View reviewed changes
@bastimeyer
installer: rewrite / clean up makeinstaller script
a233780 
- Move removed plugins list into a text file and read from it
- Add log and err functions
- Define build dependencies
- Set repo root directory instead of assuming it
- Clean up build directory before building the installer
- Set dist dir to same STREAMLINK_DIST_DIR
- Suppress warning/error messages from inkscape and imagemagick
- Replace some strings in pynsist config with variables
- Add Streamlink license file to pynsist config
- Update python license text according to the currently used version
- Remove unneeded pbs license text
@bastimeyer
installer: download ffmpeg+rtmpdump assets
f43d643 
from streamlink/streamlink-assets
@bastimeyer
installer: delete locally included binary files
ed6bbb3
@bastimeyer bastimeyer force-pushed the installer/external-assets branch from 5e64f29 to ed6bbb3 Compare April 19, 2020 18:13
@gravyboat
Copy link
Member

Changes look good to me. 👍

@back-to back-to merged commit e55401a into streamlink:master Apr 20, 2020
mkbloke pushed a commit to mkbloke/streamlink that referenced this pull request Aug 18, 2020
@back-to
Merge pull request streamlink#2916 from bastimeyer/installer/external…
5080c75 
…-assets

installer: external assets
@bastimeyer bastimeyer deleted the installer/external-assets branch January 19, 2021 23:22
@back-to back-to mentioned this pull request Feb 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@back-to back-to back-to left review comments

@gravyboat gravyboat gravyboat approved these changes

@beardypig beardypig beardypig approved these changes

Assignees
No one assigned
Labels
Windows installer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update ffmpeg included in Windows installer
4 participants
@bastimeyer @beardypig @gravyboat @back-to