wrappers: remove all desktop files from a snap on removal #6156
Conversation
mvo5
force-pushed
the
snap-remove-remove-desktop
branch
from
6d27183
to
acfbb3b
Compare
When we added support for parallel installs we broke removal of some desktop files. This was reported in LP: 1791182. The problematic commit is 84008e. This PR changes the way we write and remove desktop files. One problem here is that desktop files of a foo_instance will get removed when "snap remove foo" happens. Given that parallel installs are experimental this might be acceptable (or not).
mvo5
force-pushed
the
snap-remove-remove-desktop
branch
from
acfbb3b
to
52d5aeb
Compare
Is that an interim problem or the final functionality? that doesn't seem reasonable or I'm missing something, or snap with desktop files are not well supported to start with? Notice that plan was in 2.37 for parallel installs not to be experimental anymore if I remember correctly? ping @bboozzoo |
|
I, for one, would love if we handled desktop files with EnsureDirState functionality where we can easily scope the files associated with each snap and we can update them on demand (if snapd code evolves). Please consider this idea. |
|
Setting to blocked until this has more unit tests (but keen to see if all spread tests work). |
mvo5
changed the title
| Icon=${SNAP}/meta/gui/icon.png | ||
| Terminal=true | ||
| Type=Application | ||
| Categories=Utilities;Useless; |
| #shellcheck source=tests/lib/snaps.sh | ||
| . "$TESTSLIB/snaps.sh" | ||
| echo "Installing of a snap with a desktop file creates the desktop file" | ||
| echo "Given a snap declaring the home plug is installed" |
|
@bboozzoo Thanks for the review! I was thinking we ignore existing desktop files. The only problematic ones are those from parallel installs, the others remain unchanged. Because parallel installs are experimental I had hoped we can get away with a forum post about the issue. However if we need to migrate we can do this and use a sub patch for this I think. I would do this as a followup if needed. |
|
Parallel installs is experimental until 2.37 so a forum post sounds good to me. |
| @@ -5,6 +5,6 @@ Exec=basic-desktop.echo | |||
| Icon=${SNAP}/meta/gui/icon.png | |||
| Terminal=true | |||
| Type=Application | |||
| Categories=Utilities;Useless; | |||
| Categories=Utilities;Useful; | |||
There was a problem hiding this comment.
Hey, this is a nice upgrade :D
| @@ -220,7 +235,11 @@ func AddSnapDesktopFiles(s *snap.Info) (err error) { | |||
| return err | |||
| } | |||
|
|
|||
| installedDesktopFileName := filepath.Join(dirs.SnapDesktopFilesDir, fmt.Sprintf("%s_%s", s.InstanceName(), filepath.Base(df))) | |||
| // FIXME: don't blindly use the snap desktop filename, mangle it | |||
When we added support for parallel installs we broke removal of
some desktop files. This was reported in LP: 1791182. The problematic
commit is 84008ef.
This PR changes the way we write and remove desktop files for parallel installs. Regular snaps will still be installed as "snap_desktop-file-name.desktop" but instances will be installed as "snap+instance_desktop-file-name.desktop" so that there is a different separator between snapName and snapInstance from the separator between instanceName and desktopFile.