New Store automated review rule is incorrect


#41

@panda - I appreciate that these new checks have caused a problem. Unfortunately, it has been difficult to understand the precise problem you are experiencing because you’ve stated that things broke for you, but not how. I see in another topic that the issue is that you are using an ICO file and not a PNG file. @jamesh can comment if ICO should be supported or not (I suspect yes). As such, we can adjust the snapd PR and the review-tools to account for ICO files.

Your rhetoric has included a lot of hyperbole about breaking many snaps, however, this has not been seen in the store review queue. I do see your snap and the vscode snaps and we can work with you to approve the snaps until the ICO issue is resolved. In the meantime, you could convert your ICO to a PNG and this would allow your snap to pass automated review again while the fix is on the way for ICO files.


#42

I spoke with @jamesh in person and we agreed the review-tools should be adjusted. The svg and png limitation was meant to be only for the icon sets feature, not paths that use ${SNAP}.


#43

The new restrictions on desktop files referencing an icon by path from my PR are:

  1. the path must begin with ${SNAP}/, since it should reference a file distributed with the snap.
  2. the path must be canonicalised. By this I mean it doesn’t include extraneous path segments. For example, ${SNAP}/icon.png is okay while ${SNAP}/foo/../icon.png is not. The primary goal here is to avoid paths that could reference files outside of the snap.

There are no restrictions on image format here (that is only for new snaps trying to install icon theme icons), so there is no particular reason to reject .ico files here. So the review-tools check is probably too strict here.


#44

@panda - I have manually approved revisions 848 through 853 and I will keep an eye on the review queue while the update to the review-tools is on its way.

As an aside, while I apologize that your snap was affected, please keep in mind that snapd and its surrounding services evolve and as such new bugs may come up from time to time despite efforts to minimize them. In the future for any new bugs, I suggest that you clearly state the problem in specific terms while understanding that people are here to help address issues you may find.


#45

Here’s the PR @jdstrand and @jamesh

This PR adds logic to iterate over all the sections in a desktop file and properly handles the Exec line.


#46

This is being tracked as https://launchpad.net/bugs/1844534


#47

FYI, the fix has been live in the store for a few days.