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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne accepted this revision.
ldionne added a comment.
This revision is now accepted and ready to land.

In D148324#4301227 <https://reviews.llvm.org/D148324#4301227>, @mstorsjo wrote:

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

Ah, now I think this all makes sense.

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

Yes, that's what I meant.

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

Hmm, yeah, I guess that's an annoying limitation. I think your patch as-is is reasonable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148324