Bug 225230 – [webkitpy] Support pickling platforminfo

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Jonathan Bedard]

We need to be able to pickle the platforminfo object.

[Radar WebKit Bug Importer]

<rdar://problem/77384913>

[Jonathan Bedard]

Created attachment 427427 [details]
Patch

[Stephanie Lewis]

Comment on attachment 427427 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/platforminfo.py:66
>          self._platform_module = platform_module

Why not self.platform_module or platform?

[Jonathan Bedard]

Comment on attachment 427427 [details]
Patch

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

>> Tools/Scripts/webkitpy/common/system/platforminfo.py:66
>>          self._platform_module = platform_module
> 
> Why not self.platform_module or platform?

Because you can't pickle a module. Doing `self.platform_module or platform` would mean we could not pickle the PlatformInfo object

[Jonathan Bedard]

Waiting on EWS before landing.

[Sam Sneddon [:gsnedders]]

Comment on attachment 427427 [details]
Patch

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

> Tools/Scripts/webkitpy/common/system/platforminfo.py:62
> +    def __init__(self, sys_module=None, platform_module=None, executive=None):

I'm still marginally worried about making these optional, in case we forget that these to need to be passed, especially when in WebKit these are always the 'standard' objects. Do you have any idea of any approach to make sure that we remember to pass these when needed?

> Tools/Scripts/webkitpy/common/system/platforminfo.py:139
> +            return "Mac OS X %s" % (self._platform_module or platform).mac_ver()[0]

I feel like it would be clearer to just use self._platform_module everywhere and then delete it if it is platform in __getstate__ (and reintroduce it if omitted in __setstate__)?

Either that or make it self._platform_module_override and add a getter for _platform_module which does this?

[Jonathan Bedard]

(In reply to Sam Sneddon [:gsnedders] from comment #6)
> Comment on attachment 427427 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427427&action=review
> 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:62
> > +    def __init__(self, sys_module=None, platform_module=None, executive=None):
> 
> I'm still marginally worried about making these optional, in case we forget
> that these to need to be passed, especially when in WebKit these are always
> the 'standard' objects. Do you have any idea of any approach to make sure
> that we remember to pass these when needed?

PlatformInfo objects shouldn't be instantiated outside of the Host, and the defaults we're setting here are correct unless you have a specific reason to override them. It seems to me that naively instantiation the PlatformInfo object with this set of default arguments would give you what you're looking for.

The other option would be to not set defaults here, but still support "None" as an argument.

> 
> > Tools/Scripts/webkitpy/common/system/platforminfo.py:139
> > +            return "Mac OS X %s" % (self._platform_module or platform).mac_ver()[0]
> 
> I feel like it would be clearer to just use self._platform_module everywhere
> and then delete it if it is platform in __getstate__ (and reintroduce it if
> omitted in __setstate__)?
> 
> Either that or make it self._platform_module_override and add a getter for
> _platform_module which does this?

The problem I see with this approach is that if the platform_module isn't the default, it likely is able to be pickled (and that's probably what we want to do).

[EWS]

Committed r276846 (237197@main): <https://commits.webkit.org/237197@main>

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

[WebKit Commit Bot]

Re-opened since this is blocked by bug 225242

[Jonathan Bedard]

Created attachment 427443 [details]
Patch

[Aakash Jain]

rs=me

[Jonathan Bedard]

Created attachment 427608 [details]
Patch for landing

[EWS]

Committed r276935 (237271@main): <https://commits.webkit.org/237271@main>

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