New Store automated review rule is incorrect


#1

I noticed that overnight a couple more automated review rules were added (from 59 to 62). One in particular seems wrong, which is now preventing us from releasing our VS Code Insiders snap:

malformed desktop file: ‘Icon=’ specified multiple times lint-snap-v2_desktop_file (icon, code-insiders.desktop)

The specified file does have two Icon= directives, but each is in its own section, which should be fine given the spec.

I’m inlining the contents of code-insiders.desktop:

[Desktop Entry]
Name=Visual Studio Code - Insiders
Comment=Code Editing. Redefined.
GenericName=Text Editor
Exec=/usr/share/code-insiders/code-insiders --unity-launch %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png
Type=Application
StartupNotify=false
StartupWMClass=Code - Insiders
Categories=Utility;TextEditor;Development;IDE;
MimeType=text/plain;inode/directory;
Actions=new-empty-window;
Keywords=vscode;

[Desktop Action new-empty-window]
Name=New Empty Window
Exec=/usr/share/code-insiders/code-insiders --new-window %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png

#2

Looks like this code in the review-tools did this. @jdstrand - one for you?


#3

I’ll fix this. Sorry for the inconvenience. We can manually approve in the meantime.


#4

Thanks, no worries! Let me know if that involves doing something from our side!


#5

Can you please go to the particular revision’s page on https://dashboard.snapcraft.io and “request manual review”? (from your list of snaps go to “code-insiders”, then on the left-hand side click on the revision with the red exclamation mark, and the controls are at the bottom).

I can take it from there.

  • Daniel

#6

Done for https://dashboard.snapcraft.io/snaps/code-insiders/revisions/236/

We do nightlies so this workaround will only go so far. Any ETA for the fix to be deployed to the Store? This will let us plan better our upcoming release and whether we need to update the .desktop file to pass the faulty lint rule.


#7

Or, is there a way to disable these review checks? We already have yet another build pending, as we sometimes have multiple daily builds rolling out.


#8

I expect to have the fix in production later today, or tomorrow if something doesn’t go my way today.

Feel free to mention new builds here and we’ll continue to approve manually until the fix is deployed.

Thanks for your patience and apologies for the trouble!

  • Daniel

#9

Great, thanks! Here’s a new one:https://dashboard.snapcraft.io/snaps/code-insiders/revisions/237/


#10

Already approved that one, you can release it now.


#11

Hey folks,

The review-tools update needed to allow multiple Icon= lines in a .desktop file is now in production; upcoming code-insiders uploads should go through automatically. Please poke here if that’s not the case!

  • Daniel

#12

Interestingly, it looks like Snapcraft isn’t properly sanitising the desktop file it puts in your snap. Here’s what the desktop file included in your snap looks like:

[Desktop Entry]
Name=Visual Studio Code - Insiders
Comment=Code Editing. Redefined.
GenericName=Text Editor
Exec=code-insiders %U
Icon=${SNAP}/usr/share/pixmaps/com.visualstudio.code.insiders.png
Type=Application
StartupNotify=false
StartupWMClass=Code - Insiders
Categories=Utility;TextEditor;Development;IDE;
MimeType=text/plain;inode/directory;
Actions=new-empty-window;
Keywords=vscode;

[Desktop Action new-empty-window]
Name=New Empty Window
Exec=/usr/share/code-insiders/code-insiders --new-window %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png

Note that the Icon line for the new-empty-window action is not ${SNAP}/ relative, and that the Exec line also points outside of the snap. The version installed by snapd ends up looking like:

...
[Desktop Action new-empty-window]
Name=New Empty Window
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png

… and when PR #6767 is merged, that problematic Icon line will be removed too. Some of this could be considered a Snapcraft bug (since it is usually the one adding the ${SNAP}/ prefix to the Icon line), but the handling of the Exec line might be your responsibility.


#13

Indeed. The review-tools considered https://github.com/snapcore/snapd/pull/6767 and running the updated tools on https://dashboard.snapcraft.io/snaps/code-insiders/revisions/237/ manually, they will now error differently:

 - lint-snap-v2:desktop_file_icon:code-insiders.desktop:/usr/share/pixmaps/com.visualstudio.code.insiders.png
	invalid icon path '/usr/share/pixmaps/com.visualstudio.code.insiders.png'. Should either specify the basename of the file (with or without file extension), snap.<snap name>.<snap command>[.(png|svg)] or ${SNAP}/path/to/icon.(png|svg)

The tools are showing a real problem in the snap which should be addressed (though that may be a snapcraft bug as @jamesh mentioned).


#14

