From e109aed960513088804afb9ed9a8927646d35902 Mon Sep 17 00:00:00 2001 From: "L. E. Segovia" Date: Sat, 14 Jun 2025 14:46:48 -0300 Subject: [PATCH] Implement pkg-config flag deduplication Should reduce a lot of the noise in Cflags and Libraries. See #455 --- Cargo.toml | 1 + src/build.rs | 75 ++++++++++++++++------ src/pkg_config_gen.rs | 140 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 192 insertions(+), 24 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 0743e34..032aae6 100644 --- a/Cargo.toml +++ b/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 = [] diff --git a/src/build.rs b/src/build.rs index 6225f8c..df42c9e 100644 --- a/src/build.rs +++ b/src/build.rs @@ -235,14 +235,14 @@ struct FingerPrint { root_output: PathBuf, build_targets: BuildTargets, install_paths: InstallPaths, - static_libs: String, + static_libs: Vec, hasher: DefaultHasher, } #[derive(serde::Serialize, serde::Deserialize)] struct Cache { hash: String, - static_libs: String, + static_libs: Vec, } 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 { + let libs = link_line .trim() .split(' ') .filter(|s| { @@ -1081,7 +1081,6 @@ 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")); @@ -1089,8 +1088,36 @@ fn static_libraries(link_line: &str, rustc_target: &target::Target) -> String { lib.trim().to_string() }) .filter(|s| !s.is_empty()) - .collect::>() - .join(" ") + .collect::>(); + + let mut final_libs: Vec = 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" ); } diff --git a/src/pkg_config_gen.rs b/src/pkg_config_gen.rs index f913ab1..48ce920 100644 --- a/src/pkg_config_gen.rs +++ b/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>(path: P) -> String { let mut stack = Vec::with_capacity(16); @@ -57,6 +62,12 @@ fn canonicalize>(path: P) -> String { } } +#[derive(Debug, Clone)] +struct PkgConfigDedupInformation { + requires: Vec, + requires_private: Vec, +} + #[derive(Debug, Clone)] pub struct PkgConfig { prefix: PathBuf, @@ -77,6 +88,8 @@ pub struct PkgConfig { cflags: Vec, conflicts: Vec, + + dedup: PkgConfigDedupInformation, } impl PkgConfig { @@ -99,11 +112,53 @@ 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::>() + }; + 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::>() + }; + let mut libdir = PathBuf::new(); libdir.push("${libdir}"); if let Some(subdir) = &capi_config.library.install_subdir { @@ -111,7 +166,7 @@ impl PkgConfig { } 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,77 @@ impl PkgConfig { self.render_help(String::with_capacity(1024)).unwrap() } + fn get_libs_cflags(arg: &[pkg_config::Library]) -> (HashSet, HashSet) { + let mut libs: HashSet = HashSet::new(); + let mut cflags: HashSet = 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, 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::>() + .join(" ") + } + fn render_help(&self, mut w: W) -> Result { + // 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); + + (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 +377,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() {