llvm dev - May 2021 - Using clang/lld from the PATH when testing

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210506/6ee63c7f/attachment.html>

On 2021-05-06, James Henderson via llvm-dev wrote:
>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?

If every executable needs an environment variable, I worry that there may be
too much customization.
>Thanks,
>
>James
>_______________________________________________
>LLVM Developers mailing list
>llvm-dev at lists.llvm.org
>https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

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.

On Thu, May 6, 2021 at 3:49 AM James Henderson via llvm-dev <
llvm-dev at lists.llvm.org> wrote:
> 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210514/26ae2855/attachment.html>