We need to be able to pickle the platforminfo object.
<rdar://problem/77384913>
Created attachment 427427 [details] Patch
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?
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
Waiting on EWS before landing.
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?
(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).
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].
Re-opened since this is blocked by bug 225242
Created attachment 427443 [details] Patch
rs=me
Created attachment 427608 [details] Patch for landing
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].