Skip to content

Auto generate module map file for C libs #219

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 6 commits into from
Apr 6, 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
5 changes: 0 additions & 5 deletions Fixtures/ClangModules/CLibraryFlat/include/module.modulemap

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

7 changes: 7 additions & 0 deletions Fixtures/ClangModules/ModuleMapGenerationCases/Package.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import PackageDescription

let package = Package(
name: "ModuleMapGenerationCases",
targets: [
Target(name: "Baz", dependencies: ["FlatInclude", "UmbrellaHeader", "UmbellaModuleNameInclude", "UmbrellaHeaderFlat"])]
)
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import UmbellaModuleNameInclude
import FlatInclude
import UmbrellaHeader
import UmbrellaHeaderFlat

let _ = foo()
let _ = bar()
let _ = jaz()
let _ = umbrellaHeaderFlat()
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "include/FlatIncludeHeader.h"

int bar() {
int a = 6;
int b = a;
a = b;
return a;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
int bar();
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@

int noDir() {
int a = 6;
int b = a;
a = b;
return a;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "include/UmbellaModuleNameInclude/Paz.h"

int jaz() {
int a = 6;
int b = a;
a = b;
return a;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
int jaz();
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "include/UmbrellaHeader/UmbrellaHeader.h"

int foo() {
int a = 5;
int b = a;
a = b;
return a;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
int foo();
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
#include "include/UmbrellaHeaderFlat.h"

int umbrellaHeaderFlat() {
int a = 5;
int b = a;
a = b;
return a;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
int umbrellaHeaderFlat();

This file was deleted.

This file was deleted.

This file was deleted.

This file was deleted.

28 changes: 26 additions & 2 deletions Sources/Build/Buildable.swift
Original file line number Diff line number Diff line change
Expand Up @@ -9,20 +9,44 @@
*/

import PackageType
import struct Utility.Path

protocol Buildable {
var targetName: String { get }
var isTest: Bool { get }
}

extension CModule {
///Returns the build directory path of a CModule
func buildDirectory(prefix: String) -> String {
return Path.join(prefix, "\(c99name).build")
}
}

extension Module: Buildable {
var isTest: Bool {
return self is TestModule
}

var Xcc: [String] {
func XccFlags(prefix: String) -> [String] {
return recursiveDependencies.flatMap { module -> [String] in
if let module = module as? CModule {
if let module = module as? ClangModule {
///For ClangModule we check if there is a user provided module map
///otherwise we return with path of generated one.
///We will fail before this is ever called if there is no module map.
///FIXME: The user provided modulemap should be copied to build dir
///but that requires copying the complete include dir because it'll
///mostly likely contain relative paths.
///FIXME: This is already computed when trying to generate modulemap
///in ClangModule's `generateModuleMap(inDir wd: String)`
///there shouldn't be need to redo this but is difficult in
///current architecture
if module.moduleMapPath.isFile {
return ["-Xcc", "-fmodule-map-file=\(module.moduleMapPath)"]
}
let genModuleMap = Path.join(module.buildDirectory(prefix), module.moduleMap)
return ["-Xcc", "-fmodule-map-file=\(genModuleMap)"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is obvious, but I am not seeing how this doesn't fail for non-library C language targets?

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think it is unfortunate that we are more or less recomputing state here which is independently computed by generateModuleMap. I don't think this is fixable in the current SwiftPM architecture, but it would be good to include a comment which points out that the behavior here is tied to that of the other function (and vice versa), so that if someone updates one they know to look at the other.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

they wont really be in dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding comment, but will push once CI returns

} else if let module = module as? CModule {
return ["-Xcc", "-fmodule-map-file=\(module.moduleMapPath)"]
} else {
return []
Expand Down
9 changes: 7 additions & 2 deletions Sources/Build/Command.compile(ClangModule).swift
Original file line number Diff line number Diff line change
Expand Up @@ -67,9 +67,14 @@ private extension Sources {
}

extension Command {
static func compile(clangModule module: ClangModule, externalModules: Set<Module>, configuration conf: Configuration, prefix: String, CC: String) -> ([Command], Command) {
static func compile(clangModule module: ClangModule, externalModules: Set<Module>, configuration conf: Configuration, prefix: String, CC: String) throws -> ([Command], Command) {

let wd = Path.join(prefix, "\(module.c99name).build")
let wd = module.buildDirectory(prefix)

if module.type == .Library {
try module.generateModuleMap(inDir: wd)
}

let mkdir = Command.createDirectory(wd)

///------------------------------ Compile -----------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/Command.compile(SwiftModule).swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ 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.Xcc
let otherArgs = otherArgs + module.XccFlags(prefix)

func cmd(tool: ToolProtocol) -> Command {
return Command(node: module.targetName, tool: tool)
Expand Down
2 changes: 1 addition & 1 deletion Sources/Build/Command.link().swift
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ extension Command {
let main = Path.join(testDirectory, "LinuxMain.swift")
args.append(main)
for module in product.modules {
args += module.Xcc
args += module.XccFlags(prefix)
}
args.append("-emit-executable")
args += ["-I", prefix]
Expand Down
12 changes: 1 addition & 11 deletions Sources/Build/describe().swift
Original file line number Diff line number Diff line change
Expand Up @@ -43,17 +43,7 @@ public func describe(prefix: String, _ conf: Configuration, _ modules: [Module],
targets.append(compile, for: module)

case let module as ClangModule:
//FIXME: Generate modulemaps if possible
//Since we're not generating modulemaps currently we'll just emit empty module map file
//if it not present
if module.type == .Library && !module.moduleMapPath.isFile {
try POSIX.mkdir(module.moduleMapPath.parentDirectory)
try fopen(module.moduleMapPath, mode: .Write) { fp in
try fputs("\n", fp)
}
}

let (compile, mkdir) = Command.compile(clangModule: module, externalModules: externalModules, configuration: conf, prefix: prefix, CC: CC)
let (compile, mkdir) = try Command.compile(clangModule: module, externalModules: externalModules, configuration: conf, prefix: prefix, CC: CC)
commands += compile
commands.append(mkdir)
targets.main.cmds += compile
Expand Down
101 changes: 100 additions & 1 deletion Sources/Build/misc.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,9 @@

import func POSIX.getenv
import func POSIX.popen
import func POSIX.mkdir
import func POSIX.fopen
import func libc.fclose
import PackageType
import Utility

Expand All @@ -29,8 +32,104 @@ func platformFrameworksPath() throws -> String {
}

extension CModule {

var moduleMap: String {
return "module.modulemap"
}

var moduleMapPath: String {
return Path.join(path, "module.modulemap")
return Path.join(path, moduleMap)
}
}

extension ClangModule {

public enum ModuleMapError: ErrorProtocol {
case UnsupportedIncludeLayoutForModule(String)
}

///FIXME: we recompute the generated modulemap's path
///when building swift modules in `XccFlags(prefix: String)`
///there shouldn't be need to redo this there but is difficult
///in current architecture
public func generateModuleMap(inDir wd: String) throws {

///Return if module map is already present
guard !moduleMapPath.isFile else {
Copy link
Member

Choose a reason for hiding this comment

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

What happens when a new header file is added and the modulemap file is checked in? Won't that not generate a new modulemap and thus not make the new header file available?

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 there are two problems currently...

  • module maps should probably be generated in build dir
  • incremental map generation

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, I did not realize we were not generating them to the build directory. We should be doing that if we are going to generate at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will move the generation to build dir

return
}

let includeDir = path

///Warn and return if no include directory
guard includeDir.isDirectory else {
print("warning: No include directory found, a library can not be imported without any public headers.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but we should start to get a proper diagnostics infrastructure instead of just embedding print() statements...

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed.

return
}

let walked = walk(includeDir, recursively: false).map{$0}

let files = walked.filter{$0.isFile && $0.hasSuffix(".h")}
let dirs = walked.filter{$0.isDirectory}

///We generate modulemap for a C module `foo` if:
///* `umbrella header "path/to/include/foo/foo.h"` exists and `foo` is the only
/// directory under include directory
///* `umbrella header "path/to/include/foo.h"` exists and include contains no other
/// directory
///* `umbrella "path/to/include"` in all other cases

let umbrellaHeaderFlat = Path.join(includeDir, "\(c99name).h")
if umbrellaHeaderFlat.isFile {
guard dirs.isEmpty else { throw ModuleMapError.UnsupportedIncludeLayoutForModule(name) }
try createModuleMap(inDir: wd, type: .Header(umbrellaHeaderFlat))
return
}
diagnoseInvalidUmbrellaHeader(includeDir)

let umbrellaHeader = Path.join(includeDir, c99name, "\(c99name).h")
if umbrellaHeader.isFile {
guard dirs.count == 1 && files.isEmpty else { throw ModuleMapError.UnsupportedIncludeLayoutForModule(name) }
try createModuleMap(inDir: wd, type: .Header(umbrellaHeader))
return
}
diagnoseInvalidUmbrellaHeader(Path.join(includeDir, c99name))

try createModuleMap(inDir: wd, type: .Directory(includeDir))
}

///warn user if in case module name and c99name are different and there a `name.h` umbrella header
private func diagnoseInvalidUmbrellaHeader(path: String) {
let umbrellaHeader = Path.join(path, "\(c99name).h")
let invalidUmbrellaHeader = Path.join(path, "\(name).h")
if c99name != name && invalidUmbrellaHeader.isFile {
print("warning: \(invalidUmbrellaHeader) should be renamed to \(umbrellaHeader) to be used as an umbrella header")
}
}

private enum UmbrellaType {
case Header(String)
case Directory(String)
}

private func createModuleMap(inDir wd: String, type: UmbrellaType) throws {
try POSIX.mkdir(wd)
let moduleMapFile = Path.join(wd, self.moduleMap)
let moduleMap = try fopen(moduleMapFile, mode: .Write)
defer { fclose(moduleMap) }

var output = "module \(c99name) {\n"
switch type {
case .Header(let header):
output += " umbrella header \"\(header)\"\n"
case .Directory(let path):
output += " umbrella \"\(path)\"\n"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would be easier to read if it simply computed the exact line to use instead of appending to a line starting with "umbrella ".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

output += " link \"\(c99name)\"\n"
output += " export *\n"
output += "}\n"

try fputs(output, moduleMap)
}
}

Expand Down
1 change: 0 additions & 1 deletion Sources/PackageType/Module.swift
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ public class ClangModule: CModule {

public init(name: String, sources: Sources) {
self.sources = sources
//TODO: generate module map using swiftpm if layout can support
super.init(name: name, path: sources.root + "/include")
}
}
Expand Down
14 changes: 13 additions & 1 deletion Tests/Functional/TestClangModules.swift
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class TestClangModulesTestCase: XCTestCase {
XCTAssertEqual(output, "hello 5")
}
}

func testCUsingCDep2() {
//The C dependency "Foo" has different layout
fixture(name: "DependencyResolution/External/CUsingCDep2") { prefix in
Expand All @@ -82,6 +82,17 @@ class TestClangModulesTestCase: XCTestCase {
XCTAssertDirectoryExists(prefix, "Bar/Packages/Foo-1.2.3")
}
}

func testModuleMapGenerationCases() {
fixture(name: "ClangModules/ModuleMapGenerationCases") { prefix in
XCTAssertBuilds(prefix)
XCTAssertFileExists(prefix, ".build", "debug", "libUmbrellaHeader.so")
XCTAssertFileExists(prefix, ".build", "debug", "libFlatInclude.so")
XCTAssertFileExists(prefix, ".build", "debug", "libUmbellaModuleNameInclude.so")
XCTAssertFileExists(prefix, ".build", "debug", "libNoIncludeDir.so")
XCTAssertFileExists(prefix, ".build", "debug", "Baz")
}
}
}


Expand All @@ -95,6 +106,7 @@ extension TestClangModulesTestCase {
("testiquoteDep", testiquoteDep),
("testCUsingCDep", testCUsingCDep),
("testCExecutable", testCExecutable),
("testModuleMapGenerationCases", testModuleMapGenerationCases),
]
}
}