[cmake-developers] Generator expressions for install destination

[Robert Goulet]

> This and its related changes are also refactoring that should go in its own commit.

Ok let's begin with this. Patch attached for adding makefile to install generators. This refactoring is required for install(FILES) genex support, and most likely other install() signatures in the future.

Thanks.

-----Original Message-----
From: Brad King [mailto:brad.king at kitware.com] 
Sent: Monday, September 21, 2015 1:55 PM
To: Robert Goulet <Robert.Goulet at autodesk.com>
Cc: cmake-developers at cmake.org
Subject: Re: Generator expressions for install destination

On 09/18/2015 03:49 PM, Robert Goulet wrote:
> Here's a version that is more conservative. It doesn't change the 
> install(EXPORT) behavior.

> install(TARGET) already supported genex

Right.  I'd forgotten about these changes:

 install: Allow generator expressions in TARGETS DESTINATION (#14317)  http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f30022eb
 http://www.cmake.org/Bug/view.php?id=14317#c37959

>, so basically this patch adds install(FILES) destination genex.

Okay.  I see many hunks of this form:

> -                       this->Destination,
> +                       this->GetDestination(),

Making the Destination member private and moving GetDestination() back into cmInstallGenerator is refactoring that should be split out into its own commit.  Note that GetDestination() was removed from the base class here:

 cmInstallGenerator: Move GetDestination to subclasses that need it  http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=f99991db

Restoring it will need a justification in the commit message.
I'm concerned about having both overloads avilable:

> +  std::string GetDestination() const;  std::string 
> + GetDestination(std::string const& config) const;

when only one is safe to call from each subclass.  This is why the above-linked commit removed the method from cmInstallGenerator in favor of having the needed overload in each subclass.

> -  cmInstallGenerator(const char* destination,
> +  cmInstallGenerator(cmMakefile* mf,
> +                     const char* destination,

This and its related changes are also refactoring that should go in its own commit.

> +  // We need per-config actions if destination have generator expressions.
> +  if(cmGeneratorExpression::Find(Destination) != std::string::npos)
> +    {
> +    this->ActionsPerConfig = true;
> +    }

Was this the solution to the availability of a configuration for calls to GetDestination()?

> Perhaps we should update the Help to only mention install(FILES) 
> destination instead of all variations of install?

Yes, the actual availability of the behavior should be documented.

Also please look at making the test suite actually verify that the installation works as expected.  The above-linked commit f30022eb adds a test that fails if generator expressions are not evaluated to the correct values.

Thanks,
-Brad

-------------- next part --------------
A non-text attachment was scrubbed...
Name: add-makefile-to-install-generators.patch
Type: application/octet-stream
Size: 15306 bytes
Desc: add-makefile-to-install-generators.patch
URL: <http://public.kitware.com/pipermail/cmake-developers/attachments/20150922/d65a6ac7/attachment.obj>