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.

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

Use flatpak-spawn to run streamlink when running as a flatpak #3178

Merged
merged 21 commits into from
Aug 21, 2021
Merged

Use flatpak-spawn to run streamlink when running as a flatpak #3178

merged 21 commits into from
Aug 21, 2021

Conversation

ilya-zlobintsev
Copy link
Contributor

@ilya-zlobintsev ilya-zlobintsev commented Aug 17, 2021 •
edited
Loading

Pull request checklist:

  • CHANGELOG.md was updated, if applicable

Description

Currently, when Chatterino is installed as a Flatpak package, streamlink doesn't work because it is not included in the application runtime. This PR makes it so chatterino calls the streamlink command with flatpak-spawn --host when it detcets a flatpak runtime, which means the command will be called directly on the host machine, and not within the application's sandbox.

Fixes flathub/com.chatterino.chatterino#1

leon-richardt
leon-richardt requested changes Aug 17, 2021
View reviewed changes
Copy link
Collaborator

@leon-richardt leon-richardt left a comment

Choose a reason for hiding this comment

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

Very nice that you're looking into this 👍

I'm wondering though whether the logic for building the program call to open Streamlink via flatpak-spawn could also be integrated into getStreamlinkProgram()? I believe that could be nicer in regards to SoC and code reusability

src/common/Version.hpp Outdated Show resolved Hide resolved
@ilya-zlobintsev
Copy link
Contributor Author

Very nice that you're looking into this +1

I'm wondering though whether the logic for building the program call to open Streamlink via flatpak-spawn could also be integrated into getStreamlinkProgram()? I believe that could be nicer in regards to SoC and code reusability

The problem with integrating this into getStreamlinkProgram() is that QProcess takes in the binary name and the arguments separately. Normally the streamlink binary would be the actual binary name, but when running inside the sandbox the binary name has to be flatpak-spawn, with the streamlink path being an argument. It would be nice to centralize it into a single method, but in that case it would have to return both the binary and arguments (which would be empty outside of flatpak).

ilya-zlobintsev and others added 2 commits August 17, 2021 16:20
@ilya-zlobintsev @leon-richardt
Update src/common/Version.hpp
c453b41 
Co-authored-by: Leon Richardt <leon.richardt@gmail.com>
@ilya-zlobintsev
Make isFlatpak not const
694bdcd
leon-richardt
leon-richardt reviewed Aug 18, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@ilya-zlobintsev @leon-richardt
Update CHANGELOG.md
a487ab5 
Co-authored-by: Leon Richardt <leon.richardt@gmail.com>
Felanbird
Felanbird reviewed Aug 18, 2021
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
zneix
zneix reviewed Aug 19, 2021
View reviewed changes
src/util/StreamLink.cpp Outdated Show resolved Hide resolved
src/util/StreamLink.cpp Outdated Show resolved Hide resolved
ilya-zlobintsev and others added 2 commits August 19, 2021 22:11
@ilya-zlobintsev @zneix
Update src/util/StreamLink.cpp
72bc2c4 
Co-authored-by: Paweł <zneix@zneix.eu>
@ilya-zlobintsev @zneix
Update src/util/StreamLink.cpp
4ebc182 
Co-authored-by: Paweł <zneix@zneix.eu>
@ilya-zlobintsev ilya-zlobintsev mentioned this pull request Aug 20, 2021
Closed
@leon-richardt
ref: simplify spawning Streamlink process on Flatpak
3f1be3b 
As suggested in written communication with @ilyazzz, we now
incrementally build the Streamlink process by specifying the correct
path and adding the required arguments at different locations in the
code.

The correct path is `flatpak-spawn` when Chatterino is running as a
Flatpak or the direct path to the Streamlink executable otherwise.
Arguments are chosen depending on the context Streamlink is called from,
e.g. for fetching quality options or for opening a stream.

The `getStreamlinkProgram()` has been removed and its functionality has
been integrated in `createStreamlinkProcess()` instead.
@leon-richardt
Copy link
Collaborator

I have implemented the concept we discussed in chat in https://github.com/ilyazzz/chatterino2/pull/1. Please consider before merging

@ilya-zlobintsev
Merge pull request #1 from leon-richardt/ref/build-streamlink-process…
dc4714d 
…-depending-on-whether-flatpak-is-used-or-not

ref: simplify spawning Streamlink process on Flatpak
@ilya-zlobintsev
Merge branch 'master' into flatpak-streamlink
6161ece
leon-richardt
leon-richardt approved these changes Aug 21, 2021
View reviewed changes
Copy link
Collaborator

@leon-richardt leon-richardt left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Tested on non-Flatpak version, works as expected 👍 @ilyazzz verified it works on Flatpak as well. I would just like some additional input on that REVIEW comment (Update: Removed in 9d67b26 👌)

@leon-richardt
Copy link
Collaborator

Tests fail because we get a 502 on some requests to httpbin.org:

/home/runner/work/chatterino2/chatterino2/tests/src/NetworkRequest.cpp:139: Failure
Expected equality of these values:
  result.status()
    Which is: 502
  code
    Which is: 406

@pajlada pajlada enabled auto-merge (squash) August 21, 2021 10:40
@pajlada pajlada merged commit a7ef7e6 into Chatterino:master Aug 21, 2021
zneix added a commit to SevenTV/chatterino7 that referenced this pull request Aug 21, 2021
@zneix
Merge remote-tracking branch 'origin/master' into chatterino7
bf1af80 
Now we're on commit fe8aa33; Changes from upstream we've pulled:

