[libcxx-commits] [PATCH] D148324: [libcxx] [test] Prepend to PATH instead of overriding it

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added a comment.

In D148324#4300520 <https://reviews.llvm.org/D148324#4300520>, @ldionne wrote:

> I'm not sure I follow.
>
>> This had the unfortunate side effect of making other tools available in PATH during the runtime of the tests
>
> Shouldn't it be the other way around, i.e. shouldn't it make *only* the things in `%{lib}` available, which is less than if you prepend `%{lib}` to the rest of the `PATH`?

Sorry, yes, that sentence lacked a negation. I'll try to rephrase it in a simpler manner.

> This leads me to wonder whether we instead might want to acknowledge that this is only used to pass a dyld-library-path-kind-of-variable instead of trying to make it general in purpose. After all, you do use `os.pathsep` in there so it's pretty clear that this can only be used to set some sort of `$PATH`-like variable.

So you mean we'd add an option like `--library-path` to `run.py`, which then sets it in the platform specific way (i.e. either setting or prepending to an environment variable)? I guess that'd work too (then we should use that in essentially all test configs).

There's one aspect with that setup which is slightly less flexible though; it works great when `%{executor}` is `run.py` which executes things locally (so we can ask Python all about the executing system), but when the executor is set to `ssh.py`, that script doesn't really know whether we should be setting e.g. `LD_LIBRARY_PATH` or `DYLD_LIBRARY_PATH` - while the test config knows more about the intended setup (where it currently spells out the expected env variable name).

Remote testing in general is mostly useful if using a static libc++ though, so that combination probably isn't something that must work flawlessly (since it probably can't, in general), but we should at least make an effort to not fall over flat if faced with those arguments.

Even with this version of the patch, I now realized that this patch breaks one of my cross-testing setups; I'll amend it to handle it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148324