Page MenuHomePhabricator

[debuginfo-tests] Fix environment variable used to specify LLDB
ClosedPublic

Authored by jhenderson on May 6 2021, 2:07 AM.

Details

Summary

Currently, if the user specifies the environment variable 'CLANG', tests will attempt to use the value as a path to the clang executable. Previously, lldb could also be specified via the CLANG environment variable, but this was almost certainly a bug, because that meant both clang and lldb would have the same path. This patch changes the environment variable for lldb to 'LLDB'.

Diff Detail

Event Timeline

jhenderson created this revision.May 6 2021, 2:07 AM
jhenderson requested review of this revision.May 6 2021, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 6 2021, 2:07 AM
jhenderson edited the summary of this revision. (Show Details)May 6 2021, 2:33 AM
jhenderson added a reviewer: thopre.
thopre added a comment.May 6 2021, 2:34 AM

Should we add a test for that?

Should we add a test for that?

We could add a lit test for the additional pass-through variables, if you think there's value in it (I don't think the other variables in the list are tested however), but I don't think we can test the lit.cfg.py change. To test that, we'd need to have a lit tests within the debuginfo-tests directory which spawns lit itself, with certain variables specified, and show that the right tools are used. I don't think this is practical. I've manually confirmed that the expected tools are used when the variables are set.

thopre accepted this revision.May 6 2021, 3:08 AM

Should we add a test for that?

We could add a lit test for the additional pass-through variables, if you think there's value in it (I don't think the other variables in the list are tested however), but I don't think we can test the lit.cfg.py change. To test that, we'd need to have a lit tests within the debuginfo-tests directory which spawns lit itself, with certain variables specified, and show that the right tools are used. I don't think this is practical. I've manually confirmed that the expected tools are used when the variables are set.

That's the overal feature I would like tested so more the lit.cfg.py side of things. If you think this is difficult to test for little value and since you have tested manually I'd say LGTM.

This revision is now accepted and ready to land.May 6 2021, 3:08 AM

I'm going to hold off landing this for now until the discussion on the mailing list (https://lists.llvm.org/pipermail/llvm-dev/2021-May/150428.html) has reached a conclusion.

jhenderson planned changes to this revision.May 17 2021, 2:00 AM

The mailing list discussion seems to suggest not adding these new variables. There are obviously no use-cases for the majority of them. LLDB is one exception, where it has been requested (see https://reviews.llvm.org/D95339#2638619). I'll update the patch accordingly.

jhenderson retitled this revision from [debuginfo-tests] Allow use of environment variable to specify tools to [debuginfo-tests] Fix environment variable used to specify LLDB.
jhenderson edited the summary of this revision. (Show Details)

Reduce scope of patch to just LLDB fix.

This revision is now accepted and ready to land.May 17 2021, 3:04 AM
jhenderson requested review of this revision.May 17 2021, 3:04 AM

@thopre, I've reduce the scope of this patch to just the LLDB fix, as per the mailing list discussion. Please confirm that you are happy still!

thopre accepted this revision.May 17 2021, 3:11 AM

Still LGTM

This revision is now accepted and ready to land.May 17 2021, 3:11 AM
teemperor accepted this revision.May 17 2021, 3:14 AM
teemperor added a subscriber: teemperor.

LGTM. Please watch the Green Dragon bot for LLDB after landing (the bot runs the debuginfo-tests + LLDB tests). I noticed some time ago that part of the debuginfo-tests there were partly running the system LLDB (and it wasn't clear to me if that was actually on purpose). So this patch might bring up some previously hidden failures.

LGTM. Please watch the Green Dragon bot for LLDB after landing (the bot runs the debuginfo-tests + LLDB tests). I noticed some time ago that part of the debuginfo-tests there were partly running the system LLDB (and it wasn't clear to me if that was actually on purpose). So this patch might bring up some previously hidden failures.

Thanks for the heads-up!

This revision was landed with ongoing or failed builds.May 17 2021, 4:50 AM
This revision was automatically updated to reflect the committed changes.