Page MenuHomePhabricator

[CMake] Include dependency on cxx-headers in compiler-rt tests
ClosedPublic

Authored by phosek on Mar 31 2021, 11:26 PM.

Details

Diff Detail

Event Timeline

phosek created this revision.Mar 31 2021, 11:26 PM
phosek requested review of this revision.Mar 31 2021, 11:26 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2021, 11:26 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
ldionne added inline comments.Apr 1 2021, 7:50 AM
compiler-rt/test/profile/CMakeLists.txt
12 ↗(On Diff #334595)

As I've said in https://bugs.chromium.org/p/chromium/issues/detail?id=1194745, I think the right fix would be to make sure that we run these tests against a properly installed copy of Clang + libc++. Instead, what this will do is add a -I path/to/configured/include/c++/v1 argument to the compiler, which should hopefully work but also exposes an implementation detail of libc++'s build (the cxx-headers target) to compiler-rt.

TLDR, I think this will work but it's not the ideal fix IMO.

phosek added inline comments.Apr 1 2021, 10:08 AM
compiler-rt/test/profile/CMakeLists.txt
12 ↗(On Diff #334595)

I agree, but we still need to ensure that those headers are in place before we use them so we're still going to need that dependency.

ldionne accepted this revision.Apr 1 2021, 10:29 AM
ldionne added inline comments.
compiler-rt/test/profile/CMakeLists.txt
12 ↗(On Diff #334595)

We could do that by ensuring that the compiler-rt build as a whole installs libc++ before running its tests, and then running against the just-installed library (both headers and dylib). But let's do this if it fixes the build now.

This revision is now accepted and ready to land.Apr 1 2021, 10:29 AM

I'd be curious to hear if that solves @thakis 's problems though.

phosek updated this revision to Diff 334756.Apr 1 2021, 10:39 AM
phosek retitled this revision from [CMake] Includ a dependency on cxx-headers in profile tests to [CMake] Include dependency on cxx-headers in compiler-rt tests.
This revision was landed with ongoing or failed builds.Apr 1 2021, 10:42 AM
This revision was automatically updated to reflect the committed changes.
phosek added a comment.Apr 1 2021, 8:32 PM

Thanks for the heads up, I accidentally used set(...) instead of list(APPEND ...), it should be addressed by b0d286b03c6e20e57fd2ecf3ea7935669083f3e5.