This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Prepend to PATH instead of overriding it
ClosedPublic

Authored by mstorsjo on Apr 14 2023, 4:04 AM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project
Restricted Project
Restricted Project
Commits
rGba3bddb6f473: [libcxx] [test] Prepend to PATH instead of overriding it
Summary

On Windows, the PATH env variable is used for locating dynamically
linked librarys, akin to LD_LIBRARY_PATH on Linux.

The tests that run with a dynamically linked libc++ used "--env
PATH=%{lib}" in the test config. This had the unfortunate side effect
of making other tools available in PATH during the runtime of the tests;
in particular, it caused the "executor-has-no-bash" flag to be set for
all those Windows test configs (with the clang-cl static config being
the only one lacking it).

Thus, this increases the number of tests actually included in the
clang-cl dll and all mingw test configs by 9 tests.

The clang-cl static test configuration has been executing those tests
since the "--env PATH=%{lib}" was removed from that test config in
e78223e79efc886ef6f0ea5413deab3737d6d63b. (For mingw we haven't had a
need to split the test config between shared and static, which means
that the mingw static test config previously ran with --env PATH
needlessly.)

This increases the test coverage for patches like D146398 which
can't be executed in the executor-has-no-bash configs.

Diff Detail

Unit TestsFailed

Event Timeline

mstorsjo created this revision.Apr 14 2023, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 4:04 AM
Herald added a subscriber: arichardson. · View Herald Transcript
mstorsjo requested review of this revision.Apr 14 2023, 4:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2023, 4:04 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

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

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.

mstorsjo updated this revision to Diff 517466.Apr 27 2023, 12:17 AM

Updated to make ssh.py handle the new option in a best-effort way (not flat out refusing to run when the option is set).

Also use the new option in libcxxabi and libunwind test configs.

Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 27 2023, 12:17 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne accepted this revision.Apr 27 2023, 8:12 AM

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.

This revision is now accepted and ready to land.Apr 27 2023, 8:12 AM
This revision was landed with ongoing or failed builds.Apr 27 2023, 9:27 AM
This revision was automatically updated to reflect the committed changes.