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

Flatpak for SyncthingTray #261

Closed
1 of 6 tasks
qgymib opened this issue May 15, 2024 · 21 comments
Closed
1 of 6 tasks

Flatpak for SyncthingTray #261

qgymib opened this issue May 15, 2024 · 21 comments

Comments

@qgymib
Copy link

qgymib commented May 15, 2024

Relevant components

  • Standalone tray application (based on Qt Widgets)
  • Plasmoid/applet for Plasma desktop
  • Dolphin integration
  • Command line tool (syncthingctl)
  • Integrated Syncthing instance (libsyncthing)
  • Backend libraries

Hi Martchus,

Thank your for amazing work. Since #33 I see no further plans for flatpak, so I just pack it which should works:
https://github.com/qgymib/io.github.Martchus.syncthingtray/tree/new-pr

To follow flatpak's Requirements & Conventions a little modifications were made:

  1. The application ID is changed from org.martchus.syncthingtray to io.github.Martchus.syncthingtray

This package should be ready (or with some little optimize, e.g. remove build dependency) to publish to flathub. Since flathub prefer app’s developers to publish the software, would you like to take the repo's ownership? And of course I also can help to publish and maintain the flatpak distribution if you want.

@Martchus
Copy link
Owner

I'll review your repo and in general I'd also be willing to take ownership.

Why exactly do you need to change the app ID? I don't care about the ID but it would probably make sense to upstream that change first. @sten0 When I remember correctly, you cared about the AppStream metadata when packaging this for Debian. So would it be ok for you if we'd change it? (Maybe it makes most sense to make it a configurable build parameter at this point.)

And of course I also can help to publish and maintain the flatpak distribution if you want.

That would be required because maintaining the Flatpak would exceed the amount of effort I want to put into the project - I rather focus on development.

@Martchus
Copy link
Owner

Has the change of the ID something to do with the website URL? I only set it up recently (after seeing the app ID). So that's why the app ID is not in line with that.

@qgymib
Copy link
Author

qgymib commented May 15, 2024

Hi Martchus,

The app ID located in /share/metainfo/syncthingtray.appdata.xml, with content:

<id>org.martchus.syncthingtray</id>

There are some examples on flatpak's page shows what should the appID looks like. So according to the page the prefered ID should be either org.martchus.SyncthingTray or io.github.Martchus.syncthingtray.

The app ID is used in flathub.org it self, so I'm not sure whether upstream should change it. May be we can try to publish without change first, and see if flathub team is OK with it?

@qgymib
Copy link
Author

qgymib commented May 15, 2024

Has the change of the ID something to do with the website URL? I only set it up recently (after seeing the app ID). So that's why the app ID is not in line with that.

No, it's not. It only related to /share/metainfo/io.github.Martchus.syncthingtray.metainfo.xml which will be used in flathub.org store.

@qgymib
Copy link
Author

qgymib commented May 15, 2024

After some research, I do discovery some apps that does not following flatpak conventions, like this, this and this.

Maybe it is OK to left app ID untouched, and try to publish to flathub.org first.

@Martchus
Copy link
Owner

Ok, I guess I'll just keep the app ID defined in my upstream AppStream meta-data file for now as-is. I can add build system variables so you can avoid all the manual moves/replacements¹ and just specify a build parameter like -D…=… if changes on your side turn out to be necessary.


I had a look at https://github.com/qgymib/io.github.Martchus.syncthingtray/blob/new-pr/io.github.Martchus.syncthingtray.yml.

It seems like you're bundling Syncthing itself as well. That makes sense. I'd like to note that you could get rid of the manual build step and build Syncthing as library when building Syncthing Tray. Just add -DNO_LIBSYNCTHING:BOOL=OFF and -DUSE_LIBSYNCTHING:BOOL=OFF to Syncthing Tray's build parameters and make sure Git modules are checked out. This way you would be able to avoid having to take care about updating Syncthing itself. (I always update the bundled Syncthing library to the latest release before releasing Syncthing Tray and usually release Syncthing Tray after a new version of Syncthing has just been released.)

