-
Notifications
You must be signed in to change notification settings - Fork 7
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
Use flatpak-spawn to run Google Chrome #13
base: master
Are you sure you want to change the base?
Conversation
546b7f4
to
6b489e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually a lot simpler than I thought! Maybe too simple. Couple of tweaks to do. :)
@@ -3,26 +3,18 @@ | |||
"runtime": "org.freedesktop.Platform", | |||
"runtime-version": "1.6", | |||
"sdk": "org.freedesktop.Sdk", | |||
"command": "/app/bin/eos-google-chrome-app", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The launcher in the OS depends on this entry point existing, so I don't think we should change it (or we provide a compat symlink) - https://github.com/endlessm/eos-browser-tools/blob/master/chrome-helper/eos-google-chrome.py#L86
This means the script needs to handle two things - either running inside the flatpak and doing the spawn trick, or outside and maybe just re-execing itself with flatpak, for consistency (and no need to do the pipe that keeps the flatpak alive etc).
To rely on the regular launch behaviour in the OS, we can use an eufi "upgrade" method in the OS so that we can drop the launcher hacks, but we can't sequence the upgrade the other way (prevent an older OS getting a new Flatpak).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My idea for the transition was that the "self-contained" version of the app was only ever hosted on Flathub, not on eos-apps. (In that sense this PR is more of a RFC than something to merge right away here.)
So older OS would never get this new one and we would not worry about keeping the legacy launcher; in the same OS update we would remove all the launcher stuff and add the migration to the Flathub version.
Is that possible?
@@ -0,0 +1,6 @@ | |||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It always starts with a simple/innocent shell script... and then... :P Maybe it's fine. I would keep a low threshold for doing a Python ver if the complexity creeps up.
google-chrome-launcher
Outdated
chrome_flatpak_dir=`flatpak-spawn --host flatpak info -l com.google.Chrome` | ||
chrome_binary=${chrome_flatpak_dir}/files/extra/opt/google/chrome/google-chrome | ||
|
||
exec flatpak-spawn --host --env=LD_LIBRARY_PATH=${chrome_flatpak_dir}/files/lib ${chrome_binary} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do some $* or we'll break the methods in the desktop file, or other scripts that rely on passing arguments in to the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I had this in a later version of the patch. Pushed here now.
This allows us to make a self-contained extra-data flatpak that does not need to rely on anything on the host to work.
Newer Chrome versions do not seem to use these.
Instead of importing a copy here.
6b489e4
to
05b4365
Compare
This is needed to run on Fedora Silverblue.
We need those for the appinfo integration. We export all the other sizes from the downloaded deb package.
This allows us to make a self-contained extra-data flatpak that does
not need to rely on anything on the host to work.
CC @ramcq.
Caveat: I haven't fully tested this yet, but it seems to work well enough for me to open this PR.