From bc30f8b5e66cb9c15fd224f5e51454c0bc66c67e Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 1 Apr 2016 09:13:16 -0400 Subject: [PATCH 1/2] Fix lookup of an ALIAS target outside aliased target's directory (#16044) Refactoring in commit v3.5.0-rc1~272^2~11 (cmTarget: Implement ALIAS in terms of name mapping, 2015-10-25) accidentally introduced logic that assumes ALIAS targets always reference targets in their own directory. Fix this and add a test case. The configure-step fix is that `cmMakefile::FindTarget` should not consider aliases. The purpose of this method is just to look up targets local to a directory. Since ALIAS and normal targets share a namespace we know a locally defined target will never collide with an ALIAS target anyway. The method has 3 call sites, and this change is safe for all of them: * `cmInstallCommand::HandleTargetsMode`: Rejects aliases before the call. * `cmFLTKWrapUICommand::FinalPass`: Should never have considered aliases. * `cmMakefile::FindTargetToUse`: Falls back to a global lookup anyway. The generate-step fix is that `cmLocalGenerator::FindGeneratorTarget` should not consider aliases. This method is the generate-step equivalent to the above. The method has 2 call sites, and this change is safe for both of them: * `cmInstallTargetGenerator::Compute`: Never uses an alias target name. * `cmLocalGenerator::FindGeneratorTargetToUse`: Falls back to global lookup. Reported-by: Matteo Settenvini --- Source/cmLocalGenerator.cxx | 11 ----------- Source/cmMakefile.cxx | 16 ++-------------- Source/cmMakefile.h | 3 +-- Tests/AliasTarget/subdir/CMakeLists.txt | 5 +++++ 4 files changed, 8 insertions(+), 27 deletions(-) diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index 912be0c0f..6153fbd79 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -488,16 +488,6 @@ private: cmGeneratorTarget* cmLocalGenerator::FindGeneratorTarget( const std::string& name) const { - std::map::const_iterator i = - this->AliasTargets.find(name); - if (i != this->AliasTargets.end()) - { - std::vector::const_iterator ai = - std::find_if(this->GeneratorTargets.begin(), - this->GeneratorTargets.end(), - NamedGeneratorTargetFinder(i->second)); - return *ai; - } std::vector::const_iterator ti = std::find_if(this->GeneratorTargets.begin(), this->GeneratorTargets.end(), @@ -506,7 +496,6 @@ cmGeneratorTarget* cmLocalGenerator::FindGeneratorTarget( { return *ti; } - return 0; } diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 600c985f0..598f8c4b3 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -4058,25 +4058,13 @@ std::vector cmMakefile::GetPropertyKeys() const return this->StateSnapshot.GetDirectory().GetPropertyKeys(); } -cmTarget* cmMakefile::FindTarget(const std::string& name, - bool excludeAliases) const +cmTarget* cmMakefile::FindTarget(const std::string& name) const { - if (!excludeAliases) - { - std::map::const_iterator i = - this->AliasTargets.find(name); - if (i != this->AliasTargets.end()) - { - cmTargets::iterator ai = this->Targets.find(i->second); - return &ai->second; - } - } cmTargets::iterator i = this->Targets.find( name ); if ( i != this->Targets.end() ) { return &i->second; } - return 0; } @@ -4247,7 +4235,7 @@ cmTarget* cmMakefile::FindTargetToUse(const std::string& name, } // Look for a target built in this directory. - if(cmTarget* t = this->FindTarget(name, excludeAliases)) + if(cmTarget* t = this->FindTarget(name)) { return t; } diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index 362ea75fb..a69c7056d 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -388,8 +388,7 @@ public: } std::vector GetImportedTargets() const; - cmTarget* FindTarget(const std::string& name, - bool excludeAliases = false) const; + cmTarget* FindTarget(const std::string& name) const; /** Find a target to use in place of the given name. The target returned may be imported or built within the project. */ diff --git a/Tests/AliasTarget/subdir/CMakeLists.txt b/Tests/AliasTarget/subdir/CMakeLists.txt index 8c84aea3c..05a7d86f0 100644 --- a/Tests/AliasTarget/subdir/CMakeLists.txt +++ b/Tests/AliasTarget/subdir/CMakeLists.txt @@ -1,3 +1,8 @@ add_library(tgt STATIC empty.cpp) add_library(Sub::tgt ALIAS tgt) + +# foo comes from the top-level CMakeLists.txt +add_library(Top::foo ALIAS foo) +get_target_property(some_prop Top::foo SOME_PROP) +target_link_libraries(tgt Top::foo) From 0e44f4894f23d5eccd8d360f3420c832f9433a20 Mon Sep 17 00:00:00 2001 From: Brad King Date: Fri, 1 Apr 2016 10:26:08 -0400 Subject: [PATCH 2/2] Rename local target lookup methods to clarify purpose Rename methods: * `cmMakefile::Find{ => LocalNonAlias}Target` * `cmLocalGenerator::Find{ => LocalNonAlias}GeneratorTarget` These names clarify that they are for directory-local target names and do not consider alias targets. --- Source/cmFLTKWrapUICommand.cxx | 2 +- Source/cmInstallCommand.cxx | 2 +- Source/cmInstallTargetGenerator.cxx | 2 +- Source/cmLocalGenerator.cxx | 4 ++-- Source/cmLocalGenerator.h | 3 ++- Source/cmMakefile.cxx | 4 ++-- Source/cmMakefile.h | 2 +- 7 files changed, 10 insertions(+), 9 deletions(-) diff --git a/Source/cmFLTKWrapUICommand.cxx b/Source/cmFLTKWrapUICommand.cxx index d17d6646f..c64e81315 100644 --- a/Source/cmFLTKWrapUICommand.cxx +++ b/Source/cmFLTKWrapUICommand.cxx @@ -117,7 +117,7 @@ void cmFLTKWrapUICommand::FinalPass() // people should add the srcs to the target themselves, but the old command // didn't support that, so check and see if they added the files in and if // they didn;t then print a warning and add then anyhow - cmTarget* target = this->Makefile->FindTarget(this->Target); + cmTarget* target = this->Makefile->FindLocalNonAliasTarget(this->Target); if(!target) { std::string msg = diff --git a/Source/cmInstallCommand.cxx b/Source/cmInstallCommand.cxx index 2d78a4101..7e72a8a99 100644 --- a/Source/cmInstallCommand.cxx +++ b/Source/cmInstallCommand.cxx @@ -381,7 +381,7 @@ bool cmInstallCommand::HandleTargetsMode(std::vector const& args) return false; } // Lookup this target in the current directory. - if(cmTarget* target=this->Makefile->FindTarget(*targetIt)) + if(cmTarget* target=this->Makefile->FindLocalNonAliasTarget(*targetIt)) { // Found the target. Check its type. if(target->GetType() != cmState::EXECUTABLE && diff --git a/Source/cmInstallTargetGenerator.cxx b/Source/cmInstallTargetGenerator.cxx index 5e88fa2c7..b93fb8d61 100644 --- a/Source/cmInstallTargetGenerator.cxx +++ b/Source/cmInstallTargetGenerator.cxx @@ -446,7 +446,7 @@ cmInstallTargetGenerator::GetInstallFilename(cmGeneratorTarget const* target, void cmInstallTargetGenerator::Compute(cmLocalGenerator* lg) { - this->Target = lg->FindGeneratorTarget(this->TargetName); + this->Target = lg->FindLocalNonAliasGeneratorTarget(this->TargetName); } //---------------------------------------------------------------------------- diff --git a/Source/cmLocalGenerator.cxx b/Source/cmLocalGenerator.cxx index 6153fbd79..586e4c69b 100644 --- a/Source/cmLocalGenerator.cxx +++ b/Source/cmLocalGenerator.cxx @@ -485,7 +485,7 @@ private: std::string Name; }; -cmGeneratorTarget* cmLocalGenerator::FindGeneratorTarget( +cmGeneratorTarget* cmLocalGenerator::FindLocalNonAliasGeneratorTarget( const std::string& name) const { std::vector::const_iterator ti = @@ -1828,7 +1828,7 @@ cmLocalGenerator::FindGeneratorTargetToUse(const std::string& name) const return *imported; } - if(cmGeneratorTarget* t = this->FindGeneratorTarget(name)) + if(cmGeneratorTarget* t = this->FindLocalNonAliasGeneratorTarget(name)) { return t; } diff --git a/Source/cmLocalGenerator.h b/Source/cmLocalGenerator.h index 68e766741..b673a85cc 100644 --- a/Source/cmLocalGenerator.h +++ b/Source/cmLocalGenerator.h @@ -129,7 +129,8 @@ public: void AddImportedGeneratorTarget(cmGeneratorTarget* gt); void AddOwnedImportedGeneratorTarget(cmGeneratorTarget* gt); - cmGeneratorTarget* FindGeneratorTarget(const std::string& name) const; + cmGeneratorTarget* + FindLocalNonAliasGeneratorTarget(const std::string& name) const; cmGeneratorTarget* FindGeneratorTargetToUse(const std::string& name) const; /** diff --git a/Source/cmMakefile.cxx b/Source/cmMakefile.cxx index 598f8c4b3..df687d03e 100644 --- a/Source/cmMakefile.cxx +++ b/Source/cmMakefile.cxx @@ -4058,7 +4058,7 @@ std::vector cmMakefile::GetPropertyKeys() const return this->StateSnapshot.GetDirectory().GetPropertyKeys(); } -cmTarget* cmMakefile::FindTarget(const std::string& name) const +cmTarget* cmMakefile::FindLocalNonAliasTarget(const std::string& name) const { cmTargets::iterator i = this->Targets.find( name ); if ( i != this->Targets.end() ) @@ -4235,7 +4235,7 @@ cmTarget* cmMakefile::FindTargetToUse(const std::string& name, } // Look for a target built in this directory. - if(cmTarget* t = this->FindTarget(name)) + if(cmTarget* t = this->FindLocalNonAliasTarget(name)) { return t; } diff --git a/Source/cmMakefile.h b/Source/cmMakefile.h index a69c7056d..45f2efb92 100644 --- a/Source/cmMakefile.h +++ b/Source/cmMakefile.h @@ -388,7 +388,7 @@ public: } std::vector GetImportedTargets() const; - cmTarget* FindTarget(const std::string& name) const; + cmTarget* FindLocalNonAliasTarget(const std::string& name) const; /** Find a target to use in place of the given name. The target returned may be imported or built within the project. */