Bug 64225 – nrwt: linting fixes

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dirk Pranke]

nrwt: linting fixes

[Dirk Pranke]

Created attachment 100201 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 100201 [details]
Patch

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

Looks good.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:225
>          self.msg = reason

We should have probably just passed reason as msg to the constructor.

> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:757
> +        for _ in xrange(num_workers):
>              manager_connection.post_message('stop')

Huh?  Why do we post the same message num_workers times?  Does that guarentee that each gets the message?  I assume "stop" is the last mesage any worker will grab?

> Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:-127
> -        raise NotImplementedError

Yay!  Throwing NotImplemented from a constructor seems wrong to me. :)  I'm glad pylint agrees.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:55
> +        self._filesystem = None

Huh?  It's passed a filesystem later?

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:70
>          This routine exists so that the mixin can be created and then marshaled
>          across into a child process."""
>          self._port = port
> -        self._filesystem = port._filesystem
> +        self._filesystem = port.filesystem

I'm very confused by the existance of this safe_init method.  This smells wrong.

> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:239
> -        result = getattr(thread, 'result', None)
> +        result = thread.result

Die evil get/has/setattr calls! Die, die!

> Tools/Scripts/webkitpy/layout_tests/port/base.py:196
> +            _ = self._executive.run_command(['ruby', '--version'])

Um.  Just remove the _= :)

> Tools/Scripts/webkitpy/layout_tests/port/base.py:218
> +            _ = self._executive.run_command([self._path_to_wdiff(), '--help'])

Same here.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:275
> +        def to_raw_bytes(string_value):
> +            if isinstance(string_value, unicode):
> +                return string_value.encode('utf-8')
> +            return string_value

Ick.  This smells wrong too.  We should know what type of data we're passing around...

> Tools/Scripts/webkitpy/layout_tests/port/base.py:525
> +    def skipped_layout_tests(self):
> +        return []

I wonder what prompted this addition.

> Tools/Scripts/webkitpy/layout_tests/port/base.py:951
> -        for system in self.all_systems():
> +        for version, architecture in self.all_systems():

much nicer!

[Dirk Pranke]

(In reply to comment #2)
> (From update of attachment 100201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100201&action=review
> 
> Looks good.
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:225
> >          self.msg = reason
> 
> We should have probably just passed reason as msg to the constructor.
> 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:757
> > +        for _ in xrange(num_workers):
> >              manager_connection.post_message('stop')
> 
> Huh?  Why do we post the same message num_workers times?  Does that guarentee that each gets the message?  I assume "stop" is the last mesage any worker will grab?
>

Yes, each worker needs a message. When the worker receives that message, it exits the message loop (and the run() method of the thread/process).
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/manager_worker_broker.py:-127
> > -        raise NotImplementedError
> 
> Yay!  Throwing NotImplemented from a constructor seems wrong to me. :)  I'm glad pylint agrees.
> 

Admittedly it feels a little weird to me, but I couldn't think of a better way to make the class be pure virtual. I guess that Python/pylint doesn't even want me to try :).

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:55
> > +        self._filesystem = None
> 
> Huh?  It's passed a filesystem later?
> 

Yes (see below).

> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:70
> >          This routine exists so that the mixin can be created and then marshaled
> >          across into a child process."""
> >          self._port = port
> > -        self._filesystem = port._filesystem
> > +        self._filesystem = port.filesystem
> 
> I'm very confused by the existance of this safe_init method.  This smells wrong.
>

This has to do with the way objects are passed across the process boundaries between master and worker processes. The object must be Picklable, and some of the things in the Port and filesystem objects can't be; this means that we have to re-create some of the objects in the subprocess (which then leads to the port.real_name() method that Adam noticed a week ago).

So, we can only partially initialize the object in the constructor (which is called in the master process), which is then pickled, sent to the child process, and unpickled there, where we then provide the rest of the parameters the worker needs in safe_init() [which, admittedly, isn't a great name].
 
> > Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:239
> > -        result = getattr(thread, 'result', None)
> > +        result = thread.result
> 
> Die evil get/has/setattr calls! Die, die!
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:196
> > +            _ = self._executive.run_command(['ruby', '--version'])
> 
> Um.  Just remove the _= :)
> 

