Hi all,
Summary: can we stop allowing clang/lld to be picked up from the PATH environment when running testing?
Details:
As part of my work on a cross-project testsuite (see https://reviews.llvm.org/D95339 and other related patches), I noticed that where lit has been configured using use_clang() or use_lld() the respective tools will be selected from the system PATH, if they aren’t found in the build directory. If users are running check-* to run the relevant tests, this isn’t an issue, as the tools will be built (and therefore used in preference).
I personally, and I’m sure many others, routinely don’t use these check-* targets, preferring to manually build and run the subset of tests that have the potential to be impacted by my change. Potentially this leaves room for human error (e.g. a misconfigured build bot or simply a user error when checking things) to not build clang (or lld) in a clean environment, and as such, the “wrong” tool might be used, causing spurious tests failures (or worse, spurious passes).
An environment variable (‘CLANG’) can be specified to specify the clang executable to use, in preference to a built one or one on the PATH. I plan to make a similar change for LLD soon too. As such, do we need the “use the PATH version” behaviour anymore? Dropping it will allow simplifying code and reduce room for human error.
Thanks,
James
Hi all,
Summary: can we stop allowing clang/lld to be picked up from the PATH
environment when running testing?
Details:
As part of my work on a cross-project testsuite (see
⚙ D95339 [RFC][test] Adapt debug-info lit framework for more general purposes - part 1 and other related patches), I noticed that
where lit has been configured using `use_clang()` or `use_lld()` the
respective tools will be selected from the system PATH, if they aren't
found in the build directory. If users are running check-* to run the
relevant tests, this isn't an issue, as the tools will be built (and
therefore used in preference).
I personally, and I'm sure many others, routinely don't use these check-*
targets, preferring to manually build and run the subset of tests that have
the potential to be impacted by my change. Potentially this leaves room for
human error (e.g. a misconfigured build bot or simply a user error when
checking things) to not build clang (or lld) in a clean environment, and as
such, the "wrong" tool might be used, causing spurious tests failures (or
worse, spurious passes).
An environment variable ('CLANG') can be specified to specify the clang
executable to use, in preference to a built one or one on the PATH. I plan
to make a similar change for LLD soon too. As such, do we need the "use the
PATH version" behaviour anymore? Dropping it will allow simplifying code
and reduce room for human error.
Is the plan to allow running the testsuite with Clang/LLD built from a
different commit?
If every executable needs an environment variable, I worry that there may be
too much customization.
Hi all,
Summary: can we stop allowing clang/lld to be picked up from the PATH
environment when running testing?
Details:
As part of my work on a cross-project testsuite (see
https://reviews.llvm.org/D95339 and other related patches), I noticed that
where lit has been configured using use_clang() or use_lld() the
respective tools will be selected from the system PATH, if they aren’t
found in the build directory. If users are running check-* to run the
relevant tests, this isn’t an issue, as the tools will be built (and
therefore used in preference).
I personally, and I’m sure many others, routinely don’t use these check-*
targets, preferring to manually build and run the subset of tests that have
the potential to be impacted by my change. Potentially this leaves room for
human error (e.g. a misconfigured build bot or simply a user error when
checking things) to not build clang (or lld) in a clean environment, and as
such, the “wrong” tool might be used, causing spurious tests failures (or
worse, spurious passes).
An environment variable (‘CLANG’) can be specified to specify the clang
executable to use, in preference to a built one or one on the PATH. I plan
to make a similar change for LLD soon too. As such, do we need the “use the
PATH version” behaviour anymore? Dropping it will allow simplifying code
and reduce room for human error.
Is the plan to allow running the testsuite with Clang/LLD built from a
different commit?
In the case of the debuginfo-tests, a request was made in https://reviews.llvm.org/D95339 that an installed LLDB could be used for some basic backwards compatibility testing. The code path that finds LLDB is the same one as used for the CLANG variable discussed in my email, so the same mechanism should be used for them (and LLD too, which uses the same mechanism). I don’t have any specific use-case, beyond that, but it would seem silly to have one customisation point for one of these tools, but not for the others in the set.
If every executable needs an environment variable, I worry that there may be
too much customization.
The only tools impacted by this are clang (including clang++ and clang-cl), LLDB (in the debuginfo-tests only), and LLD (specifically its 4 variants ld.lld/ld64.lld/lld-link/wasm-ld). These are the only places that call use_llvm_tool currently (either via use_clang/use_lld or directly). The existing mechanism for this relies on the variable pointing directly at the executable. As such, we need one variable per executable (a maximum of 8 possible variables should a user want to specify all of them). I could obviously change this mechanism, to specify a directory, but this may not always be useful, if a user has versions of a tool without the default name (e.g. with release number suffixes etc).
Note that the only way to use the PATH versions is if someone hasn’t built the executables in question, as the build directory version is picked above the PATH version anyway.
Is there any more feedback on this from anybody? The conclusion of it is blocking progress on a patch series.
This sounds more accidental than intentional - if it only happens if you haven’t built the specified binary in your build tree & you’re running the tests for that specified binary. That seems super niche and someone could, at worst, copy some arbitrary binary into their build tree to test it, rather than relying on this fallback.
I’d say remove it & probably don’t bother adding the variables, personally. If someone comes back with a use case (they should get a pretty hard failure - since previously/currently they’d get a fallback to their intended binary and in the future/proposal they’d get a “cannot find executable” - don’t suppose there’s some way to make that failure a bit more specific without a ton of work? They can bisect back to the revision that caused the change I guess) - can add the variables to support it/discuss what it should look like then.
Thanks, I tend to agree with you.
Regarding the variables - the functionality already exists for clang. It seems like removing it would be counter-productive, especially as it mostly relies on common code I need. For the debuginfo-tests specifically, Adrian requested an option to test against an LLDB installed on the system (see https://reviews.llvm.org/D95339#2638619), which the environment variable would provide (in fact, it already exists, but is currently broken, as mentioned in the patch). Fixing the LLDB variable would only require changing one string (from “CLANG” to “LLDB” or equivalent), so is trivial to add. That leaves clang++/clang-cl in a slightly weird place, and LLD as also looking a bit different, but I guess we can add support for those when requested. I’m happy to proceed on that basis, unless there are any objections.
I’ve uploaded https://reviews.llvm.org/D102630 to go ahead with disabling PATH usage for all but one pair of cases that makes use of use_clang/use_lld/use_llvm_tool.