Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2821543003:
In run-webkit-tests, start xvfb if necessary. (Closed)
DescriptionIn run-webkit-tests, start xvfb if necessary.
BUG=711515
Review-Url: https://codereview.chromium.org/2821543003
Cr-Commit-Position: refs/heads/master@{#465331}
Committed: https://chromium.googlesource.com/chromium/src/+/8e209a99612f0af2a91c940967eacef653fa20b1
Patch Set 1 #Patch Set 2 : Add test #Patch Set 3 : Fix debug log #
Total comments: 14
Patch Set 4 : Always try to start xvfb, looping to find display; don't alter env var DISPLAY #Patch Set 5 : Always try to start xvfb, looping to find display #Patch Set 6 : Check process after starting, wait after killing, change stderr redirect to DEVNULL #
Messages
Total messages: 15 (5 generated)
qyearsley@chromium.org changed reviewers: + dpranke@chromium.org, tansell@chromium.org
https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:335: self._port.setup_test_run() I moved this line here so that it happens before the system dependency check on linux, and so that starting xvfb can be done in Port.setup_test_run, which seemed like the most obvious place to do it. But, this might change behavior. The alternatives would be: - Add a separate _start_xvfb_if_necessary and call it here, and keep setup_test_run down below. - In the linux port, make check_sys_deps responsible for starting xvfb if necessary. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:151: display = self.host.environ.get('DISPLAY') or ':20' This choice of :20 is arbitrary. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:155: ['Xvfb', display, '-screen', '0', '1280x800x24', '-ac', '-dpi', '96'], Not sure if all of these options are necessary. - `man Xvfb` says that if -screen is not given, then the default is screen 0, 1280x1024x8. - `man Xserver` says that if -dpi is not given, it's determined automatically (or guessed?), and "-ac" disables host-based access control mechanisms, and should be used with caution. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:172: self._xvfb_process.kill() Not sure whether it would be prudent to do anything else here; presumably since Xvfb is a subprocess of python run-webkit-tests, then it will be cleaned up even if for some reason it fails to be killed here?
https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:335: self._port.setup_test_run() On 2017/04/14 18:25:40, qyearsley wrote: > I moved this line here so that it happens before the system dependency check on > linux, and so that starting xvfb can be done in Port.setup_test_run, which > seemed like the most obvious place to do it. But, this might change behavior. > > The alternatives would be: > - Add a separate _start_xvfb_if_necessary and call it here, and keep > setup_test_run down below. > - In the linux port, make check_sys_deps responsible for starting xvfb if > necessary. I think moving the order of the calls is fine, but you should move everything before check_sys_deps(), i.e., call that last in the function so that everything else is ready as much as possible. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:151: display = self.host.environ.get('DISPLAY') or ':20' On 2017/04/14 18:25:40, qyearsley wrote: > This choice of :20 is arbitrary. Don't use the existing DISPLAY value, that'll just confuse things. Also, you shouldn't alter the existing environ, just make sure that the new value gets propagated to the driver in setup_environ_for_server(). You might want to use :99 for compatibility w/ xvfb-run, and also be prepared to loop to find a free display. And, check the error code from the subprocess.
https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:151: display = self.host.environ.get('DISPLAY') or ':20' On 2017/04/14 at 21:44:08, Dirk Pranke wrote: > On 2017/04/14 18:25:40, qyearsley wrote: > > This choice of :20 is arbitrary. > > Don't use the existing DISPLAY value, that'll just confuse things. Also, you shouldn't alter the existing environ, just make sure that the new value gets propagated to the driver in setup_environ_for_server(). There's one complication here: a collection of new Port objects is created for each Worker, so if the display found (e.g. :99 or :100) during setup is stored on the original Port object itself that won't work, since new Ports are created for each new Worker. I think we could store the display found on the Host or somewhere else to ensure that is available when creating and setting up Workers, but this might not be as simple. If we alter the run-webkit-tests process environment, though, then that also works (all of the subprocesses get the new DISPLAY), and this wouldn't affect the value of DISPLAY after run-webkit-tests exits. Is it still a good idea to avoid setting the existing DISPLAY environment variable? > You might want to use :99 for compatibility w/ xvfb-run, and also be prepared to loop to find a free display. Done. > And, check the error code from the subprocess. Which subprocess? The Xvfb process,by calling self._xvfb_process.wait() after self._xvfb_process.kill()?
https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:151: display = self.host.environ.get('DISPLAY') or ':20' On 2017/04/18 01:02:39, qyearsley wrote: > On 2017/04/14 at 21:44:08, Dirk Pranke wrote: > > On 2017/04/14 18:25:40, qyearsley wrote: > > > This choice of :20 is arbitrary. > > > > Don't use the existing DISPLAY value, that'll just confuse things. Also, you > shouldn't alter the existing environ, just make sure that the new value gets > propagated to the driver in setup_environ_for_server(). > > There's one complication here: a collection of new Port objects is created for > each Worker, so if the display found (e.g. :99 or :100) during setup is stored > on the original Port object itself that won't work, since new Ports are created > for each new Worker. > > I think we could store the display found on the Host or somewhere else to ensure > that is available when creating and setting up Workers, but this might not be as > simple. > > If we alter the run-webkit-tests process environment, though, then that also > works (all of the subprocesses get the new DISPLAY), and this wouldn't affect > the value of DISPLAY after run-webkit-tests exits. > > Is it still a good idea to avoid setting the existing DISPLAY environment > variable? OK, good point. I guess go w/ setting the exsting DISPLAY for now. > > > You might want to use :99 for compatibility w/ xvfb-run, and also be prepared > to loop to find a free display. > > Done. > > > And, check the error code from the subprocess. > > Which subprocess? The Xvfb process,by calling self._xvfb_process.wait() after > self._xvfb_process.kill()? I meant, make sure that Xvfb started properly, so check to make sure the process hasn't exited. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:155: ['Xvfb', display, '-screen', '0', '1280x800x24', '-ac', '-dpi', '96'], On 2017/04/14 18:25:40, qyearsley wrote: > Not sure if all of these options are necessary. > > - `man Xvfb` says that if -screen is not given, then the default is screen 0, > 1280x1024x8. > - `man Xserver` says that if -dpi is not given, it's determined automatically > (or guessed?), and "-ac" disables host-based access control mechanisms, and > should be used with caution. You need -screen, because you don't want to conflict if a user is running this. You might be able to get away without -ac, not sure. You want to set -dpi to be sure you're getting the dpi you expect. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:172: self._xvfb_process.kill() On 2017/04/14 18:25:40, qyearsley wrote: > Not sure whether it would be prudent to do anything else here; presumably since > Xvfb is a subprocess of python run-webkit-tests, then it will be cleaned up even > if for some reason it fails to be killed here? I'd probably call self._xvfb_process.wait() just to be safe.
lgtm w/ remaining comments addressed.
LGTM too. I wonder if longer term we should be using https://pypi.python.org/pypi/PyVirtualDisplay/0.2.1 -- it supports xvfb, xephyr & xvnc for doing the display and even a bunch of things like screenshotting. Tim 'mithro' Ansell
On 2017/04/18 at 06:39:59, tansell wrote: > LGTM too. > > I wonder if longer term we should be using https://pypi.python.org/pypi/PyVirtualDisplay/0.2.1 -- it supports xvfb, xephyr & xvnc for doing the display and even a bunch of things like screenshotting. Sounds possible, but probably not necessary for now I guess. Thanks for review :-) https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:335: self._port.setup_test_run() On 2017/04/14 at 21:44:08, Dirk Pranke wrote: > On 2017/04/14 18:25:40, qyearsley wrote: > > I moved this line here so that it happens before the system dependency check on > > linux, and so that starting xvfb can be done in Port.setup_test_run, which > > seemed like the most obvious place to do it. But, this might change behavior. > > > > The alternatives would be: > > - Add a separate _start_xvfb_if_necessary and call it here, and keep > > setup_test_run down below. > > - In the linux port, make check_sys_deps responsible for starting xvfb if > > necessary. > > I think moving the order of the calls is fine, but you should move everything before check_sys_deps(), i.e., call that last in the function so that everything else is ready as much as possible. Done. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py (right): https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:151: display = self.host.environ.get('DISPLAY') or ':20' On 2017/04/18 at 02:16:18, Dirk Pranke wrote: > On 2017/04/18 01:02:39, qyearsley wrote: > > On 2017/04/14 at 21:44:08, Dirk Pranke wrote: > > > On 2017/04/14 18:25:40, qyearsley wrote: > > > > This choice of :20 is arbitrary. > > > > > > Don't use the existing DISPLAY value, that'll just confuse things. Also, you > > shouldn't alter the existing environ, just make sure that the new value gets > > propagated to the driver in setup_environ_for_server(). > > > > There's one complication here: a collection of new Port objects is created for > > each Worker, so if the display found (e.g. :99 or :100) during setup is stored > > on the original Port object itself that won't work, since new Ports are created > > for each new Worker. > > > > I think we could store the display found on the Host or somewhere else to ensure > > that is available when creating and setting up Workers, but this might not be as > > simple. > > > > If we alter the run-webkit-tests process environment, though, then that also > > works (all of the subprocesses get the new DISPLAY), and this wouldn't affect > > the value of DISPLAY after run-webkit-tests exits. > > > > Is it still a good idea to avoid setting the existing DISPLAY environment > > variable? > > OK, good point. > > I guess go w/ setting the exsting DISPLAY for now. Alright, will do this for now. Added comment about this as well. > > > > > You might want to use :99 for compatibility w/ xvfb-run, and also be prepared > > to loop to find a free display. > > > > Done. > > > > > And, check the error code from the subprocess. > > > I meant, make sure that Xvfb started properly, so check to make sure the process hasn't exited. Done (by checking whether process.poll() is None). https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:155: ['Xvfb', display, '-screen', '0', '1280x800x24', '-ac', '-dpi', '96'], On 2017/04/18 at 02:16:18, Dirk Pranke wrote: > On 2017/04/14 18:25:40, qyearsley wrote: > > Not sure if all of these options are necessary. > > > > - `man Xvfb` says that if -screen is not given, then the default is screen 0, > > 1280x1024x8. > > - `man Xserver` says that if -dpi is not given, it's determined automatically > > (or guessed?), and "-ac" disables host-based access control mechanisms, and > > should be used with caution. > > You need -screen, because you don't want to conflict if a user is running this. You might be able to get away without -ac, not sure. You want to set -dpi to be sure you're getting the dpi you expect. Alright, will keep these arguments, later we could possibly experiment with removing -ac but it's probably not important. https://codereview.chromium.org/2821543003/diff/40001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/port/linux.py:172: self._xvfb_process.kill() On 2017/04/18 at 02:16:18, Dirk Pranke wrote: > On 2017/04/14 18:25:40, qyearsley wrote: > > Not sure whether it would be prudent to do anything else here; presumably since > > Xvfb is a subprocess of python run-webkit-tests, then it will be cleaned up even > > if for some reason it fails to be killed here? > > I'd probably call self._xvfb_process.wait() just to be safe. Done.
The CQ bit was checked by qyearsley@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, tansell@chromium.org Link to the patchset: https://codereview.chromium.org/2821543003/#ps100001 (title: "Check process after starting, wait after killing, change stderr redirect to DEVNULL")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492538319778860,
"parent_rev": "7275eb16ae8249a51f924ffbd110dcff82c3651b", "commit_rev":
"8e209a99612f0af2a91c940967eacef653fa20b1"}
Message was sent while issue was closed.
Description was changed from ========== In run-webkit-tests, start xvfb if necessary. BUG=711515 ========== to ========== In run-webkit-tests, start xvfb if necessary. BUG=711515 Review-Url: https://codereview.chromium.org/2821543003 Cr-Commit-Position: refs/heads/master@{#465331} Committed: https://chromium.googlesource.com/chromium/src/+/8e209a99612f0af2a91c940967ea... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/8e209a99612f0af2a91c940967ea...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:100001) has been created in https://codereview.chromium.org/2828623002/ by ojan@chromium.org. The reason for reverting is: Looks like it broke WebKit Android (Nexus4). https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20%28N... https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit.... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
