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

feat(bundler): bundle additional gstreamer files, closes #4092 #4271

Merged
merged 6 commits into from
Jun 10, 2022

Conversation

FabianLars
Copy link
Member

A couple of things:

  1. i know it's weird to have a new appimage config like this, a more obvious choice would be to have a new "parent" linux config for both appimage and deb (especially for consistency between OSes). I'm happy with changing it, i just didn't want to start this PR with a breaking change.
  2. idk how to call the flag, so i just went with gstreamer for now.
  3. It currently should fail to build on non ubuntu-like systems because i didn't modify the gstreamer plugin yet, but since this is not bound to tauri's release pipeline i thought it be better to at least get the PR out asap.
  4. The fix that worked ~3 weeks ago doesn't really work anymore. It still fixes app crashes but it doesn't play videos on youtube and twitch (some other pages like https://camendesign.com/code/video_for_everybody/test.html work fine tho), it could very well be my system's fault because it's acting up generally right now. I'll do some VM testing when i make changes to the plugin (tomorrow)
  5. As of now the plugin re-introduces the patchelf dependency and i'm not 100% sure if we will/can get rid of it (didn't really look at it).

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • Not yet, see 1)

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

@FabianLars FabianLars requested review from a team June 4, 2022 23:46
@FabianLars FabianLars requested a review from a team as a code owner June 4, 2022 23:46
@lucasfernog
Copy link
Member

  1. I don't think it's weird to have an AppImage config tbh.
  2. the flag could name could be bundleMediaFramework or something like that, gstreamer doesn't seem that bad too - i also hate naming things

@FabianLars
Copy link
Member Author

1 Yeah i'm happy with an appimage config too, it's just that i keep comparing it to windows > wix in my head. And then i think about potential rpm and flatpak props too. Well we can worry about that for v2 i guess 🤷

2 i like bundleMediaFramework, should be clearer for non-linux people etc. Unless we want to keep it synced with the env var because i'd prefer that to still be called something with gstreamer

@lucasfernog
Copy link
Member

Yeah let's worry about reorganizing the config options in v2. And we don't need to sync with the env var, makes more sense to keep using gstreamer since the bundler is also used by non-tauri users.

@lucasfernog
Copy link
Member

@FabianLars is this one ready to be merged or do you want to change something?

@FabianLars
Copy link
Member Author

FabianLars commented Jun 10, 2022

This PR won't receive any more changes, only the plugin itself will, to make it work on more distros (and hopefully fix the youtube&twitch issue again)

@lucasfernog lucasfernog merged commit d335fae into dev Jun 10, 2022
@lucasfernog lucasfernog deleted the feat/appimage-bundle-gstreamer branch June 10, 2022 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants