Bug 55535 – NRWT - implement Linux Hardy 64-bit port

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

[Dirk Pranke]

We now have a 64-bit Hardy Linux bot running on the chromium canaries. We need to implement support for detecting which build we're running, a new baseline directory, and an updated baseline search path. 

We also need new baselines (tracked in bug 55527)

[Dirk Pranke]

Created attachment 84357 [details]
Patch

[Tony Chang]

I'm confused why we need the concept of chromium-linux-x86 separate from chromium-linux.  Why not just add chromium-linux-x86_64?  People on 32 bit Linux can just use chromium-linux which will contain all the 32 bit results, like they do now.

[Dirk Pranke]

(In reply to comment #2)
> I'm confused why we need the concept of chromium-linux-x86 separate from chromium-linux.  Why not just add chromium-linux-x86_64?  People on 32 bit Linux can just use chromium-linux which will contain all the 32 bit results, like they do now.

Good question. I've been attempting to move the codebase to a place where we have fully-specified versions of each ports, and then under-specified versions. So, for example, we have "chromium-mac", which could refer to either "chromium-mac-leopard" or "chromium-mac-snowleopard", depending on which platform you're running it on.

Similary, I would prefer to have "chromium-linux" and also "linux-x86" or "linux-x86_64", so that it was clear(er) what your intent was.

[Tony Chang]

Comment on attachment 84357 [details]
Patch

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:48
> +    FALLBACK_PATHS = {
> +        'x86_64': ['chromium-linux-x86_64', 'chromium-linux', 'chromium-win', 'chromium', 'win', 'mac'],
> +        'x86': ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac'],
> +    }

Nit: I would just move this inline and do something like:
fallback = ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac']
if self._architecture == 'x64_64':
    fallback.insert(0, 'chromium-linux-x86_64')

To avoid duplication of the fallback list.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:60
> +            self._architecture = port_name[port_name.index('-linux-') + 7:]

Nit: I would use .split('-')[-1] to avoid the 7 constant needing to keep in sync with -linux-.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:64
> +    def _get_architecture(self):

This names makes it seem like this will return the value of _architecture.  Maybe name this _determine_architecture or something.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:67
> +        if not self._filesystem.exists(driver_path):
> +            return 'x86'

I think we should log a message to the user if we get here.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:68
> +        file_output = self._executive.run_command(['file', driver_path])

What happens if 'file' doesn't exist or for some reason driver_path is not readable?  It would be nice if we could print the captured stdout+stderr.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:73
> +        raise AssertionError('unrecognized or unsupported architecture')

I think we should still fallback to x86 and just log a message to the user.  If people want to run this on say FreeBSD or SunOS, we should still run.

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux_unittest.py:67
> +        self.assert_architecture(file_output='ELF 32-bit LSB executable',
> +                                 expected_architecture='x86')

Should we have negative tests for stuff like chromium-gpu-linux-x86 or chromium-x86-linux?

[Dirk Pranke]

Comment on attachment 84357 [details]
Patch

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

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:48
>> +    }
> 
> Nit: I would just move this inline and do something like:
> fallback = ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac']
> if self._architecture == 'x64_64':
>     fallback.insert(0, 'chromium-linux-x86_64')
> 
> To avoid duplication of the fallback list.

This actually follows a similar structure being introduced into the other ports, and I think the duplication actually makes things clearer in the big scheme of things (otherwise you have to parse the runtime logic to figure out all of the different variants). The -leopard and -snowleopard logic is a good example of what I mean.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:60
>> +            self._architecture = port_name[port_name.index('-linux-') + 7:]
> 
> Nit: I would use .split('-')[-1] to avoid the 7 constant needing to keep in sync with -linux-.

Great idea. Will do.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:67
>> +            return 'x86'
> 
> I think we should log a message to the user if we get here.

No, this actually happens in some common scenarios, e.g. when runnning unit tests or when running --lint-test-files from a mac. 

When this actually matters (because we're trying to run the tests), we do log messages.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:68
>> +        file_output = self._executive.run_command(['file', driver_path])
> 
> What happens if 'file' doesn't exist or for some reason driver_path is not readable?  It would be nice if we could print the captured stdout+stderr.

Good idea, I'll add that.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:73
>> +        raise AssertionError('unrecognized or unsupported architecture')
> 
> I think we should still fallback to x86 and just log a message to the user.  If people want to run this on say FreeBSD or SunOS, we should still run.

Hm. I don't know if I agree with that, but I can make the change for now.

>> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux_unittest.py:67
>> +                                 expected_architecture='x86')
> 
> Should we have negative tests for stuff like chromium-gpu-linux-x86 or chromium-x86-linux?

