WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
231381
Ensure webkitpy secret storage works successfully on Linux using only autoinstalled libraries
https://bugs.webkit.org/show_bug.cgi?id=231381
Summary
Ensure webkitpy secret storage works successfully on Linux using only autoins...
Michael Catanzaro
Reported
2021-10-07 11:40:17 PDT
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.
Attachments
Patch
(16.38 KB, patch)
2021-10-07 12:33 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(6.07 KB, patch)
2021-10-08 12:59 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(3.70 KB, patch)
2021-10-08 13:12 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(4.41 KB, patch)
2021-10-08 13:21 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Patch
(5.34 KB, patch)
2021-10-08 14:43 PDT
,
Michael Catanzaro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Michael Catanzaro
Comment 1
2021-10-07 12:33:21 PDT
Comment hidden (obsolete)
Created
attachment 440525
[details]
Patch
Jonathan Bedard
Comment 2
2021-10-07 12:48:59 PDT
I think it's
https://bugs.webkit.org/show_bug.cgi?id=231055
Jonathan Bedard
Comment 3
2021-10-07 12:49:58 PDT
(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
Comment 4
2021-10-07 12:52:43 PDT
(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
Comment 5
2021-10-07 19:26:50 PDT
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
Comment 6
2021-10-08 07:26:13 PDT
Seems like I will need to fix out the bug where a version like 1.2.0 matches 1.2.1
Michael Catanzaro
Comment 7
2021-10-08 08:03:09 PDT
(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
Comment 8
2021-10-08 08:37:38 PDT
(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
Comment 9
2021-10-08 09:25:55 PDT
Ah OK, interesting. I think that's not causing any trouble for *this* bug, but good to know about that footgun....
Michael Catanzaro
Comment 10
2021-10-08 12:59:30 PDT
Comment hidden (obsolete)
Created
attachment 440659
[details]
Patch
Michael Catanzaro
Comment 11
2021-10-08 12:59:47 PDT
(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
Comment 12
2021-10-08 13:12:33 PDT
Comment hidden (obsolete)
Created
attachment 440661
[details]
Patch
Michael Catanzaro
Comment 13
2021-10-08 13:13:53 PDT
(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
Comment 14
2021-10-08 13:21:57 PDT
Created
attachment 440663
[details]
Patch
Michael Catanzaro
Comment 15
2021-10-08 13:22:43 PDT
(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 16
2021-10-08 14:08:59 PDT
Comment on
attachment 440663
[details]
Patch Macs are happy with this!
Michael Catanzaro
Comment 17
2021-10-08 14:37:51 PDT
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
Comment 18
2021-10-08 14:41:39 PDT
(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
Comment 19
2021-10-08 14:43:49 PDT
Created
attachment 440675
[details]
Patch
Jonathan Bedard
Comment 20
2021-10-08 15:02:13 PDT
(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
Comment 21
2021-10-08 15:21:26 PDT
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
Comment 22
2021-10-08 15:22:15 PDT
<
rdar://problem/84046188
>
Philippe Normand
Comment 23
2021-10-13 06:24:23 PDT
Looks like this broke keyring when queried from the Flatpak SDK (again). Thanks Michael :/
Philippe Normand
Comment 24
2021-10-13 06:40:37 PDT
Hah, sorry, false alarm. After reinstalling the auto-installed python things I see jeepney is installed now, all good.
Michael Catanzaro
Comment 25
2021-10-13 07:19:39 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug