This is an archive of the discontinued LLVM Phabricator instance.

[runtimes] Detect changes to Tests.cmake
ClosedPublic

Authored by phosek on Mar 14 2022, 3:18 PM.

Details

Summary

This ensures that Tests.cmake is tracked by Ninja and any changes to
this file from the subbuilds are correctly detected.

Diff Detail

Event Timeline

phosek created this revision.Mar 14 2022, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 3:18 PM
Herald added a subscriber: mgorny. · View Herald Transcript
phosek requested review of this revision.Mar 14 2022, 3:18 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2022, 3:18 PM
phosek updated this revision to Diff 415336.Mar 15 2022, 12:16 AM
phosek retitled this revision from [runtimes] Create an empty Tests.cmake if absent to [runtimes] Detect changes to Tests.cmake.
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 15 2022, 12:16 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
Herald added a reviewer: Restricted Project. · View Herald Transcript
jdenny added a subscriber: jdenny.Mar 15 2022, 1:46 PM
kamaub added a subscriber: kamaub.Mar 16 2022, 6:10 AM

@ldionne would you be willing to accept this? This should address the issue reported in D121276.

@ldionne would you be willing to accept this? This should address the issue reported in D121276.

I don't see something fundamentally wrong with it, but CI isn't passing on the Bootstrapping build, so it looks like something's off. Sorry, I was waiting for CI to be green before reviewing.

ldionne added inline comments.Mar 16 2022, 11:52 AM
llvm/runtimes/CMakeLists.txt
225

I'm a bit confused -- doesn't this mean that we're going to include an empty Tests.cmake file and hence all the LLVM_RUNTIMES_LIT_TESTSUITES & friends below will expand to nothing?

@ldionne would you be willing to accept this? This should address the issue reported in D121276.

I don't see something fundamentally wrong with it, but CI isn't passing on the Bootstrapping build, so it looks like something's off. Sorry, I was waiting for CI to be green before reviewing.

I think I understand the issue, I'll upload a new version soon.

llvm/runtimes/CMakeLists.txt
225

Yes, this is just so we can include this file unconditionally below which is only necessary during the clean build. We could also use include(... OPTIONAL) but in my experience that didn't result in Tests.cmake being properly tracked by Ninja until you re-run CMake.

phosek updated this revision to Diff 416399.Mar 17 2022, 11:29 PM
phosek marked an inline comment as done.
phosek added inline comments.
llvm/runtimes/CMakeLists.txt
225

Turned out this is no longer need with BYPRODUCTS.

ldionne accepted this revision.Mar 18 2022, 7:34 AM
This revision is now accepted and ready to land.Mar 18 2022, 7:34 AM
This revision was landed with ongoing or failed builds.Mar 18 2022, 10:02 AM
This revision was automatically updated to reflect the committed changes.

The version of this patch that landed did not fix the openmp problem caused by D121276. I backed it out (locally) and applied the version that I originally tested on 3/15, and all seems fine (that is, check-all includes openmp tests). I'm testing at 3edec279dfa7.

The version of this patch that landed did not fix the openmp problem caused by D121276. I backed it out (locally) and applied the version that I originally tested on 3/15, and all seems fine (that is, check-all includes openmp tests). I'm testing at 3edec279dfa7.

@phosek I'm concerned I wasn't clear here: things are still broken upstream. check-all still doesn't include openmp tests.

The May 15 version of this patch does fix it. Can you apply something more like it?

Thanks.