Implement pkg-config flag deduplication

Should reduce a lot of the noise in Cflags and Libraries.

See #455

Author: amyspark

Cargo.toml

@@ -45,6 +45,7 @@ glob = "0.3"
 itertools = "0.14"
 implib = "0.3.5"
 object = { version = "0.36.4", default-features = false, features = ["std", "read_core", "pe"] }
+pkg-config = "0.3.32"
 
 [features]
 default = []

src/build.rs

@@ -235,14 +235,14 @@ struct FingerPrint {
     root_output: PathBuf,
     build_targets: BuildTargets,
     install_paths: InstallPaths,
-    static_libs: String,
+    static_libs: Vec<String>,
     hasher: DefaultHasher,
 }
 
 #[derive(serde::Serialize, serde::Deserialize)]
 struct Cache {
     hash: String,
-    static_libs: String,
+    static_libs: Vec<String>,
 }
 
 impl FingerPrint {
@@ -262,7 +262,7 @@ impl FingerPrint {
             root_output: root_output.to_owned(),
             build_targets: build_targets.clone(),
             install_paths: install_paths.clone(),
-            static_libs: String::new(),
+            static_libs: vec![],
             hasher,
         }
     }
@@ -1071,8 +1071,8 @@ impl LibraryTypes {
     }
 }
 
