[libcxx-commits] [PATCH] D97452: [libcxx] [test] Pass some windows environment variables through to test processes

Tue Mar 2 09:56:21 PST 2021

mstorsjo added a comment.

In D97452#2597696 <https://reviews.llvm.org/D97452#2597696>, @ldionne wrote:

> In D97452#2587362 <https://reviews.llvm.org/D97452#2587362>, @mstorsjo wrote:
>
>> In D97452#2587321 <https://reviews.llvm.org/D97452#2587321>, @curdeius wrote:
>>
>>> LGTM. But I believe that the long term solution is to run all the tests in the internal lit shell.
>>
>> Yes, but I believe that aspect is orthogonal to this script. Currently the lit test driver runs the test script (which consists of two lines, one "clang++" to compile the test and one which invokes the run.py script with python to execute the built test executable) with bash, which maybe could/should be changed to use the lit internal shell. That still keeps the run.py wrapper script for all the purposes it serves right now (codesign, setting up other test specific env variables etc).
>
> Yes, that's correct.
>
> I think it's pretty important to keep `run.py` and the other executors free of logic. Instead, we could string those environment variables into `self.exec_env` in `config.py`. They would then be passed to the executor using the `--env` argument, which was meant for that. WDYT?

Sure - maybe.

However, I see it as a bit of different concern about passing specific options requested from config.py, vs allowing some vars through transparently.

Consider cross testing (I do that, but that's further away in the upstreaming pipeline) - I don't want to pick up env vars blindly from the (unix) build env and pass them to the remote runner (windows), I just want the logic that creates a clean env to let some specifics through.

Those setups use a variant of the existing ssh.py runner (which don't clear the env, only add vars to it), while run.py (for local execution) is the one that entirely wipe the env before adding the env vars that are requested.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97452/new/

https://reviews.llvm.org/D97452