Bug 198316 - run-benchmark should report an error if the argument to --build-directory is bogus
Summary: run-benchmark should report an error if the argument to --build-directory is ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: dewei_zhu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-28 17:22 PDT by dewei_zhu
Modified: 2022-02-09 10:14 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.46 KB, patch)
2019-05-28 17:26 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (4.52 KB, patch)
2019-05-30 16:25 PDT, dewei_zhu
no flags Details | Formatted Diff | Diff
Patch (4.91 KB, patch)
2019-05-30 19:25 PDT, dewei_zhu
rniwa: review+
ews-watchlist: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews211 for win-future (13.53 MB, application/zip)
2019-05-30 23:20 PDT, EWS Watchlist
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description dewei_zhu 2019-05-28 17:22:25 PDT
run-benchmark should report an error if the argument to --build-directory is bogus
Comment 1 dewei_zhu 2019-05-28 17:26:07 PDT
Created attachment 370810 [details]
Patch
Comment 2 dewei_zhu 2019-05-28 17:26:43 PDT
rdar://27875380
Comment 3 Stephanie Lewis 2019-05-28 17:28:09 PDT
Comment on attachment 370810 [details]
Patch

There are some engineers who work on and build webkit but don't necessarily build the whole stack
Comment 4 Stephanie Lewis 2019-05-28 17:31:15 PDT
Comment on attachment 370810 [details]
Patch

If you want to not support that case I'm ok with that. Secondarily what you could do is check that one of the major frameworks (webkit.framework, javascriptcore.framework ... ) or Safari.app exist.  And if throw an exception if none of those exist.  That should cover the case where people make typos
Comment 5 Ryosuke Niwa 2019-05-28 19:12:11 PDT
Comment on attachment 370810 [details]
Patch

Can we also check the open file of Safari to see if we've successfully loaded the framework or not?
Comment 6 dewei_zhu 2019-05-30 16:25:12 PDT
Created attachment 370991 [details]
Patch
Comment 7 Ryosuke Niwa 2019-05-30 16:47:20 PDT
Comment on attachment 370991 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:43
> +                raise Exception('"{}" does not look like a build directory'.format(browser_build_path))

I think a better error message is to just say there is no framework in the specified directory.
Otherwise, people who see this error message may get confused as to why their "valid" build directory is not accepted
if they're testing something which doesn't involve building .frameworks.

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:61
> +            _log.info('Checking either Safari.app or framwork is used from "{}".'.format(browser_build_path))

It doesn't check that at all! It simply checks that there is some open file included from the build directory...

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:63
> +            assert browser_build_absolute_path in output, 'Specified build directory "{}" but none of its content is used'.format(browser_build_path)

We should probably check that join('.framework', browser_build_absolute_path) or join('.app', browser_build_absolute_path) exist instead.
Otherwise it could be loading some setting files, caches, etc...
Comment 8 dewei_zhu 2019-05-30 19:25:02 PDT
Created attachment 371020 [details]
Patch
Comment 9 EWS Watchlist 2019-05-30 23:20:51 PDT
Comment on attachment 371020 [details]
Patch

Attachment 371020 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12336293

New failing tests:
http/tests/misc/repeat-open-cancel.html
http/tests/css/filters-on-iframes-transform.html
storage/indexeddb/index-cursor.html
Comment 10 EWS Watchlist 2019-05-30 23:20:53 PDT
Created attachment 371038 [details]
Archive of layout-test-results from ews211 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews211  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 11 Ryosuke Niwa 2019-05-30 23:49:52 PDT
Comment on attachment 371020 [details]
Patch

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

> Tools/Scripts/webkitpy/benchmark_runner/browser_driver/osx_safari_driver.py:41
> +                env = {'DYLD_FRAMEWORK_PATH': browser_build_absolute_path, 'DYLD_LIBRARY_PATH': browser_build_absolute_path, '__XPC_DYLD_FRAMEWORK_PATH': browser_build_absolute_path, '__XPC_DYLD_LIBRARY_PATH': browser_build_absolute_path}

Can we split this into multiple lines? It's really long now.