Martchus / syncthingtray

Tray application and Dolphin/Plasma integration for Syncthing
https://martchus.github.io/syncthingtray/
Other
1.66k stars 44 forks source link

Always getting the correct AUTOSTART_EXEC_PATH ? #176

Closed doronbehar closed 1 year ago

doronbehar commented 1 year ago

Relevant components


Hello @Martchus,

Thanks to a peer review at Nixpkgs, I came to realize that this cmake variable design perhaps wasn't right. I think it'd be much more neat (if you'd accept the idea) to drop this cmake variable, and do the following near that desktopFile.write line at settingsdialog.cpp:

I'd appreciate any advice before I try to implement this :)

Thanks.

Martchus commented 1 year ago
  1. The current use of QCoreApplication::applicationFilePath() should be equivalent to boost::dll::program_location. I would prefer keep using the former to avoid pulling in another dependency. (And yes, I'm already using Boost because Boost.Process is clearly better than QProcess but in general I'd still like to stick to Qt when possible.)
  2. I don't think I understand the second point. What is meant by "wrapped"? So far we simply assign the program location to Exec= which makes most sense in my opinion¹. So what in addition is needed for your Nixpkgs setup?
  3. The .desktop file should generally still contain the full executable path because the syncthingtray executable might not be in PATH, e.g. when one does a custom installation. If you wanted a relative path there, you could use AUTOSTART_EXEC_PATH to do that. I'm mentioning this because using a relative path was suggested in https://github.com/NixOS/nixpkgs/pull/209062#pullrequestreview-1236514452.

¹ The only exceptions are the AppImage case (which should be kept as-is) and the AUTOSTART_EXEC_PATH override (which I personally don't care much about but we might now want to keep as well for the sake of compatibility).

doronbehar commented 1 year ago
  1. Maybe I should have wrote "locate the wrapping syncthingtray". Basically, Nix' build process takes $out/bin/syncthingtray that cmake builds, renames it to $out/bin/.syncthingtray-wrapped and creates a bash script in $out/bin/syncthingtray that calls $out/bin/.syncthingtray-wrapped with environmental variables needed mostly on NixOS. The script $out/bin/syncthingtray is the wrapper, and $out/bin/.syncthingtray-wrapped is the wrapped executable. If you'd try to run on a NixOS machine the .syncthingtray-wrapped executable without the wrapper, you'd get nasty errors.

Anyway, I was thinking about the following change, that would allow Nix to not set the newly introduced AUTOSTART_EXEC_PATH: https://github.com/Martchus/syncthingtray/pull/177