Fix EWS python unit tests and address code comments as per 140476
Created attachment 244920 [details] Patch
Martin/Carlos, is the default architecture in ews.json correct for GTK? Gyuyoung, is the default correct for EFL?
Comment on attachment 244920 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244920&action=review I'm unclear why we need logic for the default architecture as opposed to letting the tools that the EWS call (e.g. build-webkit) determine the default architecture. Is it not sufficient to only teach webkit-patch and the EWS to pass the specified architecture (given as a command line argument when you start the EWS) to the tools is calls? I mean, it seems weird that we have logic to determine the default architecture for the EWS code given that we have existing logic to compute a default architecture in webkitdirs.pm, which is used by build-webkit and other Perl tools. > Tools/Scripts/webkitpy/common/config/ews.json:4 > + "architecture": "x86", We'll need to confirm this. > Tools/Scripts/webkitpy/common/config/ews.json:11 > + "architecture": "x86", Ditto. > Tools/Scripts/webkitpy/common/config/ews.json:18 > + "architecture": "x86" I'm surprised we don't have 64-bit hardware. > Tools/Scripts/webkitpy/port/base.py:108 > + if not self.get_option('architecture'): > + self.set_option('architecture', self.DEFAULT_ARCHITECTURE) We can simplify this code by taking advantage of the convenience function set_option_default() that conditionally sets the value of the specified attribute to the specified value if it doesn't already have a value. self.set_option_default('architecture', self.DEFAULT_ARCHITECTURE) > Tools/Scripts/webkitpy/tool/commands/queues.py:77 > + if not hasattr(self, 'architecture'): How did you come to the decision to check for the existence of the attribute architecture as opposed to setting it directly? I take it you're worried about the order of calling super() in a derived class?
(In reply to comment #3) > Comment on attachment 244920 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=244920&action=review > > I'm unclear why we need logic for the default architecture as opposed to > letting the tools that the EWS call (e.g. build-webkit) determine the > default architecture. Is it not sufficient to only teach webkit-patch and > the EWS to pass the specified architecture (given as a command line argument > when you start the EWS) to the tools is calls? I mean, it seems weird that > we have logic to determine the default architecture for the EWS code given > that we have existing logic to compute a default architecture in > webkitdirs.pm, which is used by build-webkit and other Perl tools. > > > Tools/Scripts/webkitpy/common/config/ews.json:4 > > + "architecture": "x86", > > We'll need to confirm this. > > > Tools/Scripts/webkitpy/common/config/ews.json:11 > > + "architecture": "x86", > > Ditto. > > > Tools/Scripts/webkitpy/common/config/ews.json:18 > > + "architecture": "x86" > > I'm surprised we don't have 64-bit hardware. ^ I'll remove these from ews.json > > > Tools/Scripts/webkitpy/port/base.py:108 > > + if not self.get_option('architecture'): > > + self.set_option('architecture', self.DEFAULT_ARCHITECTURE) > > We can simplify this code by taking advantage of the convenience function > set_option_default() that conditionally sets the value of the specified > attribute to the specified value if it doesn't already have a value. > > self.set_option_default('architecture', self.DEFAULT_ARCHITECTURE) I agree, that is simpler. Unfortunately we now need a state variable, so we're going to need to check for the existence of the architecture option anyway. > > > Tools/Scripts/webkitpy/tool/commands/queues.py:77 > > + if not hasattr(self, 'architecture'): > > How did you come to the decision to check for the existence of the attribute > architecture as opposed to setting it directly? I take it you're worried > about the order of calling super() in a derived class? Yes. In particular when the EarlyWarningSystem classes are dynamically created, they are created with the architecture attribute before the initializer runs. Without this check they get set to None erroneously.
Created attachment 244944 [details] Patch
Comment on attachment 244944 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=244944&action=review r=me with the suggested fixes. > Tools/Scripts/webkitpy/port/base.py:104 > + # This member variable shouldn't have to exist, but it will be needed until the correct > + # default architectures for the GTK and EFL EWS bots are known. > + # FIXME: Remove this member variable. Please reword this as: # FIXME: This can be removed once default architectures for GTK and EFL EWS bots are set. > Tools/Scripts/webkitpy/port/base.py:105 > + self.did_override_architecture = True Please set this to False by default, and only set it to true when the setter is called. > Tools/Scripts/webkitpy/port/base.py:113 > + self.did_override_architecture = False Remove this line if self.did_override_architecture defaults to False. > Tools/Scripts/webkitpy/port/ios.py:49 > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8', 'ios-9'] Remove 'ios-9' here since we don't need a fallback for ios-9 until ios-10 exists. > Tools/Scripts/webkitpy/port/ios.py:50 > + DEFAULT_ARCHITECTURE = 'armv7' Nit: Put this after ARCHITECTURES but before VERSION_FALLBACK_ORDER. > Tools/Scripts/webkitpy/tool/commands/queues.py:282 > + self._port.set_architecture(self.architecture) Should probably only set this if self.architecture is not None? if self.architecture: self._port.set_architecture(self.architecture)
Created attachment 245015 [details] Patch
Comment on attachment 245015 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=245015&action=review r=me > Tools/Scripts/webkitpy/port/ios.py:50 > + VERSION_FALLBACK_ORDER = ['ios-7', 'ios-8', 'ios-9'] Talked to Jake in person; it's fine to add this for now.
Comment on attachment 245015 [details] Patch Clearing flags on attachment: 245015 Committed r178757: <http://trac.webkit.org/changeset/178757>
All reviewed patches have been landed. Closing bug.