Add a flatpak manifest #610
Conversation
|
This is great! I've thought about doing this myself, thanks a lot. Have you tried if this works with the GNOME shell extension? See hamster-shell-extension#328 for the issues we had with the snap package (permission problems with DBus service autostart). It'd be great news if flatpak didn't have these issues. |
|
@mwilck Yes, I am actually running it with the GNOME shell extension, and was pleasantly surprised to see it works out of the box! |
|
Just quickly read up on flatpack, sounds like an interesting way to distribute hamster directly to users (in addition to through distro packages), probably better than letting users run I am wondering what the implications of adding this are. How would things work, what would needed be done when? Some specific questions:
|
Usually it's done manually as part of the packaging effort. It would be possible to extract this somewhat automatically if Hamster were to use Pipenv and a Pipfile.lock, else I personally don't think it's worth the effort.
I wanted first to enable people to build a flatpak locally, which is great for testing. Publishing on flathub.org is easy enough, but then the manifest would need to be moved out of this repository and into flathub's. See https://github.com/flathub/flathub/wiki/App-Submission#how-to-submit-an-app
That would be a single-file bundle: https://docs.flatpak.org/en/latest/single-file-bundles.html
This PR is a bit more targeted at developers wanting to test it out (so one assumes a bit of knowledge of flatpak anyway), but sure, I can add that.
If Hamster is GNOME-agnostic, we can depend on the Freedesktop runtime. But the main idea is that flatpak forces you to decide which set of libraries (here, GNOME 3.36 libraries) to target, so that each system installing your app will have exactly the same set of code running. That means ease of debugging, a lot less unreproducible bugs, etc. |
|
I addressed the comments by adding some more documentation in the README, and showing how to build a bundle instead of a repository. I also tried to move away from the GNOME SDK and use the Freedesktop SDK directly, but some dependencies are not available (for instance, gobject-introspection support). |
I'm unaware of any GNOME version dependencies of hamster itself. The GNOME extension has more than enough though. |
Yeah, we don't rely on Gnome itself (that is, you can run hamster on KDE just fine, I think), but we do need some gnome-related libraries for the UI (gtk/glib/gobject), so it is probably easier to just depend on (any version of) the gnome runtime than use the freedesktop runtime and install all the needed libraries on top of that.
Yeah, looks much better (though it also makes a lot of unrelated whitespace changes, could you revert those?). Going through a bundle instead of installing from a local repo is nice because:
The last point could maybe be made explicit in the README? Something like:
I also see a One worry I have is that if we merge this, we also need to make sure the manifest file stays working. Some ingredients to tackle that:
|
|
Hm, I just noticed #561, which also intends to add a flatpack manifest. @bochecha, you created #561, can I invite you to participate in this PR as well? Overall, I'm inclined to work towards merging this PR over #561, since this PR adds some docs to the README. I also do not quite like the I also compared the flatpack manifests themselves a bit, and it seems they are mostly identical, but with some small differences. @tchernobog, @bochecha, since you are more familiar with flatpak, maybe you could compare both so we can pick the best from both? One thing I did notice in the description of #561 is that |
Yes, I agree.
I can do that if there is a way for me to be notified a release is coming.
It is possible to get a shell inside the sandboxed build environment and run tests with: I reviewed the awesome work of @bochecha, which for some reason I missed before (sorry!). There are not a lot of differences; mostly the owned D-Bus names, relying to an external git submodule for intltools (but I guess Hamster should move to plain gettext as intltool is deprecated, and it's not a lot of code), and removing the icon cache. @bochecha Can you elaborate on the need to remove the icon cache? It's the only item I am unsure about. Thanks! |
tchernobog
force-pushed
the
features/flatpak-manifest
branch
9 times, most recently
from
9f9ebd9
to
d345df8
Compare
I think that we're not actually using |
tchernobog
force-pushed
the
features/flatpak-manifest
branch
2 times, most recently
from
2c956f9
to
8e4c891
Compare
|
I added a pipeline for building and testing the flatpak bundle. |
|
@matthijskooijman, @mwilck: are there any other changes you would like? Is there still something you need before approval can be given? |
tchernobog
force-pushed
the
features/flatpak-manifest
branch
from
8e4c891
to
e3f4a7d
Compare
There was a problem hiding this comment.
Looking good, I think we're nearly there. I can't really comment on the contents of the flatpack manifest itself, bu
One new question I realized: How are datafiles handled (e.g. if you upgrade or remove the snap, where does the database go)? In the manifest I see --filesystem=xdg-documents, I suspect that means that e.g. ~/.local and ~/.config from the host system are made available inside the flatpack?
I added a pipeline for building and testing the flatpak bundle.
Cool! One useful addition would be to also make the resulting flatpack file available. I think adding an upload-artifact step would be enough for this.
I left two more inline comments about the README. In the same area, I previously asked the following, I think you did not respond to this yet:
One thing I did notice in the description of #561 is that flatpackbuilder apparently has a --install option, which I think builds and installs directly. If I got that correctly, then that might be even nicer to use as a "build yourself" command in the README?
I also still see some unrelated (whitespace) changes to the README. Those should at least be in a separate commit (which I think they are not right now), but probably just be dropped entirely, since they actually change formatting (trailing whitespace produces a newline in markdown, so it should just be removed).
As for the the PR itself, the history is a bit messy right now, we should probably squash some commits before merging. I think there should be 3 commits:
- Introduce flatpack file and documentation
- Add flatpack GH action
- flake8: Fix indentation
If you're unsure how to approach this, I'm happy to do the restructuring (now, or directly before merging).
Not a snap, it's a flatpak :-D. It creates its own sandboxed folder for the application. Data files are kept in
I will do this myself as the last step, after a final review of the result from you. |
Ah, so that means they are different from the datafiles used by a normal install. I wonder if it is possible to just use the same datafiles as a "normal" install (e.g. allow access to @tchernobog @mwilck what would you think?
Ok, great. |
I personally would prefer to avoid this. It would break the concept of sandboxing. Also, the user might want to keep installed a different system version of Hamster (via package manager) and the new one might be incompatible. If you use flatpak, it's normally accepted that applications have restricted access to your $HOME (including previous settings). |
tchernobog
force-pushed
the
features/flatpak-manifest
branch
from
08d574a
to
98a443c
Compare
Yeah, that's a good point. Might be good to document this, then, and maybe offer some advice on what files need to be copied to migrate data into and out of flatpack. I guess that migrating configuration might be more tricky (since that lives in dconf I think), but that's probably ok, I guess moving data is more relevant anyway. |
tchernobog
force-pushed
the
features/flatpak-manifest
branch
from
98a443c
to
b1f3d99
Compare
I added a section on migrating the database. For DConf, supposedly there would be a way to trigger a migration of config: https://docs.flatpak.org/en/latest/sandbox-permissions.html#dconf-access However, I tried it and it seems it breaks running the unit tests for some reason, so I did not include it as of this time. |
|
@matthijskooijman are there any pending changes you would like me to perform before merging? |
|
@tchernobog not off-hand, I still want to do a review of the whole and your new additions, but no time right now (finishing up some other things before my vacation, so I'll probably won't get around to this until after my vacation, so early august). Also, would be good if one of the other maintainers could do a full review as well. Maybe @mwilck? You have looked at this early on already? |
|
@mwilck would it be possible to merge this? |
There was a problem hiding this comment.
I don't have deep experience with flatpak building, but this looks very good to me. The documentation changes are excellent, everything looks clean and well thought-out. @tchernobog has addressed all comments.
I see no risk of regressions. If any issues with the flatpak itself will show up, I trust that @tchernobog will help us fix them. @matthijskooijman, please merge this if you agree.
|
@matthijskooijman, AFAICS the change you'd requested previously has been addressed. |
|
Just a fly by comment. I just installed the new hamster via this flatpak and it worked fine on my KDE Neon 18.04 machine. I followed the instructions in the README.md and copied over my existing database (from 2.91 Ubuntu package) and was able to get right back to work. I'm no expert but I've made a few flatpak builder recipies and tchernobog's work looks perfectly fine to me. One sugestion I have is to maybe add to the docs about needing to run cli commands as |
Thanks. @tchernobog, could you add that? |
This commit adds a manifest so that Hamster can be built as a sandboxed flatpak application. This has the benefit of always offering a stable environment, where upstream has full control on the versions of all dependencies. See https://flatpak.org/ for more information.
Adds CI actions to build Hamster as a flatpak application.
tchernobog
force-pushed
the
features/flatpak-manifest
branch
from
b1f3d99
to
2a8c5af
Compare
Done, I would prefer to proceed with a merge however now before performing further changes. Making it perfect can be left to subsequent commits. |
|
@tchernobog, thanks a lot.
Sure. As you can see, @matthijskooijman's review is still pending. I trust that as soon as he gets to it, he'll see my positive review and proceed with the merge. |
There was a problem hiding this comment.
I had another look over this PR, looks good to merge for me.
I also tried installing the flatpack version (hadn't done that before, nice that it is built for PRs!), and I like the nice and interactive installation process :-)
I did have some additional questions and remarks, which I left as inline comments. I do not think these are severe enough to hold back the merge, I agree that it is more than usable now and we can perfect things later. So I'll go ahead and merge this, but responses (and maybe a followup PR) to my comments are still appreciated.
@tchernobog, Thanks for your extensive contribution, constructive discussing and long breath :-)
| @@ -125,6 +152,20 @@ as discussed [here](https://github.com/projecthamster/hamster/pull/421#issuecomm | |||
|
|
|||
| Now restart your panels/docks and you should be able to add Hamster! | |||
|
|
|||
| ##### Flatpak | |||
|
|
|||
| Alternatively, you can also build a sandboxed | |||
There was a problem hiding this comment.
This suggests it just builds the flatpack, but I think it also installs it. Might be good to clarify.
| you can do so with: | ||
|
|
||
| ```bash | ||
| flatpak run org.gnome.Hamster [args...] |
There was a problem hiding this comment.
Would there be any easy way to make this shorter? Maybe flatpack can automically put a symlink in ~/bin or /usr/local/bin? Or otherwise register a short name so you can at least use flatpack run hamster (i.e. using the same base command as a normal install)?
Also, it might be good to clarify that (IIUC) the flatpack-hamster will be autostarted through dbus (e.g. when the shell extension tries to access it). That does make me wonder: What does flatpack do when the dbus name is already registered (i.e. when hamster is normally installed as well)? From a quick test it seems to (silently) not claim the name, that is, when I start the flatpack, the flatpack GUI connects to the non-flatpack dbus service, and when I kill the non-flatpack services, it still spawns the non-flatpack ones, rather than the flatpack ones.
For most users, I guess this is not so much an issue (they would typically only install the flatpack version), but it would be at least to document these caveats somewhat. Also, one usecase I see for flatpack is that it would help with testing nightly builds or pullrequests, and then it would be convenient of someone could install the flatpack version next to the regular version and have them completely isolated.
There was a problem hiding this comment.
That does make me wonder: What does flatpack do when the dbus name is already registered (i.e. when hamster is normally installed as well)? From a quick test it seems to (silently) not claim the name, that is, when I start the flatpack, the flatpack GUI connects to the non-flatpack dbus service, and when I kill the non-flatpack services, it still spawns the non-flatpack ones, rather than the flatpack ones.
I don't think it's possible to hijack a well-known dbus address. I guess it shouldn't be. At best, hamster itself should log a meaningful message when it fails to acquire the address. But that's independent of packaging. It applies also to a situation with installed hamster and local (developer) instance, for example.
It's a general problem that I have with all applications that can be installed either "regularly" or as flatpak/snap/appimage (you name it): It's highly intransparent to the user which package she is activating. DBus makes it even more intransparent, of course.
There was a problem hiding this comment.
I don't think it's possible to hijack a well-known dbus address. I guess it shouldn't be. At best, hamster itself should log a meaningful message when it fails to acquire the address. But that's independent of packaging. It applies also to a situation with installed hamster and local (developer) instance, for example.
Well, there's two parts to this: One is the claiming of a dbus name at runtime, which is something hamster should report. The other, which I was thinking of, is installing a file in /usr/share/somewhere that declares that a dbus interface exists and what binary to autostart if the name is requested but not on the bus yet (at least that's how I gather it works). This is something that flatpack install should handle, but maybe flatpack handles this differently? Also, it seems that the dbus name should match the flatpak name, not sure if our WindowServer postfix is then supported (the primary service does match).
| ```bash | ||
| gio copy -b \ | ||
| ~/.local/share/hamster/hamster.db \ | ||
| ~/.var/app/org.gnome.Hamster/data/hamster/ |
There was a problem hiding this comment.
Why use gio copy here? Looks like these are two regular paths that can just use normal cp?
|
Just a follow-up question. The README now says:
But it doesn't say where you can download the Flatpak. I guess you are referring to the corresponding GitHub Actions artifact[1]? Any plans on distributing the Flatpak via Flathub? [1]: I've succesfully tested this in conjunction with the GNOME-3.36 version of hamster-shell-extension on Ubuntu 20.04, works like a charm. 👍 |
You can get it from there for now. I would assume the GNOME Hamster maintainers could promote the built .flatpak file to a release artifact for next release.
That would require to get the manifest merged to a different repo, see: https://github.com/flathub/flathub/wiki/App-Submission |
This PR adds flatpak support for Hamster. Building the application is surprisingly easy, and making it available on flathub should contribute to Hamster's usage growth.