I wonder if pylint would then complain that we were ignoring the return value. I have a long-standing habit of doing things like this to be (IMO) clearer that this is being intentionally ignored (left over from C/C++).

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:218
> > +            _ = self._executive.run_command([self._path_to_wdiff(), '--help'])
> 
> Same here.
> 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:275
> > +        def to_raw_bytes(string_value):
> > +            if isinstance(string_value, unicode):
> > +                return string_value.encode('utf-8')
> > +            return string_value
> 
> Ick.  This smells wrong too.  We should know what type of data we're passing around...
> 

Hm. I wonder if this is a leftover from when we were attempting to make everything unicode, and if I can just delete it.

> > Tools/Scripts/webkitpy/layout_tests/port/base.py:525
> > +    def skipped_layout_tests(self):
> > +        return []
> 
> I wonder what prompted this addition.
>

skips_layout_tests(), right below, calls this routine, but it wasn't defined in the base class.
 
> > Tools/Scripts/webkitpy/layout_tests/port/base.py:951
> > -        for system in self.all_systems():
> > +        for version, architecture in self.all_systems():
> 
> much nicer!

[Eric Seidel (no email)]

Comment on attachment 100201 [details]
Patch

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

>>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:70
>>> +        self._filesystem = port.filesystem
>> 
>> I'm very confused by the existance of this safe_init method.  This smells wrong.
> 
> This has to do with the way objects are passed across the process boundaries between master and worker processes. The object must be Picklable, and some of the things in the Port and filesystem objects can't be; this means that we have to re-create some of the objects in the subprocess (which then leads to the port.real_name() method that Adam noticed a week ago).
> 
> So, we can only partially initialize the object in the constructor (which is called in the master process), which is then pickled, sent to the child process, and unpickled there, where we then provide the rest of the parameters the worker needs in safe_init() [which, admittedly, isn't a great name].

OK.  So I think we shuld consider renaming safe-init in a separate patch.  Some sort of construct_from_pickle?  I don't feel like I understand the design well enough to advise here.

>> Tools/Scripts/webkitpy/layout_tests/port/base.py:275
>> +            return string_value
> 
> Ick.  This smells wrong too.  We should know what type of data we're passing around...

All code should know if it's handling unicode or str (bytes in py3 iirc) objects.  If it ever doesn't know, then we're confused.

>>> Tools/Scripts/webkitpy/layout_tests/port/base.py:525
>>> +        return []
>> 
>> I wonder what prompted this addition.
> 
> skips_layout_tests(), right below, calls this routine, but it wasn't defined in the base class.

I see.  So it doesn't need/want to be NotImplemented since empty is the right implementation for chromium, I assume?  (Which reminds me, eventually Chromium should be a subclass of WebKit and base.port will die... but that's a long ways off)

[Dirk Pranke]

(In reply to comment #4)
> (From update of attachment 100201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100201&action=review
> 
> >>> Tools/Scripts/webkitpy/layout_tests/controllers/worker.py:70
> >>> +        self._filesystem = port.filesystem
> >> 
> >> I'm very confused by the existance of this safe_init method.  This smells wrong.
> > 
> > This has to do with the way objects are passed across the process boundaries between master and worker processes. The object must be Picklable, and some of the things in the Port and filesystem objects can't be; this means that we have to re-create some of the objects in the subprocess (which then leads to the port.real_name() method that Adam noticed a week ago).
> > 
> > So, we can only partially initialize the object in the constructor (which is called in the master process), which is then pickled, sent to the child process, and unpickled there, where we then provide the rest of the parameters the worker needs in safe_init() [which, admittedly, isn't a great name].
> 
> OK.  So I think we shuld consider renaming safe-init in a separate patch.  Some sort of construct_from_pickle?  I don't feel like I understand the design well enough to advise here.
> 

That's up to you ... 

> >> Tools/Scripts/webkitpy/layout_tests/port/base.py:275
> >> +            return string_value
> > 
> > Ick.  This smells wrong too.  We should know what type of data we're passing around...
> 
> All code should know if it's handling unicode or str (bytes in py3 iirc) objects.  If it ever doesn't know, then we're confused.
> 

Agreed.

> >>> Tools/Scripts/webkitpy/layout_tests/port/base.py:525
> >>> +        return []
> >> 
> >> I wonder what prompted this addition.
> > 
> > skips_layout_tests(), right below, calls this routine, but it wasn't defined in the base class.
> 
> I see.  So it doesn't need/want to be NotImplemented since empty is the right implementation for chromium, I assume?  (Which reminds me, eventually Chromium should be a subclass of WebKit and base.port will die... but that's a long ways off)

Agreed.

[Dirk Pranke]

Committed r90796: <http://trac.webkit.org/changeset/90796>