Don't set linker for Rust static libraries

As discussed in issue #270 we at least should not set the linker for Rust static libraries. In the general case the linker is not needed for static libraries, and if it is invoked, Rust should take care of what to choose (e.g. for building build scripts) instead of us forcing the choice.
corrosion-rs · Dec 18, 2022 · fa4e241 · fa4e241
1 parent 949ded9
commit fa4e241
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 18 deletions.
diff --git a/cmake/Corrosion.cmake b/cmake/Corrosion.cmake
@@ -699,8 +699,8 @@ endif()
 # A target may be either a specific bin
 function(_add_cargo_build out_cargo_build_out_dir)
     set(options NO_LINKER_OVERRIDE)
-    set(one_value_args PACKAGE TARGET MANIFEST_PATH PROFILE TARGET_KIND WORKSPACE_MANIFEST_PATH)
-    set(multi_value_args BYPRODUCTS)
+    set(one_value_args PACKAGE TARGET MANIFEST_PATH PROFILE WORKSPACE_MANIFEST_PATH)
+    set(multi_value_args BYPRODUCTS TARGET_KINDS)
     cmake_parse_arguments(
         ACB
         "${options}"
@@ -713,17 +713,18 @@ function(_add_cargo_build out_cargo_build_out_dir)
     set(target_name "${ACB_TARGET}")
     set(path_to_toml "${ACB_MANIFEST_PATH}")
     set(cargo_profile_name "${ACB_PROFILE}")
-    set(target_kind "${ACB_TARGET_KIND}")
+    set(target_kinds "${ACB_TARGET_KINDS}")
     set(workspace_manifest_path "${ACB_WORKSPACE_MANIFEST_PATH}")
 
-    if(NOT target_kind)
-        message(FATAL_ERROR "TARGET_KIND not specified")
-    elseif(target_kind STREQUAL "lib")
+
+    if(NOT target_kinds)
+        message(FATAL_ERROR "TARGET_KINDS not specified")
+    elseif("staticlib" IN_LIST target_kinds OR "cdylib" IN_LIST target_kinds)
         set(cargo_rustc_filter "--lib")
-    elseif(target_kind STREQUAL "bin")
+    elseif("bin" IN_LIST target_kinds)
         set(cargo_rustc_filter "--bin=${target_name}")
     else()
-        message(FATAL_ERROR "TARGET_KIND must be `lib` or `bin`, but was `${target_kind}`")
+        message(FATAL_ERROR "TARGET_KINDS contained unknown kind `${target_kind}`")
     endif()
 
     if (NOT IS_ABSOLUTE "${path_to_toml}")
@@ -742,9 +743,21 @@ function(_add_cargo_build out_cargo_build_out_dir)
     unset(is_windows_msvc)
     get_source_file_property(is_windows_msvc "${workspace_manifest_path}" CORROSION_PLATFORM_IS_WINDOWS_MSVC)
 
-    # For MSVC targets, don't mess with linker preferences.
-    # TODO: We still should probably make sure that rustc is using the correct cl.exe to link programs.
-    if (NOT is_windows_msvc)
+    # Corrosions currently attempts to select a correct linker, based on the enabled languages.
+    # This approach is flawed and should be revisited in the future.
+    # Currently we disable this approach for the MSVC abi, because it just doesn't work
+    # and for static libraries, because the linker shouldn't be invoked for those, but
+    # potentially could be if Rust side build scripts or proc macros etc. happened to be involved.
+    # Overriding the linker in those cases is probably unwanted.
+    if(is_windows_msvc)
+        set(determine_linker_preference FALSE)
+    elseif("staticlib" IN_LIST target_kinds AND NOT "cdylib" IN_LIST target_kinds)
+        set(determine_linker_preference FALSE)
+    else()
+        set(determine_linker_preference TRUE)
+    endif()
+
+    if(determine_linker_preference)
         set(languages C CXX Fortran)
 
         set(has_compiler OFF)

diff --git a/cmake/CorrosionGenerator.cmake b/cmake/CorrosionGenerator.cmake
@@ -105,7 +105,7 @@ function(_generator_add_package_targets workspace_manifest_path package_manifest
                 MANIFEST_PATH "${manifest_path}"
                 WORKSPACE_MANIFEST_PATH "${workspace_manifest_path}"
                 PROFILE "${profile}"
-                TARGET_KIND "lib"
+                TARGET_KINDS "${kinds}"
                 BYPRODUCTS "${byproducts}"
             )
             if(archive_byproducts)
@@ -143,7 +143,7 @@ function(_generator_add_package_targets workspace_manifest_path package_manifest
                 MANIFEST_PATH "${manifest_path}"
                 WORKSPACE_MANIFEST_PATH "${workspace_manifest_path}"
                 PROFILE "${profile}"
-                TARGET_KIND "bin"
+                TARGET_KINDS "bin"
                 BYPRODUCTS "${byproducts}"
             )
             _corrosion_copy_byproducts(

diff --git a/generator/src/subcommands/gen_cmake/target.rs b/generator/src/subcommands/gen_cmake/target.rs
@@ -19,6 +19,26 @@ pub struct CargoTarget {
     workspace_manifest_path: Rc<PathBuf>,
 }
 
+impl CargoTargetType {
+    fn to_string(&self) -> String {
+        let mut s = String::new();
+        match self {
+            Self::Executable => {
+                s.push_str("bin");
+            },
+            Self::Library { has_staticlib, has_cdylib} => {
+                if *has_staticlib {
+                    s.push_str("staticlib")
+                }
+                if *has_cdylib {
+                    s.push_str(" cdylib")
+                }
+            }
+        }
+        s
+    }
+}
+
 impl CargoTarget {
     pub fn from_metadata(
         cargo_package: Rc<cargo_metadata::Package>,
@@ -75,7 +95,7 @@ impl CargoTarget {
             .expect("Non-utf8 path encountered")
             .replace("\\", "/");
 
-        let target_kind = match self.target_type {
+        match self.target_type {
             CargoTargetType::Library {
                 has_staticlib,
                 has_cdylib,
@@ -109,7 +129,6 @@ impl CargoTarget {
                     has_staticlib = has_staticlib,
                     has_cdylib = has_cdylib,
                 )?;
-                "lib"
             }
             CargoTargetType::Executable => {
                 writeln!(
@@ -124,9 +143,9 @@ impl CargoTarget {
                     workspace_manifest_path = ws_manifest,
                     target_name = self.cargo_target.name,
                 )?;
-                "bin"
             }
         };
+        let target_kinds = self.target_type.to_string();
         writeln!(out_file,
             "
             set(cargo_build_out_dir \"\")
@@ -137,7 +156,7 @@ impl CargoTarget {
                 MANIFEST_PATH \"{package_manifest_path}\"
                 WORKSPACE_MANIFEST_PATH \"{workspace_manifest_path}\"
                 {profile_option}
-                TARGET_KIND \"{target_kind}\"
+                TARGET_KINDS {target_kinds}
                 BYPRODUCTS \"${{byproducts}}\"
             )
 
@@ -171,7 +190,7 @@ impl CargoTarget {
             package_manifest_path = self.cargo_package.manifest_path.as_str().replace("\\", "/"),
             workspace_manifest_path = ws_manifest,
             profile_option = cargo_build_profile_option,
-            target_kind = target_kind,
+            target_kinds = target_kinds,
 
         )?;
         Ok(())