Skip to content

pkgconfig System Module Search Paths #257

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions Sources/Build/Buildable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,30 @@ extension Module: Buildable {
}
}

extension SwiftModule {
func pkgConfigArgs() throws -> [String] {
return try recursiveDependencies.flatMap { module -> [String] in
guard case let module as CModule = module, let pkgConfigName = module.pkgConfig else {
return []
}

do {
var pkgConfig = try PkgConfig(name: pkgConfigName)
try pkgConfig.load()
return pkgConfig.cFlags.map{["-Xcc", $0]}.flatten() + pkgConfig.libs
}
catch PkgConfigError.CouldNotFindConfigFile {
if let providers = module.providers,
provider = SystemPackageProvider.providerForCurrentPlatform(providers: providers) {
print("note: you may be able to install \(pkgConfigName) using your system-packager:\n")
print(provider.installText)
}
}
return []
}
}
}

extension Product: Buildable {
var isTest: Bool {
if case .Test = type {
Expand Down
4 changes: 2 additions & 2 deletions Sources/Build/Command.compile(SwiftModule).swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ import Utility
extension Command {
static func compile(swiftModule module: SwiftModule, configuration conf: Configuration, prefix: String, otherArgs: [String], SWIFT_EXEC: String) throws -> (Command, [Command]) {

let otherArgs = otherArgs + module.XccFlags(prefix)

let otherArgs = otherArgs + module.XccFlags(prefix) + (try module.pkgConfigArgs())
func cmd(_ tool: ToolProtocol) -> Command {
return Command(node: module.targetName, tool: tool)
}
Expand Down
6 changes: 5 additions & 1 deletion Sources/Build/Command.link().swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ extension Command {
case .Executable:
args.append("-emit-executable")
}


for module in product.modules {
args += try module.pkgConfigArgs()
}

args += objects

if case .Library(.Static) = product.type {
Expand Down
161 changes: 161 additions & 0 deletions Sources/Build/PkgConfig.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/*
This source file is part of the Swift.org open source project

Copyright 2015 - 2016 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See http://swift.org/LICENSE.txt for license information
See http://swift.org/CONTRIBUTORS.txt for Swift project authors
*/

import Utility
import func POSIX.getenv

enum PkgConfigError: ErrorProtocol {
case CouldNotFindConfigFile
}

struct PkgConfig {
static let searchPaths = ["/usr/local/lib/pkgconfig",
"/usr/local/share/pkgconfig",
"/usr/lib/pkgconfig",
"/usr/share/pkgconfig",
// FIXME: These should only be searched for linux?
"/usr/lib/x86_64-linux-gnu/pkgconfig",
"/usr/local/lib/x86_64-linux-gnu/pkgconfig",
]

let name: String
let pcFile: String
var cFlags = [String]()
var libs = [String]()
private var parser: PkgConfigParser

init(name: String) throws {
self.name = name
self.pcFile = try PkgConfig.locatePCFile(name: name)
parser = PkgConfigParser(pcFile: pcFile)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a weird API, having a throwing initializer + not-yet-initialized mutable cFlags member variables. Why not just move the load() into the initializer? Also, I would recommend using private where possible non-private structs, I think it makes things more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in #265


static var envSearchPaths: [String] {
if let configPath = getenv("PKG_CONFIG_PATH") {
return configPath.characters.split(separator: ":").map(String.init)
}
return []
}

static func locatePCFile(name: String) throws -> String {
for path in (searchPaths + envSearchPaths) {
let pcFile = Path.join(path, "\(name).pc")
if pcFile.isFile {
return pcFile
}
}
throw PkgConfigError.CouldNotFindConfigFile
}

mutating func load() throws {
cFlags = [String]()
libs = [String]()
try parser.parse()
if let cFlags = parser.cFlags {
// FIXME: handle spaces in paths.
self.cFlags += cFlags.characters.split(separator: " ").map(String.init)
}
if let libs = parser.libs {
// FIXME: handle spaces in paths.
self.libs += libs.characters.split(separator: " ").map(String.init)
}

if(parser.dependencies.isEmpty) {
return
}

for dep in parser.dependencies {
var pkg = try PkgConfig(name: dep)
try pkg.load()
self.cFlags += pkg.cFlags
self.libs += pkg.libs
}
}
}

private struct PkgConfigParser {
let pcFile: String
var variables = [String: String]()
var dependencies = [String]()
var cFlags: String?
var libs: String?

enum Token {
case CFlats(String)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This enum is unused...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed in #265

case Libs(String)
case Dependencies(String)
case Variable(name: String, value: String)
}

init(pcFile: String) {
self.pcFile = pcFile
}

mutating func parse() throws {
let file = File(path: self.pcFile)
for line in try file.enumerate() {
if !line.characters.contains(":") && line.characters.contains("=") {
let equalsIndex = line.characters.index(of: "=")!
let name = line[line.startIndex..<equalsIndex]
let value = line[equalsIndex.successor()..<line.endIndex]
variables[name] = resolveVariables(value)
} else if line.hasPrefix("Requires: ") {
dependencies = parseDependencies(value(line: line))
} else if line.hasPrefix("Libs: ") {
libs = resolveVariables(value(line: line)).chomp()
} else if line.hasPrefix("Cflags: ") {
cFlags = resolveVariables( value(line: line)).chomp()
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not very familiar with the format, but I would recommend trying to get an else clause in here that will error out if we see something unexpected. This prevents the code from silently failing to work properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not really an exhaustive list of possible cases in the file, there are some things for eg Name: Description: etc which we aren't interested in and nor want to parse, how to handle those ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a complete list available? If so, we could just check if it is in the known complete list and ignore if not there.

We can at least partition into the following three cases, right?

  1. Variable assignment
  2. Key-value binding (Name: Value)
  3. Comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think those are the only cases. I'll rework the parser in another PR which should address the parser related comments

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in fb5e87b

}
}

func parseDependencies(_ depString: String) -> [String] {
let exploded = depString.characters.split(separator: " ").map(String.init)
let operators = ["=", "<", ">", "<=", ">="]
var deps = [String]()
var skipNext = false
for depString in exploded {
if skipNext {
skipNext = false
continue
}
if operators.contains(depString) {
skipNext = true
} else {
deps.append(depString)
}
}
return deps
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method isn't very robust, it accepts a lot of things which (I think) are invalid ("a = = b"), and it would be good to have some comments here about what it is trying to do.

Also, IMHO here is a nicer way to write this kind of loop which makes it more readable:

    var it = exploded.makeIterator()
    while let arg = it.next() {
        // Check if we have found an operator.
        if operators.contains(arg) {
          // We should have a version number next.
          guard let arg = it.next() else {
             // ... error ...
             continue
          }
        } else {
          // ...
        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a survey of some .pc files on one of my systems, it also looks like pkg-config allows ',' as a separator here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 8d38e90


func resolveVariables(_ line: String) -> String {
func resolve(_ string: String) -> String {
var resolvedString = string
guard let dollar = resolvedString.characters.index(of: "$") else { return string }
guard let variableEndIndex = resolvedString.characters.index(of: "}") else {return string }
let variable = string[dollar.successor().successor()..<variableEndIndex]
let value = variables[variable]!
resolvedString = resolvedString[resolvedString.startIndex..<dollar] + value + resolvedString[variableEndIndex.successor()..<resolvedString.endIndex]
return resolvedString
}
var resolved = line
while resolved.characters.contains("$") {
resolved = resolve(resolved)
}
return resolved
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, need comments about what this is trying to do. Other notes:

  • This code should be more strict, it isn't validating the syntax (e.g., it accepts "$(foo}").
  • This should build up the result in pieces by parsing left to right, rather than repeatedly rescanning the string. Right now it is O(n^2) when it should be O(n).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in a82dcba


func value(line: String) -> String {
guard let colonIndex = line.characters.index(of: ":") else {
return ""
}
return line[colonIndex.successor().successor()..<line.endIndex]
}
}
38 changes: 38 additions & 0 deletions Sources/Build/misc.swift
Original file line number Diff line number Diff line change
Expand Up @@ -173,3 +173,41 @@ extension Product {
return ((), plist: s)
}
}

extension SystemPackageProvider {

var installText: String {
switch self {
case .Brew(let name):
return " brew install \(name)\n"
case .Apt(let name):
return " apt-get install \(name)\n"
}
}

static func providerForCurrentPlatform(providers: [SystemPackageProvider]) -> SystemPackageProvider? {
guard let uname = try? popen(["uname"]).chomp().lowercased() else { return nil }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to factor out the information about the target platform somewhere else (in particular, just using a popen(["uname"]) here to infer the platform is gross).

Also, I think it probably make more sense to just have SystemPackageProvider have a method which determines whether or not it is appropriate for the build environment, and then have the loop here be simply:

for provider in providers {
  if provider.isAvailable {
    return provider
  }
}
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated in #265

switch uname {
case "darwin":
for provider in providers {
if case .Brew = provider {
return provider
}
}
case "linux":
if "/etc/debian_version".isFile {
for provider in providers {
if case .Apt = provider {
return provider
}
}
}
break

default:
return nil
}
return nil
}
}

33 changes: 31 additions & 2 deletions Sources/ManifestParser/fromTOML().swift
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,11 @@ extension PackageDescription.Package {
if case .String(let value)? = table.items["name"] {
name = value
}

var pkgConfig: String? = nil
if case .String(let value)? = table.items["pkgConfig"] {
pkgConfig = value
}

// Parse the targets.
var targets: [PackageDescription.Target] = []
Expand All @@ -30,7 +35,15 @@ extension PackageDescription.Package {
targets.append(PackageDescription.Target.fromTOML(item))
}
}


var providers: [PackageDescription.SystemPackageProvider]? = nil
if case .Array(let array)? = table.items["providers"] {
providers = []
for item in array.items {
providers?.append(PackageDescription.SystemPackageProvider.fromTOML(item))
}
}

// Parse the dependencies.
var dependencies: [PackageDescription.Package.Dependency] = []
if case .Array(let array)? = table.items["dependencies"] {
Expand All @@ -56,7 +69,7 @@ extension PackageDescription.Package {
}
}

return PackageDescription.Package(name: name, targets: targets, dependencies: dependencies, testDependencies: testDependencies, exclude: exclude)
return PackageDescription.Package(name: name, pkgConfig: pkgConfig, providers: providers, targets: targets, dependencies: dependencies, testDependencies: testDependencies, exclude: exclude)
}
}

Expand Down Expand Up @@ -85,6 +98,22 @@ extension PackageDescription.Package.Dependency {
}
}

extension PackageDescription.SystemPackageProvider {
private static func fromTOML(_ item: TOMLItem) -> PackageDescription.SystemPackageProvider {
guard case .Table(let table) = item else { fatalError("unexpected item") }
guard case .String(let name)? = table.items["name"] else { fatalError("missing name") }
guard case .String(let value)? = table.items["value"] else { fatalError("missing value") }
switch name {
case "Brew":
return .Brew(value)
case "Apt":
return .Apt(value)
default:
fatalError("unexpected string")
}
}
}

extension PackageDescription.Target {
private static func fromTOML(_ item: TOMLItem) -> PackageDescription.Target {
// This is a private API, currently, so we do not currently try and
Expand Down
Loading