189702 2018-09-18 10:28:05 -0700 webkitpy: Clobbering and building occurs multiple times for iOS Simulator ports 2018-09-27 08:46:29 -0700 1 1 1 Unclassified WebKit Tools / Tests WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=189623 InRadar P2 Normal --- 1 jbedard jbedard aakash_jain commit-queue dbates ews-watchlist glenn lforschler ryanhaddad webkit-bug-importer oldest_to_newest 1460637 0 jbedard 2018-09-18 10:28:05 -0700 Currently, we clobber old results and and build in the _set_up_run function, which is run multiple times per run for the iOS simulator. 1460638 1 jbedard 2018-09-18 10:28:16 -0700 <rdar://problem/44541704> 1460639 2 350031 jbedard 2018-09-18 10:29:34 -0700 Created attachment 350031 Patch 1460647 3 jbedard 2018-09-18 10:43:33 -0700 Waiting for EWS since my local testing was limited. 1460691 4 350031 commit-queue 2018-09-18 13:10:41 -0700 Comment on attachment 350031 Patch Clearing flags on attachment: 350031 Committed r236151: <https://trac.webkit.org/changeset/236151> 1460692 5 commit-queue 2018-09-18 13:10:43 -0700 All reviewed patches have been landed. Closing bug. 1460693 6 webkit-bug-importer 2018-09-18 13:11:40 -0700 <rdar://problem/44572385> 1462463 7 350031 dbates 2018-09-22 20:06:06 -0700 Comment on attachment 350031 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350031&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-167 > - self._printer.write_update("Checking build ...") > - if not self._port.check_build(): > - _log.error("Build check failed") > - return False > - Does run-webkit-tests on Mac run the tests if you perform a clean build of the WebKit stack and not the tools? This removal does not seem correct or does not seem correct for Mac. What guarantees that LayoutTestHelper was built before we try to launch it below? Even if we are guaranteed it to be built in advance it seems unintuitive that we moved this logic from this function, used to setup a run, to the function that is responsible for running the test. Maybe we have to do this for iOS, but then it may be better to make iOS the exception that it is than to modify all ports to fit iOS's weird model of the world. 1462464 8 350031 dbates 2018-09-22 20:08:56 -0700 Comment on attachment 350031 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350031&action=review > Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:240 > + if self._options.clobber_old_results: > + self._clobber_old_results() > + > + # Create the output directory if it doesn't already exist. > + self._port.host.filesystem.maybe_make_directory(self._results_directory) Similarly this seems unintuitive. If this is necessary to fix iOS then make iOS the exception (and have dedicated conditional branches or a dedicated code path) and not make a mess of the code for all the other ports. 1463966 9 350031 jbedard 2018-09-27 08:46:29 -0700 Comment on attachment 350031 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350031&action=review >> Tools/Scripts/webkitpy/layout_tests/controllers/manager.py:-167 >> - > > Does run-webkit-tests on Mac run the tests if you perform a clean build of the WebKit stack and not the tools? This removal does not seem correct or does not seem correct for Mac. What guarantees that LayoutTestHelper was built before we try to launch it below? Even if we are guaranteed it to be built in advance it seems unintuitive that we moved this logic from this function, used to setup a run, to the function that is responsible for running the test. Maybe we have to do this for iOS, but then it may be better to make iOS the exception that it is than to modify all ports to fit iOS's weird model of the world. This function starts with an _. That indicates that it should be a private function and other classes shouldn't be calling it directly. This code wasn't removed, it was MOVED. We still run it. Just not multiple times. All this change does is move this code so we don't do this multiple times during a single instantiation of run-webkit-tests for iOS Simulator. Since we've already established long ago that a single instantiation of run-webkit-tests may contain multiple sequential runs, it doesn't make sense for this code to be in _set_up_run(...), we only need it to be run once per instantiation of run-webkit-test. It's also not just an iOS problem anymore because watchOS uses exactly the same model as iOS. 350031 2018-09-18 10:29:34 -0700 2018-09-18 13:10:41 -0700 Patch bug-189702-20180918102933.patch text/plain 2957 jbedard SW5kZXg6IFRvb2xzL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBUb29scy9DaGFuZ2VMb2cJKHJl dmlzaW9uIDIzNjE0NCkKKysrIFRvb2xzL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwz ICsxLDE2IEBACisyMDE4LTA5LTE4ICBKb25hdGhhbiBCZWRhcmQgIDxqYmVkYXJkQGFwcGxlLmNv bT4KKworICAgICAgICB3ZWJraXRweTogQ2xvYmJlcmluZyBhbmQgYnVpbGRpbmcgb2NjdXJzIG11 bHRpcGxlIHRpbWVzIGZvciBpT1MgU2ltdWxhdG9yIHBvcnRzCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xODk3MDIKKyAgICAgICAgPHJkYXI6Ly9wcm9i bGVtLzQ0NTQxNzA0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisg ICAgICAgICogU2NyaXB0cy93ZWJraXRweS9sYXlvdXRfdGVzdHMvY29udHJvbGxlcnMvbWFuYWdl ci5weToKKyAgICAgICAgKE1hbmFnZXIuX3NldF91cF9ydW4pOiBNb3ZlIGJ1aWxkIGNoZWNrIGFu ZCBjbG9iYmVyaW5nIHRvIHJ1biwgc2luY2Ugc2V0IHVwIGlzCisgICAgICAgIHJ1biBtdWx0aXBs ZSB0aW1lcyBmb3IgaU9TIHNpbXVsYXRvci4KKyAgICAgICAgKE1hbmFnZXIucnVuKToKKwogMjAx OC0wOS0xOCAgQ2hyaXMgRHVtZXogIDxjZHVtZXpAYXBwbGUuY29tPgogCiAgICAgICAgICJEaWRG aXJzdFZpc3VhbGx5Tm9uRW1wdHlMYXlvdXQiIGNhbGxiYWNrIGRvZXMgbm90IGdldCBjYWxsZWQg d2hlbiByZXN0b3JpbmcgYSBwYWdlIGZyb20gUGFnZUNhY2hlCkluZGV4OiBUb29scy9TY3JpcHRz L3dlYmtpdHB5L2xheW91dF90ZXN0cy9jb250cm9sbGVycy9tYW5hZ2VyLnB5Cj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0X3Rlc3RzL2NvbnRyb2xsZXJzL21hbmFn ZXIucHkJKHJldmlzaW9uIDIzNDgxMikKKysrIFRvb2xzL1NjcmlwdHMvd2Via2l0cHkvbGF5b3V0 X3Rlc3RzL2NvbnRyb2xsZXJzL21hbmFnZXIucHkJKHdvcmtpbmcgY29weSkKQEAgLTE2MCwxMSAr MTYwLDYgQEAgY2xhc3MgTWFuYWdlcihvYmplY3QpOgogICAgICAgICBzZWxmLl9vcHRpb25zLmNo aWxkX3Byb2Nlc3NlcyA9IHdvcmtlcl9jb3VudAogCiAgICAgZGVmIF9zZXRfdXBfcnVuKHNlbGYs IHRlc3RfbmFtZXMsIGRldmljZV9jbGFzcz1Ob25lKToKLSAgICAgICAgc2VsZi5fcHJpbnRlci53 cml0ZV91cGRhdGUoIkNoZWNraW5nIGJ1aWxkIC4uLiIpCi0gICAgICAgIGlmIG5vdCBzZWxmLl9w b3J0LmNoZWNrX2J1aWxkKCk6Ci0gICAgICAgICAgICBfbG9nLmVycm9yKCJCdWlsZCBjaGVjayBm YWlsZWQiKQotICAgICAgICAgICAgcmV0dXJuIEZhbHNlCi0KICAgICAgICAgc2VsZi5fb3B0aW9u cy5kZXZpY2VfY2xhc3MgPSBkZXZpY2VfY2xhc3MKIAogICAgICAgICAjIFRoaXMgbXVzdCBiZSBz dGFydGVkIGJlZm9yZSB3ZSBjaGVjayB0aGUgc3lzdGVtIGRlcGVuZGVuY2llcywKQEAgLTE4Mywx MiArMTc4LDYgQEAgY2xhc3MgTWFuYWdlcihvYmplY3QpOgogICAgICAgICAgICAgICAgIHNlbGYu X3BvcnQuc3RvcF9oZWxwZXIoKQogICAgICAgICAgICAgICAgIHJldHVybiBGYWxzZQogCi0gICAg ICAgIGlmIHNlbGYuX29wdGlvbnMuY2xvYmJlcl9vbGRfcmVzdWx0czoKLSAgICAgICAgICAgIHNl bGYuX2Nsb2JiZXJfb2xkX3Jlc3VsdHMoKQotCi0gICAgICAgICMgQ3JlYXRlIHRoZSBvdXRwdXQg ZGlyZWN0b3J5IGlmIGl0IGRvZXNuJ3QgYWxyZWFkeSBleGlzdC4KLSAgICAgICAgc2VsZi5fcG9y dC5ob3N0LmZpbGVzeXN0ZW0ubWF5YmVfbWFrZV9kaXJlY3Rvcnkoc2VsZi5fcmVzdWx0c19kaXJl Y3RvcnkpCi0KICAgICAgICAgc2VsZi5fcG9ydC5zZXR1cF90ZXN0X3J1bihzZWxmLl9vcHRpb25z LmRldmljZV9jbGFzcykKICAgICAgICAgcmV0dXJuIFRydWUKIApAQCAtMjM5LDYgKzIyOCwxNyBA QCBjbGFzcyBNYW5hZ2VyKG9iamVjdCk6CiAgICAgICAgIHNlbGYuX3J1bm5lciA9IExheW91dFRl c3RSdW5uZXIoc2VsZi5fb3B0aW9ucywgc2VsZi5fcG9ydCwgc2VsZi5fcHJpbnRlciwgc2VsZi5f cmVzdWx0c19kaXJlY3RvcnksIHNlbGYuX3Rlc3RfaXNfc2xvdywKICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICBuZWVkc19odHRwPW5lZWRzX2h0dHAsIG5lZWRzX3dlYl9w bGF0Zm9ybV90ZXN0X3NlcnZlcj1uZWVkc193ZWJfcGxhdGZvcm1fdGVzdF9zZXJ2ZXIsIG5lZWRz X3dlYnNvY2tldHM9bmVlZHNfd2Vic29ja2V0cykKIAorICAgICAgICBzZWxmLl9wcmludGVyLndy aXRlX3VwZGF0ZSgiQ2hlY2tpbmcgYnVpbGQgLi4uIikKKyAgICAgICAgaWYgbm90IHNlbGYuX3Bv cnQuY2hlY2tfYnVpbGQoKToKKyAgICAgICAgICAgIF9sb2cuZXJyb3IoIkJ1aWxkIGNoZWNrIGZh aWxlZCIpCisgICAgICAgICAgICByZXR1cm4gdGVzdF9ydW5fcmVzdWx0cy5SdW5EZXRhaWxzKGV4 aXRfY29kZT0tMSkKKworICAgICAgICBpZiBzZWxmLl9vcHRpb25zLmNsb2JiZXJfb2xkX3Jlc3Vs dHM6CisgICAgICAgICAgICBzZWxmLl9jbG9iYmVyX29sZF9yZXN1bHRzKCkKKworICAgICAgICAj IENyZWF0ZSB0aGUgb3V0cHV0IGRpcmVjdG9yeSBpZiBpdCBkb2Vzbid0IGFscmVhZHkgZXhpc3Qu CisgICAgICAgIHNlbGYuX3BvcnQuaG9zdC5maWxlc3lzdGVtLm1heWJlX21ha2VfZGlyZWN0b3J5 KHNlbGYuX3Jlc3VsdHNfZGlyZWN0b3J5KQorCiAgICAgICAgIGlmIGRlZmF1bHRfZGV2aWNlX3Rl c3RzOgogICAgICAgICAgICAgX2xvZy5pbmZvKCcnKQogICAgICAgICAgICAgX2xvZy5pbmZvKCJS dW5uaW5nICVzIiwgcGx1cmFsaXplKGxlbih0ZXN0c190b19ydW4pLCAidGVzdCIpKQo=