From 493ab683f2a7bdafc78d4709925b1aa50e8750a0 Mon Sep 17 00:00:00 2001 From: Euan Harris Date: Thu, 18 Apr 2024 09:18:38 +0100 Subject: [PATCH] pkgconfig: Apply PKG_CONFIG_SYSROOTDIR when generating paths SwiftPM's pkg-config implementation sets the `pc_sysrootdir` variable, but most `.pc` files do not use this variable directly. Instead, they rely on the pkg-config tool rewriting the generated paths to include the sysroot prefix when necessary. SwiftPM does not do this, so it does not generate the correct compiler flags to use libraries from a sysroot. This problem was reported in issue https://github.com/apple/swift-package-manager/issues/7409 There are two major pkg-config implementations which handle sysroot differently: * `pkg-config` (the original https://pkg-config.freedesktop.org implementation) prepends sysroot after variable expansion, when it creates the compiler flag lists * `pkgconf` (the newer http://pkgconf.org implementation) prepends sysroot to variables when they are defined, so sysroot is included when they are expanded `pkg-config`'s method skips single character compiler flags, such as `-I` and `-L`, and has special cases for longer options. It does not handle spaces between the flags and their values properly, and prepends sysroot multiple times in some cases, such as when the .pc file uses the `sysroot_dir` variable directly or has been rewritten to hard-code the sysroot prefix. `pkgconf`'s method handles spaces correctly, although it also makes extra checks to ensure that sysroot is not applied more than once. In 2024 `pkg-config` is the more popular option according to Homebrew installation statistics, but the major Linux distributions have generally switched to `pkgconf`. We will use `pkgconf`'s method here as it seems more robust than `pkg-config`'s, and `pkgconf`'s greater popularity on Linux means libraries developed there may depend on the specific way it handles `.pc` files. SwiftPM will now apply the sysroot prefix to compiler flags, such as include (`-I`) and library (`-L`) search paths. This is a partial fix for https://github.com/apple/swift-package-manager/issues/7409. The sysroot prefix is only applied when the `PKG_CONFIG_SYSROOT_DIR` environment variable is set. A future commit could apply an appropriate sysroot automatically when the `--experimental-swift-sdk` flag is used. --- Sources/PackageLoading/PkgConfig.swift | 94 ++++++++++++++++++- .../PkgConfigParserTests.swift | 84 ++++++++++++++++- .../pkgconfigInputs/case_insensitive.pc | 3 + .../pkgconfigInputs/deps_variable.pc | 3 + .../pkgconfigInputs/double_sysroot.pc | 7 ++ .../pkgconfigInputs/dummy_dependency.pc | 3 + .../pkgconfigInputs/empty_cflags.pc | 3 + .../pkgconfigInputs/escaped_spaces.pc | 3 + .../pkgconfigInputs/failure_case.pc | 3 + .../pkgconfigInputs/not_double_sysroot.pc | 6 ++ .../pkgconfigInputs/quotes_failure.pc | 3 + 11 files changed, 206 insertions(+), 6 deletions(-) create mode 100644 Tests/PackageLoadingTests/pkgconfigInputs/double_sysroot.pc create mode 100644 Tests/PackageLoadingTests/pkgconfigInputs/not_double_sysroot.pc diff --git a/Sources/PackageLoading/PkgConfig.swift b/Sources/PackageLoading/PkgConfig.swift index 2fdb1d60788..8838be90135 100644 --- a/Sources/PackageLoading/PkgConfig.swift +++ b/Sources/PackageLoading/PkgConfig.swift @@ -47,6 +47,7 @@ public struct PkgConfig { name: String, additionalSearchPaths: [AbsolutePath]? = .none, brewPrefix: AbsolutePath? = .none, + sysrootDir: AbsolutePath? = .none, fileSystem: FileSystem, observabilityScope: ObservabilityScope ) throws { @@ -54,6 +55,7 @@ public struct PkgConfig { name: name, additionalSearchPaths: additionalSearchPaths ?? [], brewPrefix: brewPrefix, + sysrootDir: sysrootDir, loadingContext: LoadingContext(), fileSystem: fileSystem, observabilityScope: observabilityScope @@ -64,6 +66,7 @@ public struct PkgConfig { name: String, additionalSearchPaths: [AbsolutePath], brewPrefix: AbsolutePath?, + sysrootDir: AbsolutePath?, loadingContext: LoadingContext, fileSystem: FileSystem, observabilityScope: ObservabilityScope @@ -85,7 +88,7 @@ public struct PkgConfig { ) } - var parser = try PkgConfigParser(pcFile: pcFile, fileSystem: fileSystem) + var parser = try PkgConfigParser(pcFile: pcFile, fileSystem: fileSystem, sysrootDir: ProcessEnv.block["PKG_CONFIG_SYSROOT_DIR"]) try parser.parse() func getFlags(from dependencies: [String]) throws -> (cFlags: [String], libs: [String]) { @@ -103,6 +106,7 @@ public struct PkgConfig { name: dep, additionalSearchPaths: additionalSearchPaths, brewPrefix: brewPrefix, + sysrootDir: sysrootDir, loadingContext: loadingContext, fileSystem: fileSystem, observabilityScope: observabilityScope @@ -162,13 +166,93 @@ internal struct PkgConfigParser { public private(set) var privateDependencies = [String]() public private(set) var cFlags = [String]() public private(set) var libs = [String]() + public private(set) var sysrootDir: String? - public init(pcFile: AbsolutePath, fileSystem: FileSystem) throws { + public init(pcFile: AbsolutePath, fileSystem: FileSystem, sysrootDir: String?) throws { guard fileSystem.isFile(pcFile) else { throw StringError("invalid pcfile \(pcFile)") } self.pcFile = pcFile self.fileSystem = fileSystem + self.sysrootDir = sysrootDir + } + + // Compress repeated path separators to one. + private func compressPathSeparators(_ value: String) -> String { + let components = value.components(separatedBy: "/").filter { !$0.isEmpty }.joined(separator: "/") + if value.hasPrefix("/") { + return "/" + components + } else { + return components + } + } + + // Trim duplicate sysroot prefixes, matching the approach of pkgconf + private func trimDuplicateSysroot(_ value: String) -> String { + // If sysroot has been applied more than once, remove the first instance. + // pkgconf makes this check after variable expansion to handle rare .pc + // files which expand ${pc_sysrootdir} directly: + // https://github.com/pkgconf/pkgconf/issues/123 + // + // For example: + // /sysroot/sysroot/remainder -> /sysroot/remainder + // + // However, pkgconf's algorithm searches for an additional sysrootdir anywhere in + // the string after the initial prefix, rather than looking for two sysrootdir prefixes + // directly next to each other: + // + // /sysroot/filler/sysroot/remainder -> /filler/sysroot/remainder + // + // It might seem more logical not to strip sysroot in this case, as it is not a double + // prefix, but for compatibility trimDuplicateSysroot is faithful to pkgconf's approach + // in the functions `pkgconf_tuple_parse` and `should_rewrite_sysroot`. + + // Only trim if sysroot is defined with a meaningful value + guard let sysrootDir, sysrootDir != "/" else { + return value + } + + // Only trim absolute paths starting with sysroot + guard value.hasPrefix("/"), value.hasPrefix(sysrootDir) else { + return value + } + + // If sysroot appears multiple times, trim the prefix + // N.B. sysroot can appear anywhere in the remainder + // of the value, mirroring pkgconf's logic + let pathSuffix = value.dropFirst(sysrootDir.count) + if pathSuffix.contains(sysrootDir) { + return String(pathSuffix) + } else { + return value + } + } + + // Apply sysroot to generated paths, matching the approach of pkgconf + private func applySysroot(_ value: String) -> String { + // The two main pkg-config implementations handle sysroot differently: + // + // `pkg-config` (freedesktop.org) prepends sysroot after variable expansion, when in creates the compiler flag lists + // `pkgconf` prepends sysroot to variables when they are defined, so sysroot is included when they are expanded + // + // pkg-config's method skips single character compiler flags, such as '-I' and '-L', and has special cases for longer options. + // It does not handle spaces between the flags and their values properly, and prepends sysroot multiple times in some cases, + // such as when the .pc file uses the sysroot_dir variable directly or has been rewritten to hard-code the sysroot prefix. + // + // pkgconf's method handles spaces correctly, although it also requires extra checks to ensure that sysroot is not applied + // more than once. + // + // In 2024 pkg-config is the more popular option according to Homebrew installation statistics, but the major Linux distributions + // have generally switched to pkgconf. + // + // We will use pkgconf's method here as it seems more robust than pkg-config's, and pkgconf's greater popularity on Linux + // means libraries developed there may depend on the specific way it handles .pc files. + + if value.hasPrefix("/"), let sysrootDir, !value.hasPrefix(sysrootDir) { + return compressPathSeparators(trimDuplicateSysroot(sysrootDir + value)) + } else { + return compressPathSeparators(trimDuplicateSysroot(value)) + } } public mutating func parse() throws { @@ -183,7 +267,9 @@ internal struct PkgConfigParser { variables["pcfiledir"] = pcFile.parentDirectory.pathString // Add pc_sysrootdir variable. This is the path of the sysroot directory for pc files. - variables["pc_sysrootdir"] = ProcessEnv.block["PKG_CONFIG_SYSROOT_DIR"] ?? AbsolutePath.root.pathString + // pkgconf does not define pc_sysrootdir if the path of the .pc file is outside sysrootdir. + // SwiftPM does not currently make that check. + variables["pc_sysrootdir"] = sysrootDir ?? AbsolutePath.root.pathString let fileContents: String = try fileSystem.readFileContents(pcFile) for line in fileContents.components(separatedBy: "\n") { @@ -199,7 +285,7 @@ internal struct PkgConfigParser { // Found a variable. let (name, maybeValue) = line.spm_split(around: "=") let value = maybeValue?.spm_chuzzle() ?? "" - variables[name.spm_chuzzle() ?? ""] = try resolveVariables(value) + variables[name.spm_chuzzle() ?? ""] = try applySysroot(resolveVariables(value)) } else { // Unexpected thing in the pc file, abort. throw PkgConfigError.parsingError("Unexpected line: \(line) in \(pcFile)") diff --git a/Tests/PackageLoadingTests/PkgConfigParserTests.swift b/Tests/PackageLoadingTests/PkgConfigParserTests.swift index 009f1328ff4..110bf941ff7 100644 --- a/Tests/PackageLoadingTests/PkgConfigParserTests.swift +++ b/Tests/PackageLoadingTests/PkgConfigParserTests.swift @@ -244,12 +244,92 @@ final class PkgConfigParserTests: XCTestCase { } } + func testSysrootDir() throws { + // sysroot should be prepended to all path variables, and should therefore appear in cflags and libs. + try loadPCFile("gtk+-3.0.pc", sysrootDir: "/opt/sysroot/somewhere") { parser in + XCTAssertEqual(parser.variables, [ + "libdir": "/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9/lib", + "gtk_host": "x86_64-apple-darwin15.3.0", + "includedir": "/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9/include", + "prefix": "/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9", + "gtk_binary_version": "3.0.0", + "exec_prefix": "/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9", + "targets": "quartz", + "pcfiledir": parser.pcFile.parentDirectory.pathString, + "pc_sysrootdir": "/opt/sysroot/somewhere" + ]) + XCTAssertEqual(parser.dependencies, ["gdk-3.0", "atk", "cairo", "cairo-gobject", "gdk-pixbuf-2.0", "gio-2.0"]) + XCTAssertEqual(parser.privateDependencies, ["atk", "epoxy", "gio-unix-2.0"]) + XCTAssertEqual(parser.cFlags, ["-I/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9/include/gtk-3.0"]) + XCTAssertEqual(parser.libs, ["-L/opt/sysroot/somewhere/usr/local/Cellar/gtk+3/3.18.9/lib", "-lgtk-3"]) + } + + // sysroot should be not be prepended if it is already a prefix + // - pkgconf makes this check, but pkg-config does not + // - If the .pc file lies outside sysrootDir, pkgconf sets pc_sysrootdir to the empty string + // https://github.com/pkgconf/pkgconf/issues/213 + // SwiftPM does not currently implement this special case. + try loadPCFile("gtk+-3.0.pc", sysrootDir: "/usr/local/Cellar") { parser in + XCTAssertEqual(parser.variables, [ + "libdir": "/usr/local/Cellar/gtk+3/3.18.9/lib", + "gtk_host": "x86_64-apple-darwin15.3.0", + "includedir": "/usr/local/Cellar/gtk+3/3.18.9/include", + "prefix": "/usr/local/Cellar/gtk+3/3.18.9", + "gtk_binary_version": "3.0.0", + "exec_prefix": "/usr/local/Cellar/gtk+3/3.18.9", + "targets": "quartz", + "pcfiledir": parser.pcFile.parentDirectory.pathString, + "pc_sysrootdir": "/usr/local/Cellar" + ]) + XCTAssertEqual(parser.dependencies, ["gdk-3.0", "atk", "cairo", "cairo-gobject", "gdk-pixbuf-2.0", "gio-2.0"]) + XCTAssertEqual(parser.privateDependencies, ["atk", "epoxy", "gio-unix-2.0"]) + XCTAssertEqual(parser.cFlags, ["-I/usr/local/Cellar/gtk+3/3.18.9/include/gtk-3.0"]) + XCTAssertEqual(parser.libs, ["-L/usr/local/Cellar/gtk+3/3.18.9/lib", "-lgtk-3"]) + } + + // sysroot should be not be double-prepended if it is used explicitly by the .pc file + // - pkgconf makes this check, but pkg-config does not + try loadPCFile("double_sysroot.pc", sysrootDir: "/sysroot") { parser in + XCTAssertEqual(parser.variables, [ + "prefix": "/sysroot/usr", + "datarootdir": "/sysroot/usr/share", + "pkgdatadir": "/sysroot/usr/share/pkgdata", + "pcfiledir": parser.pcFile.parentDirectory.pathString, + "pc_sysrootdir": "/sysroot" + ]) + } + + // pkgconfig strips a leading sysroot prefix if sysroot appears anywhere else in the + // expanded variable. SwiftPM's implementation is faithful to pkgconfig, even + // thought it might seem more logical not to strip the prefix in this case. + try loadPCFile("not_double_sysroot.pc", sysrootDir: "/sysroot") { parser in + XCTAssertEqual(parser.variables, [ + "prefix": "/sysroot/usr", + "datarootdir": "/sysroot/usr/share", + "pkgdatadir": "/filler/sysroot/usr/share/pkgdata", + "pcfiledir": parser.pcFile.parentDirectory.pathString, + "pc_sysrootdir": "/sysroot" + ]) + } + + // pkgconfig does not strip sysroot if it is a relative path + try loadPCFile("double_sysroot.pc", sysrootDir: "sysroot") { parser in + XCTAssertEqual(parser.variables, [ + "prefix": "sysroot/usr", + "datarootdir": "sysroot/usr/share", + "pkgdatadir": "sysroot/sysroot/usr/share/pkgdata", + "pcfiledir": parser.pcFile.parentDirectory.pathString, + "pc_sysrootdir": "sysroot" + ]) + } + } + private func pcFilePath(_ inputName: String) -> AbsolutePath { return AbsolutePath(#file).parentDirectory.appending(components: "pkgconfigInputs", inputName) } - private func loadPCFile(_ inputName: String, body: ((PkgConfigParser) -> Void)? = nil) throws { - var parser = try PkgConfigParser(pcFile: pcFilePath(inputName), fileSystem: localFileSystem) + private func loadPCFile(_ inputName: String, sysrootDir: String? = nil, body: ((PkgConfigParser) -> Void)? = nil) throws { + var parser = try PkgConfigParser(pcFile: pcFilePath(inputName), fileSystem: localFileSystem, sysrootDir: sysrootDir) try parser.parse() body?(parser) } diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/case_insensitive.pc b/Tests/PackageLoadingTests/pkgconfigInputs/case_insensitive.pc index b65a14c7d16..83017682ea9 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/case_insensitive.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/case_insensitive.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} #some comment +Name: case_insensitive +Version: 1 +Description: Demonstrate that pkgconfig keys are case-insensitive # upstream pkg-config parser allows Cflags & CFlags as key CFlags: -I/usr/local/include diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/deps_variable.pc b/Tests/PackageLoadingTests/pkgconfigInputs/deps_variable.pc index 225d86d1369..de06c50db85 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/deps_variable.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/deps_variable.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} my_dep=atk #some comment +Name: deps_variable +Version: 1 +Description: Demonstrate use of a locally-defined variable Requires: gdk-3.0 >= 1.0.0 ${my_dep} Libs: -L${prefix} -lgtk-3 diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/double_sysroot.pc b/Tests/PackageLoadingTests/pkgconfigInputs/double_sysroot.pc new file mode 100644 index 00000000000..be7abee01ce --- /dev/null +++ b/Tests/PackageLoadingTests/pkgconfigInputs/double_sysroot.pc @@ -0,0 +1,7 @@ +prefix=/usr +datarootdir=${prefix}/share +pkgdatadir=${pc_sysrootdir}/${datarootdir}/pkgdata + +Name: double_sysroot +Description: Demonstrate double-prefixing of pc_sysrootdir (https://github.com/pkgconf/pkgconf/issues/123) +Version: 1 diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/dummy_dependency.pc b/Tests/PackageLoadingTests/pkgconfigInputs/dummy_dependency.pc index c4ae253c81a..e3a797923da 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/dummy_dependency.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/dummy_dependency.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} #some comment +Name: dummy_dependency +Version: 1 +Description: Demonstrate a blank dependency entry Requires: pango, , fontconfig >= 2.13.0 Libs:-L${prefix} -lpangoft2-1.0 diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/empty_cflags.pc b/Tests/PackageLoadingTests/pkgconfigInputs/empty_cflags.pc index 0f7f8e6c1a2..a7d2ffa6bae 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/empty_cflags.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/empty_cflags.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} #some comment +Name: empty_cflags +Version: 1 +Description: Demonstrate an empty cflags list Requires: gdk-3.0 atk Libs:-L${prefix} -lgtk-3 diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/escaped_spaces.pc b/Tests/PackageLoadingTests/pkgconfigInputs/escaped_spaces.pc index f00283e070f..e0e77e73774 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/escaped_spaces.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/escaped_spaces.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} my_dep=atk #some comment +Name: escaped_spaces +Version: 1 +Description: Demonstrate use of escape characters in flag values Requires: gdk-3.0 >= 1.0.0 ${my_dep} Libs: -L"${prefix}" -l"gtk 3" -wantareal\\here -one\\ -two diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/failure_case.pc b/Tests/PackageLoadingTests/pkgconfigInputs/failure_case.pc index e81dc924502..f1981c1b3d4 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/failure_case.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/failure_case.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} #some comment +Name: failure_case +Version: 1 +Description: Demonstrate failure caused by use of an undefined variable Requires: gdk-3.0 >= 1.0.0 Libs: -L${prefix} -lgtk-3 ${my_dep} diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/not_double_sysroot.pc b/Tests/PackageLoadingTests/pkgconfigInputs/not_double_sysroot.pc new file mode 100644 index 00000000000..c537069beb6 --- /dev/null +++ b/Tests/PackageLoadingTests/pkgconfigInputs/not_double_sysroot.pc @@ -0,0 +1,6 @@ +prefix=/usr +datarootdir=${prefix}/share +pkgdatadir=${pc_sysrootdir}/filler/${datarootdir}/pkgdata + +Name: double_sysroot +Description: Demonstrate pc_sysrootdir appearing elsewhere in a path - this is not a double prefix and should not be removed \ No newline at end of file diff --git a/Tests/PackageLoadingTests/pkgconfigInputs/quotes_failure.pc b/Tests/PackageLoadingTests/pkgconfigInputs/quotes_failure.pc index 3006c2fbd85..07ec4503a76 100644 --- a/Tests/PackageLoadingTests/pkgconfigInputs/quotes_failure.pc +++ b/Tests/PackageLoadingTests/pkgconfigInputs/quotes_failure.pc @@ -2,6 +2,9 @@ prefix=/usr/local/bin exec_prefix=${prefix} my_dep=atk #some comment +Name: quotes_failure +Version: 1 +Description: Demonstrate failure due to unbalanced quotes Requires: gdk-3.0 >= 1.0.0 ${my_dep} Libs: -L"${prefix}" -l"gt"k3" -wantareal\\here -one\\ -two