- Minor: Added the ability to open an entire tab as a popup. (Chatterino#3082)
- Minor: Added optional parameter to /usercard command for opening a usercard in a different channel context. (Chatterino#3172)
- Bugfix: Fixed colored usernames sometimes not working. (Chatterino#3170)
- Bugfix: Allow starting Streamlink from Chatterino when running as a Flatpak. (Chatterino#3178)
D3XX3R added a commit to D3XX3R/chatterino7 that referenced this pull request Aug 22, 2021
@D3XX3R
Squashed commit of the following:
9dbddf3 
commit bf1af80
Merge: ba3b6dc fe8aa33
Author: zneix <zneix@zneix.eu>
Date:   Sat Aug 21 16:19:45 2021 +0200

    Merge remote-tracking branch 'origin/master' into chatterino7

    Now we're on commit fe8aa33; Changes from upstream we've pulled:

    - Minor: Added the ability to open an entire tab as a popup. (Chatterino#3082)
    - Minor: Added optional parameter to /usercard command for opening a usercard in a different channel context. (Chatterino#3172)
    - Bugfix: Fixed colored usernames sometimes not working. (Chatterino#3170)
    - Bugfix: Allow starting Streamlink from Chatterino when running as a Flatpak. (Chatterino#3178)

commit fe8aa33
Author: pajlada <rasmus.karlsson@pajlada.com>
Date:   Sat Aug 21 14:41:06 2021 +0200

    Update Usage messages to conform to new Usage message contributor guidelines (Chatterino#3180)

commit ad4a0c2
Author: Tal Neoran <talneoran@gmail.com>
Date:   Sat Aug 21 15:16:00 2021 +0300

    Add opening tab in popup (Chatterino#3082)

    Co-authored-by: zneix <zneix@zneix.eu>
    Co-authored-by: Rasmus Karlsson <rasmus.karlsson@pajlada.com>

commit 773c4bb
Author: LosFarmosCTL <80157503+LosFarmosCTL@users.noreply.github.com>
Date:   Sat Aug 21 13:37:57 2021 +0200

    Add optional parameter to /usercard command for opening a usercard in a different channel context. (Chatterino#3172)

    Co-authored-by: apa420 <17131426+apa420@users.noreply.github.com>
    Co-authored-by: Mm2PL <mm2pl+gh@kotmisia.pl>
    Co-authored-by: Leon Richardt <leon.richardt@gmail.com>

commit a7ef7e6
Author: ilyazzz <ilya.zl@protonmail.com>
Date:   Sat Aug 21 14:00:01 2021 +0300

    Use flatpak-spawn to run streamlink when running as a flatpak (Chatterino#3178)

    Co-authored-by: Leon Richardt <leon.richardt@gmail.com>
    Co-authored-by: Paweł <zneix@zneix.eu>

commit d7fd08b
Author: pajlada <rasmus.karlsson@pajlada.com>
Date:   Sat Aug 21 12:38:38 2021 +0200

    Fix color @usernames sometimes not working at all (Chatterino#3170)

    Definitely memory fuckery involved - The comment from @Lubieerror Chatterino#2822 (comment) is finally what led me to adding tests and hopefully fixing this.

commit 07454d0
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Thu Aug 19 21:59:46 2021 +0200

    Bump dangoslen/changelog-enforcer from 2.3.0 to 2.3.1 (Chatterino#3179)

    Bumps [dangoslen/changelog-enforcer](https://github.com/dangoslen/changelog-enforcer) from 2.3.0 to 2.3.1.
    - [Release notes](https://github.com/dangoslen/changelog-enforcer/releases)
    - [Changelog](https://github.com/dangoslen/changelog-enforcer/blob/master/CHANGELOG.md)
    - [Commits](dangoslen/changelog-enforcer@v2.3.0...v2.3.1)

    ---
    updated-dependencies:
    - dependency-name: dangoslen/changelog-enforcer
      dependency-type: direct:production
      update-type: version-update:semver-patch
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

commit f74b277
Author: apa420 <17131426+apa420@users.noreply.github.com>
Date:   Tue Aug 17 13:26:19 2021 +0200

    Browser extension issue template now properly redirects (Chatterino#3171)

commit f369511
Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Date:   Mon Aug 16 21:03:53 2021 +0200

    Bump dangoslen/changelog-enforcer from 2.2.0 to 2.3.0 (Chatterino#3174)

    Bumps [dangoslen/changelog-enforcer](https://github.com/dangoslen/changelog-enforcer) from 2.2.0 to 2.3.0.
    - [Release notes](https://github.com/dangoslen/changelog-enforcer/releases)
    - [Changelog](https://github.com/dangoslen/changelog-enforcer/blob/master/CHANGELOG.md)
    - [Commits](dangoslen/changelog-enforcer@v2.2.0...v2.3.0)

    ---
    updated-dependencies:
    - dependency-name: dangoslen/changelog-enforcer
      dependency-type: direct:production
      update-type: version-update:semver-minor
    ...

    Signed-off-by: dependabot[bot] <support@github.com>

    Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@zneix zneix mentioned this pull request Aug 22, 2021
Closed
@goldbattle goldbattle mentioned this pull request Sep 11, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Felanbird Felanbird Felanbird left review comments

@zneix zneix zneix approved these changes

@leon-richardt leon-richardt leon-richardt approved these changes

@pajlada pajlada pajlada approved these changes

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

Successfully merging this pull request may close these issues.

Unable to launch streamlink / media player
5 participants
@ilya-zlobintsev @leon-richardt @pajlada @Felanbird @zneix