Skip to content
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

Auto generate module map file for C libs #219

Merged
merged 6 commits into from Apr 6, 2016

Conversation

@aciidb0mb3r
Copy link
Member

aciidb0mb3r commented Mar 22, 2016

This generates modulemap files according to the C proposal https://github.com/apple/swift-evolution/blob/master/proposals/0038-swiftpm-c-language-targets.md

Additionally if there is no include dir then it will emit an empty modulemap file with a warning.

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from 1bf031e to 28e7caa Mar 22, 2016
public func generateModuleMap() throws {

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

This comment has been minimized.

Copy link
@czechboy0

czechboy0 Mar 22, 2016

Contributor

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?

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Mar 22, 2016

Author Member

Yeah there are two problems currently...

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

This comment has been minimized.

Copy link
@mxcl

mxcl Mar 31, 2016

Contributor

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.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Mar 31, 2016

Author Member

will move the generation to build dir

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from 28e7caa to d6efcea Mar 22, 2016
@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Mar 23, 2016

@mxcl review/CI

@mxcl
Copy link
Contributor

mxcl commented Mar 31, 2016

Maybe it should error if there is no includes and it is a library? Since it is impossible to import it.

I'm mainly thinking of the packaging graph. We don't want broken packages in the wild.

OTOH maybe this is tedious.

@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Mar 31, 2016

Maybe it should error if there is no includes and it is a library? Since it is impossible to import it.

I'm mainly thinking of the packaging graph. We don't want broken packages in the wild.

OTOH maybe this is tedious.

  • Module map is only generated for libraries
  • if includes is not present then it creates an empty module map with a warning
  • if includes is present and is empty it errors out

should I change it to not emit empty module and error out?

@mxcl
Copy link
Contributor

mxcl commented Mar 31, 2016

should I change it to not emit empty module and error out?

I think this would be more consistent considering:

if includes is not present then it creates an empty module map with a warning

@ddunbar
Copy link
Member

ddunbar commented Mar 31, 2016

We definitely should be generating this stuff into a side directory in the build dir.

I would prefer it not be an error to not have includes for a lib, just because that is useful when starting development. In general, I prefer it not to be an error for things that are "well formed" even if they aren't totally usable

@mxcl
Copy link
Contributor

mxcl commented Mar 31, 2016

I would prefer it not be an error to not have includes for a lib, just because that is useful when starting development. In general, I prefer it not to be an error for things that are "well formed" even if they aren't totally usable

Maybe instead of:

if includes is not present then it creates an empty module map with a warning

We do:

if includes is not present then it creates no module-map, but warns that you should create an includes directory because otherwise it is a useless library-module

Which then flows nicely with the existing:

if includes is present and is empty it errors out

@ddunbar
Copy link
Member

ddunbar commented Apr 1, 2016

Sounds good to me!

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from d6efcea to 1866db0 Apr 1, 2016
@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 1, 2016

done, done, done and done 😄

@mxcl @ddunbar re-review


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

extension CModule {
func workingDirectory(prefix: String) -> String {

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

Can we get a documentation comment explaining what this function is?

Also, I think "workingDirectory" isn't the best name, something like "buildDirectory" would be closer to the intent.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed

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

var Xcc: [String] {
func XccFlagsForPrefix(prefix: String) -> [String] {

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

ForPrefix is redundant, if you want it to be explicit better to rename to XccFlags(prefix:)

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed

let genModuleMap = Path.join(module.workingDirectory(prefix), module.moduleMap)
if genModuleMap.isFile {
moduleMap = genModuleMap
}

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

This overwrites use of module.moduleMapPath if the generated module.moduleMap is present, which seems wrong. Also, some comments here explaining intent would be good.

I would prefer that the implementation always create the module map in the build directory, and it do so by either copying the user provided module map or generating one. The reason to always have it in the build directory is it makes downstream commands more consistent, and it more clearly separates out exactly what things are allowed to be referenced. This, however, will require copying all of the headers as well which makes it a more involved project so probably best to defer that to another PR.

This comment has been minimized.

Copy link
@mxcl

mxcl Apr 2, 2016

Contributor

Does it require copying all the headers? We have to use absolute paths anyway, and then we can just -I the package directory.

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

I don't think we need to use absolute paths, although it may be there isn't a downside to doing so.

It does require not copying the headers if we want to drop the -fmodule-map-file arguments, but maybe there isn't a need to do that. Let's start out by trying to not copy the headers (which I agree is nice), and see how far we get.

The one thing that copying the headers does give us is that if the target author makes a mistake and refers to one of the headers from their source using, e.g., "../foo.h" then it would catch that error. That is the kind of thing that could break in the future if we decided to separate out the build of dependent targets, and then move their "products" (i.e. built binary and installable headers) around.

However, that probably isn't enough motivation by itself to copy things.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed and not copying header for now.

I would prefer that the implementation always create the module map in the build directory, and it do so by either copying the user provided module map or generating one.

if user provided modulemap contains relative paths (most likely for C targets) just copying modulemap to build dir won't work unless we copy the headers too. Added a FIXME for now.

if genModuleMap.isFile {
moduleMap = genModuleMap
}
//No module map found, return with no args

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

This is a confusing comment, given the code then does the opposite. I would probably err on duplicating the -Xcc so this code can use an early return on line 36, or else factoring out the "get the module map path and/or generate" piece from the "generate the -Xcc args" part (again so an early return can be used).

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

This comment was misplaced. fixed.

let wd = module.workingDirectory(prefix)

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

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

In general, I don't think it is good that the code that decides whether or not to generate a module map is separate from the code which decides how to find that. I'm not familiar enough with the current code base to how hard that is to fix, here, but if there is a structural reason we can't compute these things in one place we should figure that out and fix it.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

For now I can't think of a direct way to do that because we try to generate module map when building C targets and module maps are needed for swift modules but I get your point.

try fopen(moduleMapPath, mode: .Write) { fp in
try fputs("\n", fp)
}
print("warning: No include directory found, a library can not be imported without any public headers.")

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

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

This comment has been minimized.

Copy link
@mxcl

mxcl Apr 2, 2016

Contributor

Agreed.

@@ -77,9 +78,9 @@ extension ClangModule {

let umbrellaHeader = Path.join(moduleHeaderDir, "\(name).h")

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

I'm tempted to say c99name is the right thing to look for here, given that is what users will write in source code. I think we should start with that, and if c99name != name then produce a warning if name.h exists.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed

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) }

try fputs("module \(name) {\n", moduleMap)

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

c99name?

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed

@@ -67,7 +68,7 @@ extension ClangModule {

if dirs.isEmpty {

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

We should get some comments here explaining the intent of this code w.r.t. what convention it is trying to support / enforce.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

done

} else {
try createModuleMap(.ModuleNameDir)
try createModuleMap(inDir: wd, type: .ModuleNameDir)

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 2, 2016

Member

The cases here aren't exactly right. There are two different axes of choice:

  • Does the target use an "umbrella" header (defined as \(c99name).h), or not.
  • Does the target use a flat structure, a single subdirectory with headers, or neither (multiple subdirectories with headers).

If the target uses multiple subdirectories, we should not create a module map.

The difference between using a flat structure or a single subdirectory just changes where the module map should go. If it is a flat structure, we should produce:

.build/include/module.modulemap
.build/include/copied.h

If it is a subdirectory, we should produce:

.build/include/foo/module.modulemap
.build/include/foo/copied.h

(this is one other reason that copying may be required, so that the relative paths in the module map work out right). It probably should work out that we no longer need the -fmodule-map-file arguments when this is all done, but I am not 100% sure of that.

If the target uses an umbrella header, then the module map should be:

module foo {
  umbrella header "foo.h"
}

If the target does not use an umbrella header, then the module map should be:
module foo {
header "a.h"
header "b.h"
...
header "xx.h"
}

where that lists all headers in the directory in lexicographic order.

Hopefully this is clear, unfortunately modules end up being rather complicated and we don't have a lot of great documentation about "why" for these things. Hopefully you have seen:
  http://clang.llvm.org/docs/Modules.html

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

As discussed, modulemap allows umbrella "dir" so not listing headers explicitly.

}

private func createModuleMap(type: UmbrellaType) throws {
let moduleMap = try fopen(moduleMapPath, mode: .Write)

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

Given that there is no performance concern here about the module map file, I suggest the this code simply create the entire module map as a String, then write it to a file. This should be cleaner than using repeated fputs() declarations.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed

case .HeaderFile:
try fputs("header \"\(name)/\(name).h\"\n", moduleMap)

}

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

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 ".

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

done

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from ff84d87 to b0057e3 Apr 4, 2016
@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 4, 2016

@ddunbar addressed the comments re-review

@ddunbar
Copy link
Member

ddunbar commented Apr 4, 2016

@swift-ci please test

let package = Package(
name: "ModuleMapGenerationCases",
targets: [
Target(name: "Baz", dependencies: ["Foo", "Bar", "Jaz"])]

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

I would recommend renaming the subdirs here to match what the intent they are trying to test is. Otherwise, when someone comes back to this test they won't necessarily know why each case exists.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

done

@ddunbar
Copy link
Member

ddunbar commented Apr 4, 2016

@swift-ci please test

return ["-Xcc", "-fmodule-map-file=\(module.moduleMapPath)"]
}
let genModuleMap = Path.join(module.buildDirectory(prefix), module.moduleMap)
return ["-Xcc", "-fmodule-map-file=\(genModuleMap)"]

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

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

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

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.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

they wont really be in dependencies

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

Adding comment, but will push once CI returns


///There are three possible cases for which we will generate modulemaps:
///Flat includes dir with only header files, in that case we generate
/// `umbrella "path/to/includes/"`

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

I think we should generate umbrella header "foo.h" even for the flat layout, if "foo.h" exists.


let invalidUmbrellaHeader = Path.join(moduleHeaderDir, "\(name).h")
if c99name != name && invalidUmbrellaHeader.isFile {
print("warning: \(invalidUmbrellaHeader) should be renamed to \(umbrellaHeader) to be used as Umbrella header")

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

I wouldn't capitalize umbrella, and there is a grammar-o: "to be used as an umbrella"

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed pending push

throw ModuleMapError.UnsupportedIncludeLayoutForModule(name)
}

let umbrellaHeader = Path.join(moduleHeaderDir, "\(c99name).h")

This comment has been minimized.

Copy link
@ddunbar

ddunbar Apr 4, 2016

Member

Comment here would be good clarifying that this is to diagnose cases where there is an ambiguity between the "natural" spelling of a module name and the "c99" spelling.

This comment has been minimized.

Copy link
@aciidb0mb3r

aciidb0mb3r Apr 4, 2016

Author Member

fixed pending push

@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from be1ff65 to 0b8e728 Apr 5, 2016
@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 5, 2016

@ddunbar updated the generation logic to :

1. Look for include/foo/foo.h, if so, use umbrella header and error if there are any other dirs or headers outside of there
2. Look for include/foo.h, if so, use umbrella header and error if there are any other dirs outside of there
3. All other cases, just use umbrella dir and don't bother to diagnose anything else
@ddunbar
Copy link
Member

ddunbar commented Apr 5, 2016

@swift-ci please test

@ddunbar
Copy link
Member

ddunbar commented Apr 5, 2016

LGTM!

A nice follow on would be to have more explanation in the "invalid layout" cases about exactly what failed, e.g.:
error: unable to generate module map for "include/foo/foo.h" with stray header file "include/bar.h"
or something (could use some more word smithing). Anything to give the user an idea of exactly what it was that kicked them out of the convention path.

@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 5, 2016

Yay! Cool. I'll try to improve the error messages in another PR

these are the updated cases for generating modulemaps:

1. Look for include/foo/foo.h, if so, use umbrella header and error if there are any other dirs or headers outside of there
2. Look for include/foo.h, if so, use umbrella header and error if there are any other dirs outside of there
3. All other cases, just use umbrella dir and don't bother to diagnose anything else
@aciidb0mb3r aciidb0mb3r force-pushed the aciidb0mb3r:modulemap_generation branch from d6450e6 to c47c6b8 Apr 5, 2016
@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 5, 2016

Fixed the linux error, @ddunbar re run CI please

@ddunbar
Copy link
Member

ddunbar commented Apr 5, 2016

@swift-ci please test

@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 5, 2016

osx tests keep failing due to some watch kit thingy

@aciidb0mb3r
Copy link
Member Author

aciidb0mb3r commented Apr 6, 2016

@mxcl @ddunbar can you retrigger CI

@ddunbar
Copy link
Member

ddunbar commented Apr 6, 2016

@swift-ci please test and merge

@ddunbar
Copy link
Member

ddunbar commented Apr 6, 2016

@swift-ci please test

@aciidb0mb3r aciidb0mb3r merged commit e93a62d into apple:master Apr 6, 2016
2 checks passed
2 checks passed
Swift Test Linux Platform Build finished. 8046 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform Build finished. 32348 tests run, 0 skipped, 0 failed.
Details
@aciidb0mb3r aciidb0mb3r deleted the aciidb0mb3r:modulemap_generation branch Apr 6, 2016
@ddunbar
Copy link
Member

ddunbar commented Apr 6, 2016

Thanks!

aciidb0mb3r pushed a commit to aciidb0mb3r/swift-package-manager that referenced this pull request Jan 11, 2019
Implemented a build manifest generator to help stress test llbuild.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.