portal-impl: fix config ordering #1240
Conversation
GeorgesStavracas
added
bug
config
alyssais
force-pushed
the
order
branch
2 times, most recently
from
27d2100
to
2321ed8
Compare
There was a problem hiding this comment.
LGTM with the final comment above, but I'd like to have @ebassi's review before landing
Quoting portals.conf(5): > Each key in the group contains a semi-colon separated list of portal backend > implementation, to be searched for an implementation of the requested interface, > in the same order as specified in the configuration file. But this wasn't actually true. If the portals were set to z;y and z and y both implemented the same portal, y would be used. Fixing this requires reworking how portals are selected — going through the config file and selecting the first available configured portal, rather than going through each known portal and checking whether it's allowed in the config file. find_all_portal_implementations() is unchanged. The portal-first approach is fine for it, and it would be difficult for it to share as much code with find_portal_implementation() now that it's written to stop as soon as a configured portal is found. Fixes: flatpak#1111
|
ping @ebassi |
|
(I'm not sure to what extent 1394bb2 affected this — there's definitely some overlap, because I also fixed that bug. But given it's caused complicated merge conflicts, and it was merged instead of this despite my PR being open way longer, I want to feel confident that's not going to happen again before I put any more time in.) |
Hi @alyssais, I'm the author of 1394bb2. I was aware of this PR when I wrote that patch, and I tried it out see if the bug was already fixed (it didn't). I think the only reason that my PR was merged faster than yours was that it was bumped up by several users from the Arch community. I think this is a very important PR that I think should get merged since it has been the cause of bugs like this in the distribution that I contribute to. We currently use a very hacky workaround, which this PR would remove the need for. |
|
LMK if you need any help with testing or resolving conflicts. |
|
@SoumyaRanjanPatnaik if you want to resolve the conflicts that'd be great! I had a go but I found it difficult because it's been so long I've forgotten a lot of the nuances I was trying to get right here. |
|
@SoumyaRanjanPatnaik, if you can provide a separate PR that supersedes this one, using |
Quoting portals.conf(5):
But this wasn't actually true. If the portals were set to z;y and z and y both implemented the same portal, y would be used.
Fixing this requires reworking how portals are selected — going through the config file and selecting the first available configured portal, rather than going through each known portal and checking whether it's allowed in the config file.
find_all_portal_implementations() is unchanged. The portal-first approach is fine for it, and it would be difficult for it to share as much code with find_portal_implementation() now that it's written to stop as soon as a configured portal is found.
Fixes: #1111