This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Use explicit module cache path in tests
AbandonedPublic

Authored by ldionne on Jun 11 2022, 9:43 PM.

Details

Summary

For reference, this test creates about 1.5G in the cache directory. By default this will go to ~/.cache/clang/ which can fill up quick. This changes the test to put the cache path in lit temp directories. Size considerations aside it makes sense for tests to be hermetic and not touch global system state.

Diff Detail

Event Timeline

abrachet created this revision.Jun 11 2022, 9:43 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2022, 9:43 PM
abrachet requested review of this revision.Jun 11 2022, 9:43 PM
phosek accepted this revision.Jun 12 2022, 11:21 AM

LGTM but I'm not libc++ reviewer so you should wait for LGTM from either @ldionne or @EricWF.

This revision is now accepted and ready to land.Jun 12 2022, 11:21 AM

@ldionne ping would it be possible to take a look?

This revision was landed with ongoing or failed builds.Jun 14 2022, 11:08 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2022, 11:08 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne reopened this revision.Jun 14 2022, 12:24 PM

@abrachet Please don't commit patches to libc++ without having approval from the libc++ review group.

Thanks for wanting to improve the test suite -- this is indeed a slow test that creates a lot of stuff on the side. My main comment is actually whether it would make sense to reuse a modules-cache path in the whole test suite (even across tests)?

@abrachet Please don't commit patches to libc++ without having approval from the libc++ review group.

By the way, I'm not scolding, just making sure everybody follows the process :-). Let's have a discussion now that the immediate problem is solved!

@abrachet Please don't commit patches to libc++ without having approval from the libc++ review group.

By the way, I'm not scolding, just making sure everybody follows the process :-). Let's have a discussion now that the immediate problem is solved!

No worries I'm not feeling scolded :) I will wait next time.

Thanks for wanting to improve the test suite -- this is indeed a slow test that creates a lot of stuff on the side. My main comment is actually whether it would make sense to reuse a modules-cache path in the whole test suite (even across tests)?

Sure I could look into that. I'm not too familiar with libcxx's testing infrastructure, do you reckon that would be added to all of the %{compile_flags} substitutions?

ldionne commandeered this revision.Aug 31 2023, 8:38 AM
ldionne edited reviewers, added: abrachet; removed: ldionne.

[Github PR transition cleanup]

I think this is reasonable enough that we don't need to look into changing anything. I think it would be better to define a substitution like %{modules-cache} which is global to the test suite and then use -fmodules-cache-path=%{modules-cache} here in this test and also in params.py (where we currently define a modules cache path for clang-modules builds).

@abrachet If you want to investigate this, please open that review as a GH PR and we'll have a look. But otherwise the current state is acceptable as well.

I will commandeer this to close it to clear the review queue.

ldionne abandoned this revision.Aug 31 2023, 8:39 AM

This will be marked as abandoned but in reality this landed in 93c46e9632a6