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

Repository
rL LLVM

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.