It seems a bit problematic that you have to install Perl modules and Boost manually. So it isn't possible to just pull in some package? I guess the Perl modules don't really matter but Boost you would need to keep up-to-date manually.

I see that you also build libplasma manually. I highly doubt that this is useful. Prove me wrong but I think the Plasmoid needs to be build against the system-provided Plasma version and the system-provided plasmashell wouldn't be able to find a Plasmoid installed via a Flatpak anyway. So unless this actually works I would suggest you remove the libplasma build and add -DNO_PLASMOID=ON to the build options. Maybe it makes sense to ask Plasma developers about this.


¹

post-install:
 - mv /app/share/applications/syncthingtray.desktop /app/share/applications/io.github.Martchus.syncthingtray.desktop
 - mv /app/share/icons/hicolor/scalable/apps/syncthingtray.svg /app/share/icons/hicolor/scalable/apps/io.github.Martchus.syncthingtray.svg
 - sed -i 's/Icon=syncthingtray/Icon=io.github.Martchus.syncthingtray/g' /app/share/applications/io.github.Martchus.syncthingtray.desktop
 - mv /app/share/metainfo/syncthingtray.appdata.xml /app/share/metainfo/io.github.Martchus.syncthingtray.metainfo.xml
 - sed -i 's/org.martchus.syncthingtray/io.github.Martchus.syncthingtray/g' /app/share/metainfo/io.github.Martchus.syncthingtray.metainfo.xml

@qgymib
Copy link
Author

qgymib commented May 15, 2024

It seems like you're bundling Syncthing itself as well.

I use standalone Syncthing because build bundled Syncthing shows error about something like inconsistent vendoring. I'm not familiar with GO, but surely I can look into it later.

It seems a bit problematic that you have to install Perl modules and Boost manually. So it isn't possible to just pull in some package?

Unlike aur or snap or docker, flatpak's build process seems no way to use something like apt install or pacman -S to pull a pre-build package. That is really annoying but currently I cannot find a way to resolve it.

I see that you also build libplasma manually. I highly doubt that this is useful.

OK I will remove it.

@Martchus
Copy link
Owner

Martchus commented May 15, 2024

OK I will remove it.

By the way, I asked on the Plasma channel and got the following responses:

plasma doesn't load anything from flatpak (and actually can't reasonably load anything from flatpak)
so it's fairly pointless unless the flatpak has some way of installing the plasmoid, but that'd kinda break the sanboxing

Sandboxing isn't the only reason to use flatpak. I do wonder if flatpak could be expanded to export certain files that can be used by Plasma.

So for now this is as useful as I suspected (not useful at all). Maybe that'll change in the future but I doubt it'll change anytime soon (especially for plasmoids using native code).

@qgymib
Copy link
Author

qgymib commented May 16, 2024

Update:

The syncthing is now bundled as library.

Due to the restriction that flatpak-builder forbid download anything during build stage, the bundled syncthing must build in vendor mod, so the CMakeLists.txt has to be patched, see 0001_force_vendor_build_bound_syncthing.patch, the patch will be applied automatically during build progress. It seems no need to change upstream syncthingtray's CMakeLists.txt because event syncthing does not use vendor in their repo, they only use vendor in their release .tar.gz.

Checkout org.martchus.syncthingtray.yml to see the updated build script.


The remaining task is to try to remove Perl.

qtforkawesome require YAML::XS to build, and somehow the pre-installed perl in flatpak's SDK cannot find the YAML::XS package event I build it and set PERL5LIB to the corresponding path. In that case I build perl to make it working.

I guess it still possible to use pre-installed perl, just need a little bit research and magic.

@qgymib
Copy link
Author

qgymib commented May 16, 2024

Update:

After some testing, there are two problem that bundle syncthing library with SyncthingTray in flatpak:

  1. SyncthingTray always complain “Disconnected from Syncthing” on startup, but the web page actually can be opened by right click tray and click "Open Syncthing".
  2. In web page, there always a warning Failed to lower process priority: set niceness: permission denied.

