Bug 201311 – run-api-tests: Add support for specifying additional environment variables

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Daniel Bates]

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.

[Radar WebKit Bug Importer]

<rdar://problem/54852698>

[Jonathan Bedard]

This should be pretty straight-forward, run-webkit-tests and run-api-tests share a lot of code.

[Jonathan Bedard]

Created attachment 377981 [details]
Patch

[Daniel Bates]

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.

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Created attachment 377986 [details]
Patch

[Daniel Bates]

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.

[Jonathan Bedard]

(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.

[Jonathan Bedard]

Committed r249500: <https://trac.webkit.org/changeset/249500>

[Fujii Hironori]

Filed: Bug 204400 – [Win][Cygwin][run-api-tests] assert os.pathsep not in value in _append_value_colon_separated

[Fujii Hironori]

Created attachment 383949 [details]
Patch actually landed

[Fujii Hironori]

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.

[Fujii Hironori]

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.