This is an archive of the discontinued LLVM Phabricator instance.

[CMake] Remove unnecesary list of source files for Xray unit tests.
ClosedPublic

Authored by delcypher on Jul 11 2018, 5:54 AM.

Details

Summary

[CMake] Remove unnecesary list of source files for Xray unit tests.

The list duplicates information already available in the parent
directory so use that instead. It is unclear to me why we need
to spell out the dependencies explicitly but fixing that should
be done in a separate patch.

Diff Detail

Event Timeline

delcypher created this revision.Jul 11 2018, 5:54 AM
Herald added subscribers: Restricted Project, mgorny. · View Herald TranscriptJul 11 2018, 5:54 AM

@dberris The tests aren't built under macOS so I couldn't test if this patch actually works. Would it be possible for you to test it for me?

george.karpenkov resigned from this revision.Jul 11 2018, 2:32 PM

Looks reasonable, but I think @dberris would have to approve.

dberris accepted this revision.Jul 11 2018, 4:53 PM
dberris added a subscriber: nglevin.

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

I'm fine with landing this then iterating, no need for me to test myself.

This revision is now accepted and ready to land.Jul 11 2018, 4:53 PM

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

In general, in CMake, the target for running a test is made to be dependent on the unit test being executed.. In turn the unit test is linked against the libraries it uses. If the source files for a library change that should trigger the library to be rebuilt. At the minimum this will cause the unit test to be relinked. If the unit test also depends on header files that have changed that should cause the unit test to be recompiled too.

Why is that not sufficient for us?

I'm fine with landing this then iterating, no need for me to test myself.

Okay. I'll land this now.

This revision was automatically updated to reflect the committed changes.

LGTM -- @nglevin was working on getting the XRay unit tests at least build and run but got interrupted.

The original reason for doing this was to ensure that when any of the implementation files changed, that the unit tests would be built. As you've learned this only made sense for platforms where the tests are built and run.

In general, in CMake, the target for running a test is made to be dependent on the unit test being executed.. In turn the unit test is linked against the libraries it uses. If the source files for a library change that should trigger the library to be rebuilt. At the minimum this will cause the unit test to be relinked. If the unit test also depends on header files that have changed that should cause the unit test to be recompiled too.

Why is that not sufficient for us?

I looked into this a bit and it seems that, since the linking of the XRay runtimes is done by clang, there's no hard dependency between the runtime library built and the one that clang adds to the link line. This causes the issue where even if the XRay runtime is re-built, the unit tests don't get re-linked because there wasn't an explicit dependency between the test and the files. This is important for the flags, for instance, along with the runtime-initialisation code that the XRay unit tests also depend on.

This gets worse for internal headers/implementation details changing, since the generate_compiler_rt_tests(...) function doesn't actually use file dependency tracking (implicit tracking I mean) but instead does something different from the normal CMake approach of say using add_test(...) and having normal test binaries.

It can be argued that we could split up the components better, and is definitely something worth exploring later if there's a better way of doing this.