Both problems not exists if build syncthing as standard executable, so I guess we should keep them separated.

For problem one I will do more test to see if it only exists in flatpak or is a common issue (Some other may have already report like #252 ).

For problem two there is a strange behavior that SyncthingTray can capture this warning on very first startup (The one that shows a dialog to do the first configuration). After that SyncthingTray no long capture this warning and just leave it on the web page. There seems no configuration to suppress this warning so each time syncthing startup it will show once.

@Martchus
Copy link
Owner

SyncthingTray always complain “Disconnected from Syncthing” on startup, but the web page actually can be opened by right click tray and click "Open Syncthing".

That's strange but probably more likely a config issue and not a general issue with the builtin Syncthing library. (In #252 is was also just a config issue.)

In web page, there always a warning Failed to lower process priority: set niceness: permission denied.

I also remember seeing this independently from Flatpak. I guess it makes sense to disable this feature for the Syncthing library because setting a low niceness for the whole process doesn't make much sense. Maybe it would be nice if the Syncthing library could set this for just the Go threads. For now it might not be the worst to just use an external executable.

I had a look at https://github.com/flathub/flathub/blob/d133b382e7cc98f6c90f9b192663ddb141b13cad/patches/0001_force_vendor_build_bound_syncthing.patch and made this configurable via -DGO_BUILD_MOD:STRING=vendor as it is actually a common use case. (One can now pass other arguments to the Go build as well.)

The ID can now also be overridden using -DSYNCTHINGTRAY_RDNS_OVERRIDE:STRING=… but you probably still need to rename files. I also updated the extension of the AppStream file (because the extension being used seems deprecated).

@qgymib
Copy link
Author

qgymib commented May 16, 2024

Hi @Martchus,

To follow requirements, in file /share/metainfo/syncthingtray.appdata.xml the developer id property should be added:

<developer><name>Martchus</name></developer>

Should change to something like (See example here):

<developer id="org.martchus"><name>Martchus</name></developer>

Chould you please help to confirm what develop id you want to use?

Also this change might be suitable to apply to upstream SyncthingTray.

@Martchus
Copy link
Owner

Chould you please help to confirm what develop id you want to use?

org.martchus is good. I'll change the build system to include it in the AppStream XML as well.

Martchus added a commit to Martchus/cpp-utilities that referenced this issue May 16, 2024
@qgymib
Copy link
Author

qgymib commented May 17, 2024

OK let's summarize the issues currently have:


Flatpak only (also recorded in README):

1. Autostart not working

This caused by flatpak's sandboxing. The workaround is to manually create symbolic link to the startup file.

The actual solution is to use portal API org.freedesktop.portal.Background to do auto start/stop control. This require upstream SyncthingTray to implementation it.

2. SyncthingTray stuck on setup wizard, or show "Unable to establish connection to Syncthing" on setup wizard

Remove ~/.config/syncthingtray.ini and ~/.local/state/syncthing, then try again.


generic problems, may be open new issues to track them?

1. Welcome dialog shows incomplete fonts (also exist in syncthingtray-qt6 from AUR)

System: archlinux with KDE, x11

2024-05-17_08-27

However the release tarbar is fine.

2. When bundle syncthing as library, SyncthingTray always show "Disconnected from Syncthing" (also exist in official release)

To reproduce:

  1. Download official release.
  2. Remove ~/.config/syncthingtray.ini and ~/.local/state/syncthing (remember to backup them first)
  3. Start SyncthingTray, during setup wizard select Start Syncthing application that is built into Syncthing Tray.
  4. Finish setup, and exit SyncthingTray.
  5. Start SyncthingTray again and you should see the warning.

3. Strange left-click behavior on ubuntu 24.04

When left-click on the tray, the widget shows (may be disappear quickly), and the right-click menu shows. On more left-click, only the menu shows, the widget no longer popup again.


For the flatpak package, it is now ready to publish to flathub.org. Please help to review the repo again to see if anything should be changed. The comments in this branch will always be squash, for future maintainability.

If everything is OK, I will send PR to flathub.org team and ask to add write access for both of us. There may be more modifications in the future, according to their review comments.

@Martchus
Copy link
Owner

  1. Feel free to come up with a PR. If the feature is optional (and also all dependencies is may additionally require) and configurable at build time I can merge it. Maybe it makes also sense to be able to configure the "not supported" case meanwhile explicitly (so at least things don't look broken).
  2. Hard to tell why it isn't working without the concrete error message and details from the setup detection. I cannot reproduce this on my Arch/openSUSE/Windows systems.

  1. That's a bug in the Breeze style considering it works with Fusion, Windows and Windows Vista/11 styles. You have already noticed that the binaries on GitHub are fine. That's simply because they don't use Breeze but Fusion. If you are annoyed by it I suggest you file a ticket in the upstream KDE/Breeze repo.
  2. I'll reproduce that when I have time.
  3. Maybe a limitation/bug of whatever desktop environment this Ubuntu setup you've been testing on has installed. Please read the README section about known problems. Especially if Wayland is used here many caveats may apply. I simply cannot support the GNU/Linux desktop very well as a whole anymore considering its diverging/degrading state since the adoption of Wayland. (The only Wayland-based desktop that is actually working is KDE Plasma but only via the Plasmoid.)

I think the Flatpak repo looks good. I like that you document the autostart problem and workaround explicitly. I only have the nitpick that the human-readable/display name of the project is actually "Syncthing Tray" (with a space between the words). You may also use that name for the heading and link (instead of using all-lowercase syncthingtray which is just the repository/binary/package name).

Note that # TODO: accepted by upstream, can be removed in v1.5.4 is correct but I'm not sure what the next version number will be. It really depends on what changes will be available then. Also, the relevant repo would be cpp-utilities (so the next release would be v5.24.9 or v5.25.0 depending on whether new features will be added or not).

Maybe for now it is good if it lives under your organization as I probably won't be able to contribute much anyway. I'm also not sure what the convention is but if it would live under my GitHub profile I'd prefer to prefix it with flatpak- because otherwise the name isn't really descriptive. Or maybe just flatpaks where possibly multiple Flatpaks could be developed. (I usually want to create solutions that can apply to all of my projects/repos and one Flatpak-repo per actual project/repo seems a bit excessive and cumbersome considering I would often update multiple projects in one go.)

If the Flatpak has been published it would make sense to mention it on the README and possibly also on https://martchus.github.io/syncthingtray/#downloads-section.

@qgymib
Copy link
Author

qgymib commented May 17, 2024

the project is actually "Syncthing Tray" (with a space between the words).

I have to apologize for that😱, I use Syncthing Tray for quite some time and just realize that. Fixed in README.

Maybe for now it is good if it lives under your organization as I probably won't be able to contribute much anyway.

Sorry I didn't explain this well. A flatpak's repo actually live under flathub organization. Here is the work flow:

  1. Somebody want to publish to flathub must fork their base repo, the forked repo name and branch name actually not important. In this case is org.martchus.syncthingtray.
  2. After an application has packed, submit a PR to flathub.
  3. Once this PR is approved, a new repo will be created under flathub organization. The name of the repo is the application ID. For example this one.
  4. The one who submit this PR, as well as the one who mentioned in the PR, will receive invitation to have write access for the repository. You have to accept it before the invitation expires (we can still contact them if expires).
  5. Futher maintenance always on that repo, the original repo is useless and can be deleted.

If everything looks good with you, I will send the PR after your final confirm.

@Martchus
Copy link
Owner

No need to apologize and thanks for your work. The explanations make sense so feel free to submit.

@qgymib
Copy link
Author

qgymib commented May 21, 2024

Here it is 😊:

https://flathub.org/apps/io.github.martchus.syncthingtray

@Martchus
Copy link
Owner

I'll link it later on the website. I'll also fix the bug tracker URL and might streamline the description with the website.

Copy link

stale bot commented Jul 20, 2024

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 20, 2024
@Martchus
Copy link
Owner

The Flatpak is available now so this issue can be closed.

@Martchus Martchus removed the stale label Jul 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants