Bug 215230 – [webkitcorepy] Support wheels in the autoinstaller

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jonathan Bedard]

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.

[Radar WebKit Bug Importer]

<rdar://problem/66636527>

[Jonathan Bedard]

Created attachment 406098 [details]
Patch

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 406138 [details]
Patch

[Alexey Proskuryakov]

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?

[Jonathan Bedard]

(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.

[Stephanie Lewis]

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.

[Stephanie Lewis]

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?

[Jonathan Bedard]

(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.

[Sam Sneddon [:gsnedders]]

(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.

[Jonathan Bedard]

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.

[Jonathan Bedard]

Created attachment 406184 [details]
Patch

[EWS]

Committed r265382: <https://trac.webkit.org/changeset/265382>

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