snap,wrappers: fix common id selection with multiple desktop plugs#17016
Open
olivercalder wants to merge 2 commits into
Open
snap,wrappers: fix common id selection with multiple desktop plugs#17016olivercalder wants to merge 2 commits into
olivercalder wants to merge 2 commits into
Conversation
olivercalder
requested review from
ZeyadYasser,
andrewphelpsj,
Copilot and
natibek
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes desktop file ID selection for snaps that define multiple desktop plugs by ensuring desktop-file-ids are gathered from all desktop plugs in a deterministic way (sorted by plug name). This improves correctness for wrapper desktop file installation and for AppInfo.DesktopFile() selection, avoiding non-deterministic behavior caused by ranging over the plugs map.
Changes:
- Update
snap.Info.DesktopPlugFileIDs()to collectdesktop-file-idsacross alldesktopplugs, sort plug names for determinism, and deduplicate IDs. - Add wrapper-level regression coverage to ensure unmangled desktop IDs are honored when multiple
desktopplugs exist. - Add snap-level unit tests to confirm merged ID behavior and
DesktopFile()selection when IDs are provided via a non-default desktop plug.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
snap/info.go |
Collect and deterministically merge desktop-file-ids from all desktop plugs (sorted, deduped). |
snap/info_test.go |
Add tests validating merged ID ordering/deduping and AppInfo.DesktopFile() behavior across multiple desktop plugs. |
wrappers/desktop_test.go |
Add regression test ensuring EnsureSnapDesktopFiles honors desktop-file-ids when multiple desktop plugs are present. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17016 +/- ##
==========================================
+ Coverage 79.07% 79.15% +0.07%
==========================================
Files 1375 1363 -12
Lines 191478 191237 -241
Branches 2465 2465
==========================================
- Hits 151419 151371 -48
+ Misses 30949 30764 -185
+ Partials 9110 9102 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Tue May 12 08:27:05 UTC 2026 Failures:Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
ZeyadYasser
reviewed
May 5, 2026
Contributor
ZeyadYasser
left a comment
ZeyadYasser
left a comment
There was a problem hiding this comment.
Change looks really good, Thanks for fixing!
Would you mind updating snap under tests/main/desktop-file-ids/task.yaml to test for connection combinations of multiple desktop interfaces? I am interested in stress testing what happens on sequential connection/disconnection
3v1n0
reviewed
May 5, 2026
| if seenDesktopFileIDs[desktopFileID] { | ||
| continue | ||
| } | ||
| seenDesktopFileIDs[desktopFileID] = true |
Contributor
There was a problem hiding this comment.
Suggested change
| seenDesktopFileIDs[desktopFileID] = true | |
| seenDesktopFileIDs[desktopFileID] = struct{}{} |
Instead?
Member
Author
There was a problem hiding this comment.
I prefer using map[string]bool so it's semantically just a set and we can do if seenDesktopFileIDs[desktopFileID] without needing to check the val, ok of the result.
natibek
approved these changes
May 5, 2026
Contributor
natibek
left a comment
natibek
left a comment
There was a problem hiding this comment.
LGTM! Maybe worth adding info to https://snapcraft.io/docs/reference/interfaces/desktop-interface/ about the precedence of desktop-file-ids and that all desktop plugs are collected and sorted by name.
Member
andrewphelpsj
left a comment
andrewphelpsj
left a comment
There was a problem hiding this comment.
Thanks, impl looks good. Yeah, I'd update tests/main/desktop-file-ids/task.yaml as Zeyad suggested.
When multiple `desktop` plugs are present for a given app, we need to ensure that any `desktop-file-ids` attributes across all such plugs are considered when selecting the desktop file ID for the app. This commit ensures that all `desktop` plugs are collected and sorted by name first, and then for each, if the `desktop-file-ids` plug is present, add its IDs to the list of common IDs for the app. This ensures that if any `desktop-file-ids` attribute is present, the IDs listed always have precedence over the fallback common ID. It also ensures that if multiple plugs are present, the output is always deterministic. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
olivercalder
added
cross-distro
olivercalder
force-pushed
the
fix-snap-common-id-selection
branch
from
7caecc2 to
ca3fb72
Compare
Member
Author
|
I considered going crazy with combinations of |
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
olivercalder
force-pushed
the
fix-snap-common-id-selection
branch
from
ca3fb72 to
8f286b3
Compare
| [Desktop Entry] | ||
| Name=test-snapd-desktop.cmd | ||
| Name=test-snapd-desktop-file-ids.cmd | ||
| Comment=A desktop file for test-snapd-desktop.cmd |
|
|
||
| execute: | | ||
| files="$DIR/org.example.desktop $DIR/org.example.Fallback.desktop $DIR/org.example.MyCommonID.desktop $DIR/test-snapd-desktop-file-ids_foo.desktop $DIR/test-snapd-desktop-file-ids_bar.desktop $DIR/test-snapd-desktop-file-ids_org.example.Foo.desktop $DIR/test-snapd-desktop-file-ids_bad._____________.desktop" | ||
| files="$DIR/org.example.desktop $DIR/org.example.Foo.desktop $DIR/org.example.Fallback.desktop $DIR/test-snapd-desktop-file-ids_foo.desktop $DIR/test-snapd-desktop-file-ids_bar.desktop $DIR/test-snapd-desktop-file-ids_org.example.Bar.desktop $DIR/test-snapd-desktop-file-ids_bad._____________.desktop" |
| fi | ||
|
|
||
| echo "Just because a desktop file ID is declared does not mean it is created" | ||
| test ! -f "$DIR/org.example.NonexistentFile.desktop" |
| RESULT="$(snap debug api /v2/snaps/test-snapd-desktop-file-ids)" | ||
| echo "$RESULT" | MATCH '"status-code": 200' | ||
| APPS="$(echo "$RESULT" | gojq .result.apps)" | ||
| # Should have common-id but no file id |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When multiple
desktopplugs are present for a given app, we need to ensure that anydesktop-file-idsattributes across all such plugs are considered when selecting the desktop file ID for the app.This commit ensures that all
desktopplugs are collected and sorted by name first, and then for each, if thedesktop-file-idsplug is present, add its IDs to the list of common IDs for the app.This ensures that if any
desktop-file-idsattribute is present, the IDs listed always have precedence over the fallback common ID. It also ensures that if multiple plugs are present, the output is always deterministic.This fixes a bug first reported by @3v1n0
This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-36476