Bug 215230 - [webkitcorepy] Support wheels in the autoinstaller
Summary: [webkitcorepy] Support wheels in the autoinstaller
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jonathan Bedard
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-08-06 11:23 PDT by Jonathan Bedard
Modified: 2020-08-07 11:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.52 KB, patch)
2020-08-06 11:51 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.90 KB, patch)
2020-08-06 17:36 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2020-08-07 10:24 PDT, Jonathan Bedard
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Bedard 2020-08-06 11:23:30 PDT
Some Python packages have binary distributions. While pip in theory manages these, package authors are often conservative when declaring which configurations their packages are actually compatible with. The autoinstaller should be quite a bit more permissive then pip when finding compatible .whls.
Comment 1 Radar WebKit Bug Importer 2020-08-06 11:23:53 PDT
<rdar://problem/66636527>
Comment 2 Jonathan Bedard 2020-08-06 11:51:10 PDT
Created attachment 406098 [details]
Patch
Comment 3 Jonathan Bedard 2020-08-06 14:15:09 PDT
Comment on attachment 406098 [details]
Patch

I want to have folks look at this, but there is a better way to do the whl file filtering that I'm looking into, I don't think this is ready to land.
Comment 4 Jonathan Bedard 2020-08-06 17:36:41 PDT
Created attachment 406138 [details]
Patch
Comment 5 Alexey Proskuryakov 2020-08-06 17:53:23 PDT
Comment on attachment 406138 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:453
> +            # For compatibility reasons, it's useful to pretend to be 10.16

When can we remove this?
Comment 6 Jonathan Bedard 2020-08-06 22:27:37 PDT
(In reply to Alexey Proskuryakov from comment #5)
> Comment on attachment 406138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406138&action=review
> 
> > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:453
> > +            # For compatibility reasons, it's useful to pretend to be 10.16
> 
> When can we remove this?

Probably when https://github.com/pypa/packaging/pull/319 lands, but the commentary on that bug indicates there may be a bit more to it.

I think it's likely we'll need things like this in the future, though, even once this specific issue is resolved.
Comment 7 Stephanie Lewis 2020-08-07 09:56:55 PDT
Comment on attachment 406138 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:282
> +            for item in list(already_owned):

Whats the point of this

>>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:453
>>> +            # For compatibility reasons, it's useful to pretend to be 10.16
>> 
>> When can we remove this?
> 
> Probably when https://github.com/pypa/packaging/pull/319 lands, but the commentary on that bug indicates there may be a bit more to it.
> 
> I think it's likely we'll need things like this in the future, though, even once this specific issue is resolved.

Can we add this information to the comment.  The comment should have enough information so that down the road we can figure out whether the workaround is still needed.
Comment 8 Stephanie Lewis 2020-08-07 09:57:37 PDT
Comment on attachment 406138 [details]
Patch

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

> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:420
> +        for alias in package.aliases:

Are aliases related to wheels or is this different functionality in the same patch?
Comment 9 Jonathan Bedard 2020-08-07 10:03:27 PDT
(In reply to Stephanie Lewis from comment #8)
> Comment on attachment 406138 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=406138&action=review
> 
> > Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:420
> > +        for alias in package.aliases:
> 
> Are aliases related to wheels or is this different functionality in the same
> patch?

Not really, I can put it in a different patch.  Basically, this lets one declare that 'import _requests' should also trigger the auto-installer for the requests package.
Comment 10 Sam Sneddon [:gsnedders] 2020-08-07 10:05:36 PDT
(In reply to Stephanie Lewis from comment #7)
> Can we add this information to the comment.  The comment should have enough
> information so that down the road we can figure out whether the workaround
> is still needed.

Strongly in favour of this.

Also maybe pass feedback back up to the Discourse thread that at least WebKit is feeling a necessity to workaround this? Even without AS support landing upstream, support for Big Sur on Intel should probably land sooner rather than later.
Comment 11 Jonathan Bedard 2020-08-07 10:06:45 PDT
Comment on attachment 406138 [details]
Patch

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

>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:282
>> +            for item in list(already_owned):
> 
> Whats the point of this

We can't iterate through a set and remove elements from the set.

The reason we need it is a more general problem that setup.py also maybe has, and that is it's possible that a library (or part of a library) was already installed, so even though a directory existed before, it still needs to be re-owned.

I suppose we could get rid of this by just running AutoInstall.userspace_should_own(install_location).

>>>> Tools/Scripts/libraries/webkitcorepy/webkitcorepy/autoinstall.py:453
>>>> +            # For compatibility reasons, it's useful to pretend to be 10.16
>>> 
>>> When can we remove this?
>> 
>> Probably when https://github.com/pypa/packaging/pull/319 lands, but the commentary on that bug indicates there may be a bit more to it.
>> 
>> I think it's likely we'll need things like this in the future, though, even once this specific issue is resolved.
> 
> Can we add this information to the comment.  The comment should have enough information so that down the road we can figure out whether the workaround is still needed.

Will update the comment.
Comment 12 Jonathan Bedard 2020-08-07 10:24:01 PDT
Created attachment 406184 [details]
Patch
Comment 13 EWS 2020-08-07 11:06:30 PDT
Committed r265382: <https://trac.webkit.org/changeset/265382>

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