I need to be able to run the API tests with additional built system frameworks, like UIKit. We should support passing additional environment variables to use when running run-api-tests. Run-webkit-tests supports this via the --additional-env-var command line option. I propose we also add a --additional-env-var to run-api-tests that behaves the same.
<rdar://problem/54852698>
This should be pretty straight-forward, run-webkit-tests and run-api-tests share a lot of code.
Created attachment 377981 [details] Patch
Comment on attachment 377981 [details] Patch Thank you for taking this on. This is a good first stab. It was not the solution I expected because: 1. Specifying the *DYLD_* variables using --additional-env-var overrides (instead of appends to) them, which is not behavior I expected. I expected them to append to – the same behavior as in run-webkit-tests. FYI, this is an important behavioral difference because without the appending I cannot use my custom build UIKit **together** with a custom build WebKit. 2. There is duplication of code from run-webkit-tests. I was expecting more code sharing.
(In reply to Daniel Bates from comment #4) > Comment on attachment 377981 [details] > Patch > > Thank you for taking this on. This is a good first stab. It was not the > solution I expected because: > > 1. Specifying the *DYLD_* variables using --additional-env-var overrides > (instead of appends to) them, which is not behavior I expected. I expected > them to append to – the same behavior as in run-webkit-tests. FYI, this is > an important behavioral difference because without the appending I cannot > use my custom build UIKit **together** with a custom build WebKit. Good point, hadn't thought about this use case. > 2. There is duplication of code from run-webkit-tests. I was expecting more > code sharing. That's (mostly) more trouble than it's worth. The actual option parsing code in run-webkit-tests is coupled with layout test specific options, so reusing it would involve refactoring our option parser for run-webkit-tests. The environment variable code is split between setup_environ_for_server (which, taking a closer look, we can reuse, even though the name would sort of indicate otherwise) and _setup_environ_for_driver in driver.py. We can't use anything in driver.py, that entire class is extremely specific to layout tests.
Created attachment 377986 [details] Patch
Comment on attachment 377986 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=377986&action=review > Tools/Scripts/webkitpy/port/base.py:275 > + for key in ['DYLD_LIBRARY_PATH', '__XPC_DYLD_LIBRARY_PATH', 'DYLD_FRAMEWORK_PATH', '__XPC_DYLD_FRAMEWORK_PATH']: > + if environment.get(key): > + environment[key] = environment[key] + os.pathsep + build_root_path > + else: > + environment[key] = build_root_path Good use of os.pathsep. Can self._append_value_colon_separated() be used here instead of duplicating its functionality? If not, please explain in the ChangeLog why it cannot be used. For this patch or a future patch append_value_colon_separated() could benefit by writing in terms of os.pathsep.
(In reply to Daniel Bates from comment #7) > Comment on attachment 377986 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=377986&action=review > > > Tools/Scripts/webkitpy/port/base.py:275 > > + for key in ['DYLD_LIBRARY_PATH', '__XPC_DYLD_LIBRARY_PATH', 'DYLD_FRAMEWORK_PATH', '__XPC_DYLD_FRAMEWORK_PATH']: > > + if environment.get(key): > > + environment[key] = environment[key] + os.pathsep + build_root_path > > + else: > > + environment[key] = build_root_path > > Good use of os.pathsep. Can self._append_value_colon_separated() be used > here instead of duplicating its functionality? If not, please explain in the > ChangeLog why it cannot be used. For this patch or a future patch > append_value_colon_separated() could benefit by writing in terms of > os.pathsep. Yes it can, didn't notice that function in base.py, good catch.
Committed r249500: <https://trac.webkit.org/changeset/249500>
Filed: Bug 204400 – [Win][Cygwin][run-api-tests] assert os.pathsep not in value in _append_value_colon_separated
Created attachment 383949 [details] Patch actually landed
Comment on attachment 383949 [details] Patch actually landed View in context: https://bugs.webkit.org/attachment.cgi?id=383949&action=review > Tools/ChangeLog:12 > + (Port._append_value_colon_separated): Use os.pathsep instead of ':'. Why? The function name '_append_value_colon_separated' is misleading.
Comment on attachment 383949 [details] Patch actually landed View in context: https://bugs.webkit.org/attachment.cgi?id=383949&action=review >> Tools/ChangeLog:12 >> + (Port._append_value_colon_separated): Use os.pathsep instead of ':'. > > Why? The function name '_append_value_colon_separated' is misleading. Sorry. I misunderstood. Ignore this comment.