Bug 231381 – Ensure webkitpy secret storage works successfully on Linux using only autoinstalled libraries

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Michael Catanzaro]

Seems webkit-patch's keyring integration has broken? To test, I need a bug report to avoid spamming other bugs with spam patches by mistake.

[Michael Catanzaro]

Created attachment 440525 [details]
Patch

[Jonathan Bedard]

I think it's https://bugs.webkit.org/show_bug.cgi?id=231055

[Jonathan Bedard]

(In reply to Jonathan Bedard from comment #2)
> I think it's https://bugs.webkit.org/show_bug.cgi?id=231055

Never mind, thought that change modified libraries, it does not.

[Michael Catanzaro]

(In reply to Jonathan Bedard from comment #3)
> Never mind, thought that change modified libraries, it does not.

First problem is that python-secretstorage is not being autoinstalled, it needs to be registered as an implicit dep of python-keyring when running on Linux.

But that's not enough, something else is wrong too. I will try to debug.

[Michael Catanzaro]

Next problem is I tried updating to the latest python-keyring 23.2.1 (only when called from python3, since it is incompatible with python2), but it throws an exception whenever using any of the public APIs. I've tracked it down to _load_plugins in backend.py which is calling importlib.metadata.entry_points() https://docs.python.org/3.10/library/importlib.metadata.html#entry-points using the `group` keyword argument, which is documented to exist in python 3.10, but webkitpy's autoinstaller overrides the stdlib version with version 1.7.0 from pypy, which has an incompatible API. Not sure how hard it is to upgrade that yet.

When I downgrade to python-keyring 22.3.0, the last version compatible with our importlib.metadata, then I get past that error and uncover another exception, which I will need to investigate tomorrow.

(I also tried not updating anything, but that didn't work either, and it feels more productive to debug the newer versions rather than older versions.)

[Jonathan Bedard]

Seems like I will need to fix out the bug where a version like 1.2.0 matches 1.2.1

[Michael Catanzaro]

(In reply to Jonathan Bedard from comment #6)
> Seems like I will need to fix out the bug where a version like 1.2.0 matches
> 1.2.1

Um... I didn't know about this? What bug is this?

[Jonathan Bedard]

(In reply to Michael Catanzaro from comment #7)
> (In reply to Jonathan Bedard from comment #6)
> > Seems like I will need to fix out the bug where a version like 1.2.0 matches
> > 1.2.1
> 
> Um... I didn't know about this? What bug is this?

This bug: https://bugs.webkit.org/show_bug.cgi?id=230540, looking at a fix now

[Michael Catanzaro]

Ah OK, interesting. I think that's not causing any trouble for *this* bug, but good to know about that footgun....

[Michael Catanzaro]

Created attachment 440659 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #5) 
> When I downgrade to python-keyring 22.3.0, the last version compatible with
> our importlib.metadata, then I get past that error and uncover another
> exception, which I will need to investigate tomorrow.

Well I'm not sure what I changed, but I'm no longer seeing the same error as yesterday. Today I'm seeing python-secretstorage is broken due to a missing dependency on python-jeepney, which causes an ImportError when keyring imports secretstorage, causing the backend to be disabled. Autoinstalling jeepney fixes it.

[Michael Catanzaro]

Created attachment 440661 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #4)
> First problem is that python-secretstorage is not being autoinstalled, it
> needs to be registered as an implicit dep of python-keyring when running on
> Linux.

Hm, it works fine for me today. python-keyring does import python-secretstorage, so it shouldn't need to be listed as an implicit dep. Not sure why I convinced myself this was required yesterday. Maybe it was required with older versions of the libraries, but it works fine for me with the newer versions.

[Michael Catanzaro]

Created attachment 440663 [details]
Patch

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #14)
> Created attachment 440663 [details]
> Patch

OK, this is a real patch, not a test patch. Danger of using 'webkit-patch upload' as a test case is that it uploads a patch when your changes work. :P

[Jonathan Bedard]

Comment on attachment 440663 [details]
Patch

Macs are happy with this!

[Michael Catanzaro]

Comment on attachment 440663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=440663&action=review

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/__init__.py:75
> +if sys.version_info > (3, 6):

Oops, should be >=

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #17)
> Oops, should be >=

Also forgot to edit the webcorepy version number in setup.up and this __init__.py, although I don't quite understand why we are doing that.

[Michael Catanzaro]

Created attachment 440675 [details]
Patch

[Jonathan Bedard]

(In reply to Michael Catanzaro from comment #18)
> (In reply to Michael Catanzaro from comment #17)
> > Oops, should be >=
> 
> Also forgot to edit the webcorepy version number in setup.up and this
> __init__.py, although I don't quite understand why we are doing that.

Apple has a pypi instance we're uploading this to, the version numbers indicate a new release.

Not a huge deal with changes like this, though. It's more important when APIs are changing.

[EWS]

Committed r283843 (242723@main): <https://commits.webkit.org/242723@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 440675 [details].

[Radar WebKit Bug Importer]

<rdar://problem/84046188>

[Philippe Normand]

Looks like this broke keyring when queried from the Flatpak SDK (again). Thanks Michael :/

[Philippe Normand]

Hah, sorry, false alarm. After reinstalling the auto-installed python things I see jeepney is installed now, all good.

[Michael Catanzaro]

Unfortunately all the keyring stuff is quite fragile. :/ I've tried to make it a little more robust by relying on the autoinstaller, but it will surely break again next time we update any of these dependencies.

If anyone else has trouble after this, please delete your autoinstalled directories Tools/Scripts/libraries/autoinstalled and Tools/Scripts/webkitpy/autoinstalled. That will trigger the autoinstaller to run again and refresh everything.