This is an archive of the discontinued LLVM Phabricator instance.

Sanitizer tests CMake clean up
ClosedPublic

Authored by george.karpenkov on Jul 27 2017, 11:29 AM.

Details

Summary

This patch addresses two issues:

  1. Most of the time, hacks with if/else in order to get support for multi-configuration builds are superfluous. The variable CMAKE_CFG_INTDIR was created precisely for this purpose: it expands to . on all single-configuration builds, and to a configuration name otherwise.
  1. The if/else hacks for the library name generation should also not be done, as CMake has TARGET_FILE generator expression precisely for this purpose, as it expands to the exact filename of the resulting target.

Diff Detail

Event Timeline

rnk accepted this revision.Jul 27 2017, 11:33 AM

The resulting paths may be slightly uglier (foo/./bar), but the cmake is a lot less ugly. Thanks!

This revision is now accepted and ready to land.Jul 27 2017, 11:33 AM
This revision was automatically updated to reflect the committed changes.

CMake has a very interesting behavior when it comes to files created with add_custom_command.

The path passed in the OUTPUT parameter _is_ normalized, turning e.g. ./blah into blah.
But the normalization is _not_ applied to dependencies!

So if we create a target generating a file ./blah, and then depend on it, bad things would happen.
Furthermore, this is coupled with a fact that CMake will NOT create rules for custom commands on which nothing depends, and will NOT warn if you depend on file which does not exist, making debugging such a case a very interesting experience.

@rnk is it okay to apply this patch as well? Would also clean up the output path as a side effect.

george.karpenkov reopened this revision.Jul 27 2017, 5:41 PM
This revision is now accepted and ready to land.Jul 27 2017, 5:41 PM
george.karpenkov requested review of this revision.Jul 27 2017, 5:41 PM
george.karpenkov edited edge metadata.
rnk accepted this revision.Jul 27 2017, 5:46 PM

I'm not surprised, I figured Chapuni or whoever added that multi-config generator logic in the first place had a reason for doing it like that. :)

New thing looks better.

This revision is now accepted and ready to land.Jul 27 2017, 5:46 PM
This revision was automatically updated to reflect the committed changes.