snap,wrappers: fix common id selection with multiple desktop plugs - #17016
Merged
ernestl merged 3 commits intoMay 21, 2026
Conversation
olivercalder
requested review from
ZeyadYasser,
andrewphelpsj,
Copilot and
natibek
May 4, 2026 19:40
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.13% +0.05%
==========================================
Files 1376 1369 -7
Lines 191657 191547 -110
Branches 2465 2465
==========================================
+ Hits 151555 151572 +17
+ Misses 30972 30852 -120
+ Partials 9130 9123 -7
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:
|
|
Wed May 20 18:43:09 UTC 2026 Failures:Preparing:
Executing:
Restoring:
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
ZeyadYasser
left a comment
ZeyadYasser
left a comment
Contributor
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
natibek
left a comment
natibek
left a comment
Contributor
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.
andrewphelpsj
left a comment
andrewphelpsj
left a comment
Member
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.
olivercalder
added
cross-distro
olivercalder
force-pushed
the
fix-snap-common-id-selection
branch
from
May 10, 2026 14:15
7caecc2 to
ca3fb72
Compare
Member
Author
|
I considered going crazy with combinations of |
olivercalder
force-pushed
the
fix-snap-common-id-selection
branch
from
May 10, 2026 14:19
ca3fb72 to
8f286b3
Compare
Comment on lines
964
to
978
| // DesktopPlugFileIDs returns desktop-file-ids desktop plug attribute entries. | ||
| // The desktop-file-ids attribute is optional so an empty list is returned if | ||
| // the it is not found. | ||
| // | ||
| // Note: DesktopPlugFileIDs doesn't check if the desktop plug is connected because | ||
| // the desktop-file-ids attribute is controlled by an allow-installation rule. | ||
| func (s *Info) DesktopPlugFileIDs() ([]string, error) { | ||
| var desktopPlug *PlugInfo | ||
| for _, plug := range s.Plugs { | ||
| desktopPlugNames := make([]string, 0, len(s.Plugs)) | ||
| for name, plug := range s.Plugs { | ||
| if plug.Interface == "desktop" { | ||
| desktopPlug = plug | ||
| break | ||
| desktopPlugNames = append(desktopPlugNames, name) | ||
| } | ||
| } | ||
| if desktopPlug == nil { | ||
| return nil, nil | ||
| } | ||
|
|
||
| attrVal, exists := desktopPlug.Lookup("desktop-file-ids") | ||
| if !exists { | ||
| // desktop-file-ids attribute is optional | ||
| if len(desktopPlugNames) == 0 { | ||
| return nil, nil |
ZeyadYasser
approved these changes
May 19, 2026
Comment on lines
28
to
+53
| plugs: | ||
| desktop: | ||
| desktop-ids: | ||
| interface: desktop | ||
| desktop-file-ids: | ||
| - org.example.desktop | ||
| - org.example.Foo.desktop | ||
| - org.example.Fallback | ||
| - org.example.MyCommonID.desktop | ||
| - org.example.NonexistentFile.desktop | ||
| # Declare lots of desktop plugs without desktop-file-ids to make sure the | ||
| # correct desktop file ID is chosen | ||
| desktop-decoy1: | ||
| interface: desktop | ||
| desktop-decoy2: | ||
| interface: desktop | ||
| desktop-decoy3: | ||
| interface: desktop | ||
| desktop-decoy4: | ||
| interface: desktop | ||
| desktop-decoy5: | ||
| interface: desktop | ||
| desktop-decoy6: | ||
| interface: desktop | ||
| desktop-decoy7: | ||
| interface: desktop | ||
| desktop-decoy8: | ||
| interface: desktop |
Contributor
There was a problem hiding this comment.
since desktop-file-ids are getting merged anyway, might be worth adding some ids on other plugs and check they get merged.
nitpick, not a blocker for this PR
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>
Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
…ds test Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
olivercalder
force-pushed
the
fix-snap-common-id-selection
branch
from
May 20, 2026 11:45
6cb8b4a to
20181d0
Compare
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