If the asset is put into snap/{gui,hooks} it is copied over to meta/{gui,hooks} with little to no intervention (as little as chmod for hooks) as those are handcrafted files.

When using the desktop key, snapcraft however only looks at the “Desktop Entry” section, we can revisit the freedesktop spec and see it is safe to iterate over all the sections looking for Icon.


#15

I reviewed the spec when making the change to fix the ‘multiple Icon=’ issue and found that Actions, like the one VS Code Insiders is using, can also have Icon= entries.


#16

@jamesh I don’t think those are the correct contents of the file.

The contents of usr/share/applications/code-insiders.desktop from the latest (https://dashboard.snapcraft.io/snaps/code-insiders/revisions/241/) are the following:

[Desktop Entry]
Name=Visual Studio Code - Insiders
Comment=Code Editing. Redefined.
GenericName=Text Editor
Exec=/usr/share/code-insiders/code-insiders --unity-launch %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png
Type=Application
StartupNotify=false
StartupWMClass=Code - Insiders
Categories=Utility;TextEditor;Development;IDE;
MimeType=text/plain;inode/directory;
Actions=new-empty-window;
Keywords=vscode;

[Desktop Action new-empty-window]
Name=New Empty Window
Exec=/usr/share/code-insiders/code-insiders --new-window %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png

Can you confirm these contents?

I thought snapcraft would be able to patch all those lines.


#17

I can’t see revision 241, since the only published revision is 239 (at least for amd64). That revision looks like what I pasted above:

$ snap download code-insiders
Fetching snap "code-insiders"
Fetching assertions for "code-insiders"
Install the snap with:
   snap ack code-insiders_237.assert
   snap install code-insiders_237.snap
$ unsquashfs code-insiders_237.snap 
Parallel unsquashfs: Using 6 processors
3229 inodes (7332 blocks) to write

[=============================================================\] 7332/7332 100%

created 3048 files
created 1009 directories
created 181 symlinks
created 0 devices
created 0 fifos
$ cat squashfs-root/meta/gui/code-insiders.desktop 
[Desktop Entry]
Name=Visual Studio Code - Insiders
Comment=Code Editing. Redefined.
GenericName=Text Editor
Exec=code-insiders %U
Icon=${SNAP}/usr/share/pixmaps/com.visualstudio.code.insiders.png
Type=Application
StartupNotify=false
StartupWMClass=Code - Insiders
Categories=Utility;TextEditor;Development;IDE;
MimeType=text/plain;inode/directory;
Actions=new-empty-window;
Keywords=vscode;

[Desktop Action new-empty-window]
Name=New Empty Window
Exec=/usr/share/code-insiders/code-insiders --new-window %F
Icon=/usr/share/pixmaps/com.visualstudio.code.insiders.png

There is a second $SNAP/usr/share/applications/code-insiders.desktop that matches your content, but that’s not the one that snapd will preprocess and install.

The fact that Snapcraft has replaced the Exec line with a simple code-insiders %U also sounds a bit like a bug:

  1. we’ve lost the --unity-launch argument
  2. the application may now be invoked with URLs instead of file paths (since the %F was changed to %U).

This is the code that seems to be responsible for that rewrite:


#18

Yeah that looks pretty sketchy. Anything we can do on our side?


#19

If you don’t want to wait for Snapcraft to be updated, you could try creating the desktop file yourself. The changes you’d need are roughly the following:

  1. remove the desktop: key from your app in the snapcraft.yaml
  2. have one of your parts stage a file meta/gui/code-insiders.desktop. The primary requirements are:
    • the desktop file name should match the command.
    • the Exec line(s) should start with the command you want to run (in this case code-insiders). Not a path within your snap, since this is referencing the wrapper snapd creates for your app.
    • the Icon line(s) should use a ${SNAP}/ prefix to reference a file inside your snap.

Looking at the upstream desktop file you’re starting with, adding something like the following to the the of the override-build stanza of your part might do the trick:

mkdir -p $SNAPCRAFT_PART_INSTALL/meta/gui
sed 's|^Icon=|Icon=${SNAP}|;s|^Exec=/usr/share/code-insiders/code-insiders|Exec=code-insiders|' \
    $SNAPCRAFT_PART_INSTALL/usr/share/applications/code-insiders.desktop \
    > $SNAPCRAFT_PART_INSTALL/meta/gui/code-insiders.desktop

#20

I think we’ll wait for the update. Did I get that right that we’d need the store update (which fixes the validation rules) as well as the snapcraft tool update (which fixes the patching)? Can we get an ETA on both? Thanks!

Since we’re about to release a new stable version, I’d require a manual validation approval from you again sometime later this week. I’ll just ping in here once that comes. Thanks!