New Store automated review rule is incorrect


#27

Interestingly, I started researching this issue today: https://github.com/microsoft/vscode/issues/72042

The same root cause is behind it: snapcraft incorrectly patches the line to be Exec=code-insiders, which makes the app launcher use the binary located in the PATH which breaks the aforementioned issue. Thanks for that pointer.

Thanks for the upcoming fix @sergiusens.


#28

Hi @sergiusens, any update on that PR? Thanks for the help.

Meanwhile, @popey can we get https://dashboard.snapcraft.io/snaps/code-insiders/revisions/246/ approved? Thanks.


#29

Done.


#30

@joaomoreno (cc @popey): please note that the updated review-tools are in production and the failure is legitimate. Your snaps need to be updated to no longer use an absolute path to the file /usr/share/pixmaps/com.visualstudio.code.png or /usr/share/pixmaps/com.visualstudio.code.insiders.png. We can continue to manually approve these for a little while, but if you fix your snaps, they’ll pass automated review again.

@sergiusens - you mentioned proposing a PR, but I couldn’t find it. Can you post the PR link here when you do?


#31

@jdstrand I was under the impression the problem here is the patching of the .desktop files by snapcraft which is faulty. Shouldn’t the tool be the one correctly adding ${SNAP} to those directives, which would be the fix eventually submitted in PR by @sergiusens?


#32

Yes, which is why I asked @sergiusens for more information. However, you are free to update your use of the tool and either patch up the desktop file or ship a compliant one in the meantime as discussed elsewhere in the topic. That would enable you to unblock your uploads until snapcraft is fixed.


#33

Oh missed this reply, my bad. Giving this a try. Thanks!


#34

I created a PR which gives you all the control you want

If you don’t mind providing some simple instructions on what your build process looks like to get to the “snap” I do not mind iterating myself.


#35

Thanks to @sergiusens we’re back to normality, thanks!

@popey I just have to ask you for one more manual approval, for our candidate stable release on which I haven’t placed the fix yet: https://dashboard.snapcraft.io/snaps/code/revisions/15/ Thanks.


#36

This is failing differently.

invalid icon path '/usr/share/pixmaps/com.visualstudio.code.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). If using snapcraft, consider using 'desktop: <file>' since the Icon paths in the desktop will be rewritten to use ${SNAP}/<file>. lint-snap-v2_desktop_file_icon (code.desktop, /usr/share/pixmaps/com.visualstudio.code.png)

I’d recommend ensuring the desktop file has lines such as:

Icon=${SNAP}/usr/share/pixmaps/com.visualstudio.code.png

Which should resolve this.


#37

This is the same as the previous one I mentioned in New Store automated review rule is incorrect. Since @joaomoreno understands this is invalid but the snapcraft fix is on the way, I approved it.


#38

did you test it properly? Why was it applied at all? What for exactly do you need this test?


#39

I don’t quite understand this comment but will say that, yes, the rule was tested and yes it is finding a legitimate problem in the snap. It also found a legitimate problem in what snapcraft is doing and now snapcraft is being fixed. Finally, this is being fixed in vscode while the snapcraft fix is pending.


#40

The comment asks if you tested the fix properly, as what has been applied can not have been properly tested. On that point, sure, I would love to know how you tested it before applying it globaly. Is it wrong to ask which steps you/staff undertakes for testing code before it is globaly applied to ensure quality of the service for published software over snapcraft.io?

From your answer, you seem to have less understanding for the question where I have even less understanding when you say that you tested it. It only tells me that “test” performed for snapcraft stable are not testing at all already published snaps which results in breaking working packages.

So, is it to much asked that only stable code, properly tested wanders into something that is globally applied?

I also understand that software is never perfect and never will be, but to take it as excuse is here wrong, because if you would have taken most popular snaps under which is vscode too, you would immediately face the problem, fact that you did not face is telling that it was not properly tested.

Not that I cant fix it, I fixed it already, but again, if fix on snapcraft is coming, you created overhead for quite many people, solution for them is to wait for weeks/months/… for a fix, now those who applied their own fixes, they will have again some overhead to revert those changes, even it it is one command to apply git patch, it still requires somebodies time. All that makes snapcraft.io less reliable service and again.

In previous post I only asked you if you properly tested the fix, above I hopefully explained what the question meant which you did not understand, in previous post I assumed that you understand what I mean and wanted to save you those few minutes of reading.


#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