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)
Created attachment 84357 [details] Patch
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.
(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.
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?
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.
(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']
Created attachment 84491 [details] update w/ review feedback from tony
> >> 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.
(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.
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.
Created attachment 84495 [details] more review feedback from tony
(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.
Committed r80192: <http://trac.webkit.org/changeset/80192>