> 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.k...@kitware.com] Sent: Monday, September 21, 2015 1:55 PM To: Robert Goulet <robert.gou...@autodesk.com> Cc: cmake-developers@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
add-makefile-to-install-generators.patch
Description: add-makefile-to-install-generators.patch
-- Powered by www.kitware.com Please keep messages on-topic and check the CMake FAQ at: http://www.cmake.org/Wiki/CMake_FAQ Kitware offers various services to support the CMake community. For more information on each offering, please visit: CMake Support: http://cmake.org/cmake/help/support.html CMake Consulting: http://cmake.org/cmake/help/consulting.html CMake Training Courses: http://cmake.org/cmake/help/training.html Visit other Kitware open-source projects at http://www.kitware.com/opensource/opensource.html Follow this link to subscribe/unsubscribe: http://public.kitware.com/mailman/listinfo/cmake-developers
