[libcxx-commits] [PATCH] D148324: [libcxx] [test] Prepend to PATH instead of overriding it
Martin Storsjö via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Thu Apr 27 00:00:39 PDT 2023
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
More information about the libcxx-commits
mailing list