This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Unify and relayer testing
ClosedPublic

Authored by JDevlieghere on May 1 2018, 3:04 PM.

Details

Summary

This patch restructures part of LLDB's testing configuration:

  1. I moved the test dependencies up the chain so every dotest dependency becomes a lit dependency as well. It wouldn't make sense for dotest to have other dependencies when it's being run by lit. Lit on the other hand can still specify extra dependencies.
  2. I replaced as much generator expressions with variables as possible. This is consistent with the rest of LLVM and doesn't break generators that support multiple targets (MSVC, Xcode). This wasn't a problem before, but now we need to expand the dotest arguments in the lit configuration and there's only one test suite even with multiple targets.
  3. I moved lldb-dotest into it's own directory under utils since there's no need anymore for it to located under test/.

Diff Detail

Repository
rL LLVM

Event Timeline

JDevlieghere created this revision.May 1 2018, 3:04 PM
JDevlieghere added inline comments.May 1 2018, 3:07 PM
test/CMakeLists.txt
129 ↗(On Diff #144797)

We'll have to do the same thing here, but the debugserver is more pressing as it's keeping GreenDragon red (http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/)

test/CMakeLists.txt
138 ↗(On Diff #144797)

I am wondering if it's possible to make the logic here simpler. Since we need to handle each of the properties that can contain TARGET_FILE, it is starting to get rather complicated

I think you can actually use LLDB_DOTEST_ARGS_STR for the check-lldb-single target (since it creates a project that should be correctly substituted), so the only other place that could create issues is the lldb-dotest script. Since the tests are run as part of lit now, do we still need the lldb-dotest script?

JDevlieghere added inline comments.May 1 2018, 4:17 PM
test/CMakeLists.txt
138 ↗(On Diff #144797)

I started out that route, but how would that work for check-lldb-single with multiple targets? I'm not worried about lldb-dotest, we can do the same trick as for llvm-lit(see llvm/utils/llvm-lit/CMakeLists.txt).

test/CMakeLists.txt
138 ↗(On Diff #144797)

I cannot speak for other generators that support multiple configurations (though I suspect it is similar), but for Visual Studio, if the path to a tool contained CMAKE_CFG_INTDIR (which is $(Configuration)), when the project was built, $(Configuration) would be replaced with the configuration that is currently being used which is exactly what we want anyway.

Obviously, some additional testing will be required to verify that this is the case always.

JDevlieghere retitled this revision from [lit] Make debugserver available to lit test to [CMake] Unify and relayer testing.
JDevlieghere edited the summary of this revision. (Show Details)
JDevlieghere added reviewers: labath, aprantl.
  • Relayer and unify test dependencies between lit and dotest
  • Extract lldb dotest to utils.
  • Replace generator expressions with configuration variables
JDevlieghere marked 3 inline comments as done.May 2 2018, 4:46 AM

@stella.stamenova Would you mind applying this locally and verify if it works for the MSVC generator? It seems to work for the Xcode generator but it's always possible that I missed something.

labath added a comment.May 2 2018, 5:49 AM

Looks fine to me, but I don't use multi-config generators. I'll let you and Stella work out the details there.

I am going to run the tests after you make the last few changes. I want to make sure the compiler path is no longer being overwritten by the test cmake before I run them.

test/CMakeLists.txt
49 ↗(On Diff #144853)

I think you can get rid of LLDB_EXECUTABLE_PATH_ARGS now and just set these properties in LDLB_TEST_COMMON_ARGS instead.

56 ↗(On Diff #144853)

You actually don't need this anymore. When this was being created per configuration, we needed to overwrite the value of the compiler, but regardless of whether it is custom, it will point to the correct path. You can remove the entire if/else.

138 ↗(On Diff #144797)

Why not change the path where it is set originally? Then you won't need the if/else here.

JDevlieghere marked 4 inline comments as done.
  • Address review feedback Stella

The tests passed, but check-single was not created correctly. I added a comment above.

test/CMakeLists.txt
101 ↗(On Diff #144893)

LLDB_DOTEST_ARGS are passed here, but they were not in the generated project file when I tested this change. I suspect this has to do with setting the PARENT_SCOPE a couple of lines before. Why do we need that?

JDevlieghere added inline comments.May 2 2018, 3:51 PM
test/CMakeLists.txt
101 ↗(On Diff #144893)

We need it in order to have the arguments available in the lit subdirectory. Did it work without the scope? We could eliminate the need for the PARENT_SCOPE by using a global property:

SET_PROPERTY(GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY "${LLDB_DOTEST_ARGS}")

and

GET_PROPERTY(LLDB_DOTEST_ARGS GLOBAL PROPERTY LLDB_DOTEST_ARGS_PROPERTY)

Is there a way to land a partial version of this patch to un-block the green dragon bots and keep iterating on the check-single target separately? Or do other bots depend on check-single working correctly? I'm really uneasy about the fact that we are loosing signal from the bots while they are red. (This is not meant to say that one platform is more important than another, I'm just searching for a way to get all the bots functioning again as quickly as possible, and "no" is an acceptable answer :-).

In its current version, the patch won't *break* the windows build (the previous change broke the build as well as tests), so if it will unblock the bots, Jonas could check it in and then fix check-single in a follow up patch. I think it needs to be explicit if this goes in as-is that it IS breaking check-single though.

If that's okay with you I would suggest landing this as is to unblock the bots, and we'll keep working on fixing the check-single target ASAP, too.

If by as-is you mean with a big comment in the config file before the creation of check-single that it is broken, then yes, let's get the bots unblocked and then fix the issue.

If by as-is you mean with a big comment in the config file before the creation of check-single that it is broken, then yes, let's get the bots unblocked and then fix the issue.

Sounds good. Thanks!

I didn't realize check-lldb-single was also broken for ninja/make. Using the property indeed fixed it, so we can land this without the caveat.

I remember hearing some people use the check-lldb-single target, but it had
to do with running the tests against some remote devices/stubs that did not
handle multiple connections very well.

However, the way it seems to me, with the transition to lit, this target
will have to go away sooner or later (it still invokes dotest directly). If
there is a substantial effort involved in making it work, then maybe the
time to drop it is now. It will still be possible to run the tests in this
mode by invoking dotest directly. (and if this is something that we really
need to support in the future, we should identify who these people are, and
what requirements they have).

update diff pro forma

stella.stamenova accepted this revision.May 3 2018, 9:52 AM

I still need to verify this works with VS on Windows, but feel free to check in to unblock the bots

This revision is now accepted and ready to land.May 3 2018, 9:52 AM
This revision was automatically updated to reflect the committed changes.
lldb/trunk/test/CMakeLists.txt