-fn static_libraries(link_line: &str, rustc_target: &target::Target) -> String {
-    link_line
+fn static_libraries(link_line: &str, rustc_target: &target::Target) -> Vec<String> {
+    let libs = link_line
         .trim()
         .split(' ')
         .filter(|s| {
@@ -1081,16 +1081,43 @@ fn static_libraries(link_line: &str, rustc_target: &target::Target) -> String {
             }
             !s.is_empty()
         })
-        .unique()
         .map(|lib| {
             if rustc_target.env == "msvc" && lib.ends_with(".lib") {
                 return format!("-l{}", lib.trim_end_matches(".lib"));
             }
             lib.trim().to_string()
         })
         .filter(|s| !s.is_empty())
-        .collect::<Vec<_>>()
-        .join(" ")
+        .collect::<Vec<_>>();
+
+    let mut final_libs: Vec<String> = vec![];
+
+    let mut iter = libs.iter();
+
+    // See pkg_config::Library::parse_libs_cflags
+    // Reconstitute improperly split lines
+    while let Some(part) = iter.next() {
+        match part.as_str() {
+            "-framework" => {
+                if let Some(lib) = iter.next() {
+                    final_libs.push(format!("-framework {}", lib));
+                }
+            }
+            "-isystem" | "-iquote" | "-idirafter" => {
+                if let Some(inc) = iter.next() {
+                    final_libs.push(format!("{} {}", part, inc));
+                }
+            }
+            "-undefined" | "--undefined" => {
+                if let Some(symbol) = iter.next() {
+                    final_libs.push(format!("-Wl,{},{}", part, symbol));
+                }
+            }
+            _ => final_libs.push(part.to_string()),
+        }
+    }
+
+    final_libs.into_iter().unique().collect()
 }
 
 pub fn cbuild(
@@ -1191,20 +1218,27 @@ pub fn cbuild(
         if new_build {
             let name = &cpkg.capi_config.library.name;
             let (pkg_config_static_libs, static_libs) = if library_types.only_cdylib() {
-                (String::new(), String::new())
+                (vec![String::new()], vec![String::new()])
             } else if let Some(libs) = exec.link_line.lock().unwrap().get(&cpkg.finger_print.id) {
-                (static_libraries(libs, &rustc_target), libs.to_string())
+                (
+                    static_libraries(libs, &rustc_target),
+                    vec![libs.to_string()],
+                )
             } else {
-                (String::new(), String::new())
+                (vec![String::new()], vec![String::new()])
             };
             let capi_config = &cpkg.capi_config;
             let build_targets = &cpkg.build_targets;
 
             let mut pc = PkgConfig::from_workspace(name, &cpkg.install_paths, args, capi_config);
             if library_types.only_staticlib() {
-                pc.add_lib(&pkg_config_static_libs);
+                for lib in &pkg_config_static_libs {
+                    pc.add_lib(lib);
+                }
+            }
+            for lib in pkg_config_static_libs {
+                pc.add_lib_private(&lib);
             }
-            pc.add_lib_private(&pkg_config_static_libs);
 
             build_pc_files(ws, &capi_config.pkg_config.filename, &root_output, &pc)?;
 
@@ -1324,7 +1358,7 @@ pub fn ctest(
 
         // We push the static_libs as CFLAGS as well to avoid mangling the options on msvc
         cflags.push(" ");
-        cflags.push(&pkg.finger_print.static_libs);
+        cflags.push(pkg.finger_print.static_libs.join(" "));
     }
 
     std::env::set_var("INLINE_C_RS_CFLAGS", cflags);
@@ -1408,21 +1442,24 @@ mod tests {
         let target_msvc = target::Target::new(Some("x86_64-pc-windows-msvc"), false).unwrap();
         let target_mingw = target::Target::new(Some("x86_64-pc-windows-gnu"), false).unwrap();
 
-        assert_eq!(static_libraries(libs_osx, &target_osx), "-lSystem -lc -lm");
         assert_eq!(
-            static_libraries(libs_linux, &target_linux),
+            static_libraries(libs_osx, &target_osx).join(" "),
+            "-lSystem -lc -lm"
+        );
+        assert_eq!(
+            static_libraries(libs_linux, &target_linux).join(" "),
             "-lgcc_s -lutil -lrt -lpthread -lm -ldl -lc"
         );
         assert_eq!(
-            static_libraries(libs_hurd, &target_hurd),
+            static_libraries(libs_hurd, &target_hurd).join(" "),
             "-lgcc_s -lutil -lrt -lpthread -lm -ldl -lc"
         );
         assert_eq!(
-            static_libraries(libs_msvc, &target_msvc),
+            static_libraries(libs_msvc, &target_msvc).join(" "),
             "-lkernel32 -ladvapi32 -lntdll -luserenv -lws2_32 -lmsvcrt"
         );
         assert_eq!(
-            static_libraries(libs_mingw, &target_mingw),
+            static_libraries(libs_mingw, &target_mingw).join(" "),
             "-lkernel32 -ladvapi32 -lntdll -luserenv -lws2_32"
         );
     }

src/pkg_config_gen.rs

@@ -2,7 +2,12 @@
 
 use crate::build::CApiConfig;
 use crate::install::InstallPaths;
-use std::path::{Component, Path, PathBuf};
+use log::warn;
+use pkg_config;
+use std::{
+    collections::HashSet,
+    path::{Component, Path, PathBuf},
+};
 
 fn canonicalize<P: AsRef<Path>>(path: P) -> String {
     let mut stack = Vec::with_capacity(16);
@@ -57,6 +62,12 @@ fn canonicalize<P: AsRef<Path>>(path: P) -> String {
     }
 }
 
+#[derive(Debug, Clone)]
+struct PkgConfigDedupInformation {
+    requires: Vec<pkg_config::Library>,
+    requires_private: Vec<pkg_config::Library>,
+}
+
 #[derive(Debug, Clone)]
 pub struct PkgConfig {
     prefix: PathBuf,
@@ -77,6 +88,8 @@ pub struct PkgConfig {
     cflags: Vec<String>,
 
     conflicts: Vec<String>,
+
+    dedup: PkgConfigDedupInformation,
 }
 
 impl PkgConfig {
@@ -99,19 +112,61 @@ impl PkgConfig {
             Some(reqs) => reqs.split(',').map(|s| s.trim().to_string()).collect(),
             _ => Vec::new(),
         };
+
+        let requires_libs = {
+            let cfg = {
+                let mut c = pkg_config::Config::new();
+                // This is not sinkholed by cargo-c
+                c.env_metadata(false);
+                c.cargo_metadata(false);
+
+                c
+            };
+
+            // TODO: log which probe fails
+            requires
+                .iter()
+                .flat_map(|req| {
+                    let c = cfg.probe(req);
+                    if c.is_err() {
+                        warn!("WARNING: library not found: {}", c.as_ref().err().unwrap())
+                    }
+                    c
+                })
+                .collect::<Vec<_>>()
+        };
+
         let requires_private = match &capi_config.pkg_config.requires_private {
             Some(reqs) => reqs.split(',').map(|s| s.trim().to_string()).collect(),
             _ => Vec::new(),
         };
 
+        let requires_private_libs = {
+            let cfg = {
+                let mut c = pkg_config::Config::new();
+                c.statik(true);
+                // This is not sinkholed by cargo-c
+                c.env_metadata(false);
+                c.cargo_metadata(false);
+
+                c
+            };
+
+            // TODO: log which probe fails
+            requires_private
+                .iter()
+                .flat_map(|req| cfg.probe(req))
+                .collect::<Vec<_>>()
+        };
+
         let mut libdir = PathBuf::new();
         libdir.push("${libdir}");
         if let Some(subdir) = &capi_config.library.install_subdir {
             libdir.push(subdir);
         }
 
         let libs = vec![
-            format!("-L{}", libdir.display()),
+            format!("-L{}", canonicalize(libdir.display().to_string())),
             format!("-l{}", capi_config.library.name),
         ];
 
@@ -146,6 +201,11 @@ impl PkgConfig {
             cflags: vec![cflags],
 
             conflicts: Vec::new(),
+
+            dedup: PkgConfigDedupInformation {
+                requires: requires_libs,
+                requires_private: requires_private_libs,
+            },
         }
     }
 
@@ -236,7 +296,81 @@ impl PkgConfig {
         self.render_help(String::with_capacity(1024)).unwrap()
     }
 
+    fn get_libs_cflags(arg: &[pkg_config::Library]) -> (HashSet<String>, HashSet<String>) {
+        let mut libs: HashSet<String> = HashSet::new();
+        let mut cflags: HashSet<String> = HashSet::new();
+
+        for lib in arg.iter() {
+            for lib in lib.include_paths.iter() {
+                libs.insert(format!("-I{}", lib.to_string_lossy()));
+            }
+            for lib in lib.link_files.iter() {
+                libs.insert(lib.to_string_lossy().to_string());
+            }
+            for lib in lib.libs.iter() {
+                libs.insert(format!("-l{}", lib));
+            }
+            for lib in lib.link_paths.iter() {
+                libs.insert(format!("-L{}", lib.to_string_lossy()));
+            }
+            for lib in lib.frameworks.iter() {
+                libs.insert(format!("-framework {}", lib));
+            }
+            for lib in lib.framework_paths.iter() {
+                libs.insert(format!("-F{}", lib.to_string_lossy()));
+            }
+            for lib in lib.defines.iter() {
+                let v = match lib.1 {
+                    Some(v) => format!("-D{}={}", lib.0, v),
+                    None => format!("D{}", lib.0),
+                };
+                libs.insert(v);
+            }
+            for lib in lib.ld_args.iter() {
+                cflags.insert(format!("-Wl,{}", lib.join(",")));
+            }
+        }
+
+        (libs, cflags)
+    }
+
+    fn dedup_flags(known_flags: &HashSet<String>, flags: &[String]) -> String {
+        flags
+            .iter()
+            .filter(|&lib| {
+                !known_flags.contains(lib) || lib.starts_with("-Wl,") || lib.starts_with("/LINK")
+            })
+            .map(|lib| lib.as_str())
+            .collect::<Vec<_>>()
+            .join(" ")
+    }
+
     fn render_help<W: core::fmt::Write>(&self, mut w: W) -> Result<W, core::fmt::Error> {
+        // Dedup
+        // What libs are already known here?
+        let (dedup_cflags, dedup_libs, dedup_libs_private) = {
+            let (known_libs, known_cflags) = PkgConfig::get_libs_cflags(&self.dedup.requires);
+
+            let cflags = PkgConfig::dedup_flags(&known_cflags, &self.cflags);
+            let libs = PkgConfig::dedup_flags(&known_libs, &self.libs);
+
+            // FIXME: There's no Cflags.private?
+            let (mut known_libs_private, _) =
+                PkgConfig::get_libs_cflags(&self.dedup.requires_private);
+            // Need to be deduplicated against libs too!
+            for i in &self.libs {
+                known_libs_private.insert(i.clone());
+            }
+
+            let libs_private = PkgConfig::dedup_flags(&known_libs_private, &self.libs_private);
+
+            println!(
+                "{:?} => {:?} => {:?}",
+                known_libs_private, self.libs_private, libs_private
+            );
+            (cflags, libs, libs_private)
+        };
+
         writeln!(w, "prefix={}", canonicalize(&self.prefix))?;
         writeln!(w, "exec_prefix={}", canonicalize(&self.exec_prefix))?;
         writeln!(w, "libdir={}", canonicalize(&self.libdir))?;
@@ -247,11 +381,11 @@ impl PkgConfig {
         writeln!(w, "Name: {}", self.name)?;
         writeln!(w, "Description: {}", self.description.replace('\n', " "))?; // avoid endlines
         writeln!(w, "Version: {}", self.version)?;
-        writeln!(w, "Libs: {}", self.libs.join(" "))?;
-        writeln!(w, "Cflags: {}", self.cflags.join(" "))?;
+        writeln!(w, "Libs: {}", dedup_libs)?;
+        writeln!(w, "Cflags: {}", dedup_cflags)?;
 
         if !self.libs_private.is_empty() {
-            writeln!(w, "Libs.private: {}", self.libs_private.join(" "))?;
+            writeln!(w, "Libs.private: {}", dedup_libs_private)?;
         }
 
         if !self.requires.is_empty() {