Fix for linking different archs

[jkp]

Hi there

I was looking at trying out Ninja build with our rather large CMake C++ project but I hit a hurdle early on. We force the architecture to i386 even on x86_64 platforms and the targets the Ninja generator spat out seemed to choke when linking.

This is a patch which seems to fix that. I arrived at this by looking at the link rule for CXX (in Modules/CMakeCXXInformation.cmake)

<CMAKE_CXX_COMPILER> <FLAGS> <CMAKE_CXX_LINK_FLAGS> <LINK_FLAGS> <OBJECTS> -o <TARGET> <LINK_LIBRARIES>

Although you look up the architecture flags in cmNinjaNormalTargetGenerator::WriteLinkRule, they are only included in the build rule if it references <LANGUAGE_COMPILE_FLAGS> (which this rule doesn't).

I used the code in the makefile generator as a reference - in there they store them in FLAGS and they are picked up from there.

[pcc]

On Mon, Jan 30, 2012 at 06:55:36AM -0800, Jamie Kirkpatrick wrote:

Hi there

I was looking at trying out Ninja build with our rather large CMake C++ project but I hit a hurdle early on. We force the architecture to i386 even on x86_64 platforms and the targets the Ninja generator spat out seemed to choke when linking.

This is a patch which seems to fix that. I arrived at this by looking at the link rule for CXX (in Modules/CMakeCXXInformation.cmake)

<CMAKE_CXX_COMPILER> <FLAGS> <CMAKE_CXX_LINK_FLAGS> <LINK_FLAGS> <OBJECTS> -o <TARGET> <LINK_LIBRARIES>

Although you look up the architecture flags in cmNinjaNormalTargetGenerator::WriteLinkRule, they are only included in the build rule if it references <LANGUAGE_COMPILE_FLAGS> (which this rule doesn't).

I used the code in the makefile generator as a reference - in there they store them in FLAGS and they are picked up from there.

The latest version of the code is in the ninja-generator-pr branch, and this architecture flag issue was "fixed" while I was working on the Mac OS X port. If you look at both cmMakefileExecutableTargetGenerator and cmMakefileLibraryTargetGenerator you will notice that Executable adds the architecture flags to FLAGS and Library adds them to LANGUAGE_COMPILE_FLAGS. This looks a lot like a mistake made when duplicating code between Executable and Library. (Of course, in my generator I avoid any such nonsense by combining Executable and Library into a single module).

Rather than trying to fix this in the makefile generator and the modules (at the risk of breaking something) I decided to mimic this behaviour in my generator, for better or worse. Below are the changes that I made (which are now rolled up into the ninja-generator-pr branch):

diff --git a/Source/cmNinjaNormalTargetGenerator.cxx b/Source/cmNinjaNormalTargetGenerator.cxx
index c3894ae..7bfdd46 100644
--- a/Source/cmNinjaNormalTargetGenerator.cxx
+++ b/Source/cmNinjaNormalTargetGenerator.cxx
@@ -117,6 +117,7 @@ void
 cmNinjaNormalTargetGenerator
 ::WriteLinkRule()
 {
+  cmTarget::TargetType targetType = this->GetTarget()->GetType();
   std::string ruleName = this->LanguageLinkerRule();

   if (!this->GetGlobalGenerator()->HasRule(ruleName)) {
@@ -161,7 +162,8 @@ cmNinjaNormalTargetGenerator
     this->GetLocalGenerator()->AddLanguageFlags(langFlags,
                                                 this->TargetLinkLanguage,
                                                 this->GetConfigName());
-    langFlags += "$ARCHITECTURE_FLAGS";
+    if (targetType != cmTarget::EXECUTABLE)
+      langFlags += " $ARCH_FLAGS";
     vars.LanguageCompileFlags = langFlags.c_str();

     // Rule for linking library.
@@ -193,7 +195,7 @@ cmNinjaNormalTargetGenerator
   if (this->TargetNameOut != this->TargetNameReal) {
     std::string cmakeCommand =
       this->GetMakefile()->GetRequiredDefinition("CMAKE_COMMAND");
-    if (this->GetTarget()->GetType() == cmTarget::EXECUTABLE)
+    if (targetType == cmTarget::EXECUTABLE)
       this->GetGlobalGenerator()->AddRule("CMAKE_SYMLINK_EXECUTABLE",
                                           cmakeCommand +
                                           " -E cmake_symlink_executable"
@@ -283,11 +285,13 @@ cmNinjaNormalTargetGenerator

 void cmNinjaNormalTargetGenerator::WriteLinkStatement()
 {
+  cmTarget::TargetType targetType = this->GetTarget()->GetType();
+
   // Write comments.
   cmGlobalNinjaGenerator::WriteDivider(this->GetBuildFileStream());
   this->GetBuildFileStream()
     << "# Link build statements for "
-    << cmTarget::GetTargetTypeName(this->GetTarget()->GetType())
+    << cmTarget::GetTargetTypeName(targetType)
     << " target "
     << this->GetTargetName()
     << "\n\n";
@@ -320,14 +324,19 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement()
                                             vars["LINK_FLAGS"],
                                             *this->GetTarget());

-  // Compute specific link flags.
-  this->GetLocalGenerator()->AddArchitectureFlags(vars["ARCHITECTURE_FLAGS"],
-                                                  this->GetTarget(),
-                                                  this->TargetLinkLanguage,
-                                                  this->GetConfigName());
+  // Compute architecture specific link flags.  Yes, these go into a different
+  // variable for executables, probably due to a mistake made when duplicating
+  // code between the Makefile executable and library generators.
+  this->GetLocalGenerator()
+      ->AddArchitectureFlags(targetType == cmTarget::EXECUTABLE
+                               ? vars["FLAGS"]
+                               : vars["ARCH_FLAGS"],
+                             this->GetTarget(),
+                             this->TargetLinkLanguage,
+                             this->GetConfigName());
   vars["SONAME"] = this->TargetNameSO;

-  if (this->GetTarget()->GetType() == cmTarget::SHARED_LIBRARY) {
+  if (targetType == cmTarget::SHARED_LIBRARY) {
     std::string install_name_dir =
       this->GetTarget()->GetInstallNameDirForBuildTree(this->GetConfigName());

@@ -391,7 +400,7 @@ void cmNinjaNormalTargetGenerator::WriteLinkStatement()
                                      vars);

   if (targetOutput != targetOutputReal) {
-    if (this->GetTarget()->GetType() == cmTarget::EXECUTABLE) {
+    if (targetType == cmTarget::EXECUTABLE) {
       cmGlobalNinjaGenerator::WriteBuild(this->GetBuildFileStream(),
                                   "Create executable symlink " + targetOutput,
                                          "CMAKE_SYMLINK_EXECUTABLE",

Thanks,

Peter