It's a good question. As you've suggested, having the components of the configuration be order-dependent is actually kinda fragile. Eventually I'd like to clean up the code and make things less fragile, but I can add a couple of tests now to ensure that we at least fail predictably.

[Tony Chang]

(In reply to comment #5)
> (From update of attachment 84357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84357&action=review
> 
> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:48
> >> +    }
> > 
> > Nit: I would just move this inline and do something like:
> > fallback = ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac']
> > if self._architecture == 'x64_64':
> >     fallback.insert(0, 'chromium-linux-x86_64')
> > 
> > To avoid duplication of the fallback list.
> 
> This actually follows a similar structure being introduced into the other ports, and I think the duplication actually makes things clearer in the big scheme of things (otherwise you have to parse the runtime logic to figure out all of the different variants). The -leopard and -snowleopard logic is a good example of what I mean.

Another way to do this is:
class ChromiumLinuxPort(chromium.ChromiumPort):
  FALLBACK_PATH = {
    'x86': [...],
  }
  FALLBACK_PATH['x86_64'] = ['chromium-linux-x86_64'] + FALLBACK_PATH['x86']

[Dirk Pranke]

Created attachment 84491 [details]
update w/ review feedback from tony

[Dirk Pranke]

> >> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:68
> >> +        file_output = self._executive.run_command(['file', driver_path])
> > 
> > What happens if 'file' doesn't exist or for some reason driver_path is not readable?  It would be nice if we could print the captured stdout+stderr.
> 

So, run_command will raise a ScriptError(); if I pass return_stderr=True, it'll include that. I think that'll be good enough and I'll add a test for it.

[Dirk Pranke]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 84357 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=84357&action=review
> > 
> > >> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:48
> > >> +    }
> > > 
> > > Nit: I would just move this inline and do something like:
> > > fallback = ['chromium-linux', 'chromium-win', 'chromium', 'win', 'mac']
> > > if self._architecture == 'x64_64':
> > >     fallback.insert(0, 'chromium-linux-x86_64')
> > > 
> > > To avoid duplication of the fallback list.
> > 
> > This actually follows a similar structure being introduced into the other ports, and I think the duplication actually makes things clearer in the big scheme of things (otherwise you have to parse the runtime logic to figure out all of the different variants). The -leopard and -snowleopard logic is a good example of what I mean.
> 
> Another way to do this is:
> class ChromiumLinuxPort(chromium.ChromiumPort):
>   FALLBACK_PATH = {
>     'x86': [...],
>   }
>   FALLBACK_PATH['x86_64'] = ['chromium-linux-x86_64'] + FALLBACK_PATH['x86']

Yeah, I still think the duplication is clearer.

[Tony Chang]

Comment on attachment 84491 [details]
update w/ review feedback from tony

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

> Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:59
> +            if port_name.startswith('chromium-linux'):

Can this be == instead of startswith?

> Tools/Scripts/webkitpy/layout_tests/port/google_chrome.py:71
> +            def architecture(self):
> +                # Because we're specifying chromium-linux-86 below, we
> +                # don't really need this line, but it is probably slightly
> +                # clearer to have it.
> +                return 'x86'

I find this to be noisy.  I would just remove it.

> Tools/Scripts/webkitpy/layout_tests/port/google_chrome.py:92
> +        return GoogleChromeLinux64Port(port_name='chromium-linux-x86', **kwargs)

This might use a comment explaining why it's x86 even though we're linux 64.

[Dirk Pranke]

Created attachment 84495 [details]
more review feedback from tony

[Dirk Pranke]

(In reply to comment #10)
> (From update of attachment 84491 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=84491&action=review
> 
> > Tools/Scripts/webkitpy/layout_tests/port/chromium_linux.py:59
> > +            if port_name.startswith('chromium-linux'):
> 
> Can this be == instead of startswith?
> 

Yes. Done.

> > Tools/Scripts/webkitpy/layout_tests/port/google_chrome.py:71
> > +            def architecture(self):
> > +                # Because we're specifying chromium-linux-86 below, we
> > +                # don't really need this line, but it is probably slightly
> > +                # clearer to have it.
> > +                return 'x86'
> 
> I find this to be noisy.  I would just remove it.
>

Done.
 
> > Tools/Scripts/webkitpy/layout_tests/port/google_chrome.py:92
> > +        return GoogleChromeLinux64Port(port_name='chromium-linux-x86', **kwargs)
> 
> This might use a comment explaining why it's x86 even though we're linux 64.

Done.

[Dirk Pranke]

Committed r80192: <http://trac.webkit.org/changeset/80192>