Page MenuHomePhabricator

[test] Use a different module cache for Shell and API tests.
ClosedPublic

Authored by JDevlieghere on Oct 9 2019, 6:14 PM.

Details

Summary

Before the test reorganization, everything was part of a single test suite with a single module cache. Now that things are properly separated this is no longer the case. Only the shell tests inherited the logic to properly configure and wipe the module caches. This patch adds that logic back for the API tests. While doing so, I noticed that we were configuring a Clang module cache in CMake, but weren't actually using it from dotest.py. I included a fix for that in this patch as well.

Diff Detail

Event Timeline

JDevlieghere created this revision.Oct 9 2019, 6:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptOct 9 2019, 6:14 PM
JDevlieghere marked an inline comment as done.Oct 9 2019, 6:18 PM
JDevlieghere added inline comments.
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
316

We should have a fallback here that defaults to $(BUILDDIR)/module-cache if the variable is not set in the environment.

labath added inline comments.Oct 10 2019, 1:05 AM
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
316

Is that wise, given D68731 tries to move away from autodetecting stuff in make?

What I think would be more useful is to pass this variable via the command like instead of the environment. That way it will be visible in the make invocation that dotest prints and the exact make invocation can be reproduced by copy-pasting. It shouldn't be even hard to do that -- you'd just need builder_base.py to fetch this from the configuration object and inject it into the make arguments.

friss added inline comments.Oct 10 2019, 8:13 AM
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
316

+1

We shouldn't pass anything in the environment, it makes reproducing build failures a pain. (other variables I think are in the same bucket: DSYMUTIL and SDKROOT)

aprantl accepted this revision.Oct 10 2019, 8:33 AM

LGTM, apart from the outstanding comments.

This revision is now accepted and ready to land.Oct 10 2019, 8:33 AM
JDevlieghere marked an inline comment as done.Oct 10 2019, 9:45 AM
JDevlieghere added inline comments.
lldb/packages/Python/lldbsuite/test/make/Makefile.rules
316
  • Rebased on top of D68812.
  • Pass the module cache on the command line.
This revision was automatically updated to reflect the committed changes.
labath added inline comments.Oct 11 2019, 12:28 AM
lldb/packages/Python/lldbsuite/test/dotest.py
440

So, what's the reason for passing this around through environment? Couldn't the "builder" module just fetch that from the configuration object? Using the environment to communicate between two python functions is just wrong...