Auto generate module map file for C libs #219

aciidgh · 2016-03-22T20:11:28Z

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.

czechboy0 · 2016-03-22T20:25:33Z

Sources/Build/misc.swift

+    public func generateModuleMap() throws {
+
+        //Return if module map is already present
+        guard !moduleMapPath.isFile else {


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?

Yeah there are two problems currently...

module maps should probably be generated in build dir

incremental map generation

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.

will move the generation to build dir

aciidgh · 2016-03-23T20:13:26Z

@mxcl review/CI

mxcl · 2016-03-31T17:42:04Z

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.

aciidgh · 2016-03-31T18:09:45Z

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 · 2016-03-31T18:27:32Z

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 · 2016-03-31T18:44:08Z

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 · 2016-03-31T18:59:28Z

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 · 2016-04-01T07:07:36Z

Sounds good to me!

aciidgh · 2016-04-01T12:04:28Z

done, done, done and done 😄

@mxcl @ddunbar re-review

ddunbar · 2016-04-02T00:06:16Z

Sources/Build/Buildable.swift


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

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


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.

aciidgh · 2016-04-04T17:37:32Z

@ddunbar addressed the comments re-review

ddunbar · 2016-04-04T17:45:54Z

@swift-ci please test

ddunbar · 2016-04-04T17:48:50Z

Fixtures/ClangModules/ModuleMapGenerationCases/Package.swift

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


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.

ddunbar · 2016-04-04T17:59:47Z

@swift-ci please test

ddunbar · 2016-04-04T18:03:30Z

Sources/Build/Buildable.swift

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


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

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.

they wont really be in dependencies

Adding comment, but will push once CI returns

ddunbar · 2016-04-04T18:07:18Z

Sources/Build/misc.swift

+
+        ///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/"`


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

aciidgh · 2016-04-05T06:36:30Z

@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 · 2016-04-05T06:38:45Z

@swift-ci please test

ddunbar · 2016-04-05T06:40:35Z

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.

aciidgh · 2016-04-05T06:42:09Z

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

aciidgh · 2016-04-05T07:54:18Z

Fixed the linux error, @ddunbar re run CI please

ddunbar · 2016-04-05T15:43:09Z

@swift-ci please test

aciidgh · 2016-04-05T16:46:55Z

osx tests keep failing due to some watch kit thingy

aciidgh · 2016-04-06T02:29:10Z

@mxcl @ddunbar can you retrigger CI

ddunbar · 2016-04-06T03:41:36Z

@swift-ci please test and merge

ddunbar · 2016-04-06T03:46:27Z

@swift-ci please test

ddunbar · 2016-04-06T16:54:42Z

Thanks!

Implemented a build manifest generator to help stress test llbuild.

aciidgh force-pushed the modulemap_generation branch from 1bf031e to 28e7caa Compare March 22, 2016 20:18

czechboy0 reviewed Mar 22, 2016
View reviewed changes

aciidgh force-pushed the modulemap_generation branch from 28e7caa to d6efcea Compare March 22, 2016 21:24

aciidgh force-pushed the modulemap_generation branch from d6efcea to 1866db0 Compare April 1, 2016 10:13

ddunbar reviewed Apr 2, 2016
View reviewed changes

aciidgh force-pushed the modulemap_generation branch from ff84d87 to b0057e3 Compare April 4, 2016 17:35

ddunbar reviewed Apr 4, 2016
View reviewed changes

aciidgh added 5 commits April 5, 2016 11:38

Auto generate module map file for C libs

26b56cf

Generate module maps in build dir

060204b

Add comments for some of the methods

7fb0539

Rename modules in fixtures to reflect their intent

8986238

Add more documentation

0b8e728

aciidgh force-pushed the modulemap_generation branch from be1ff65 to 0b8e728 Compare April 5, 2016 06:08

aciidgh force-pushed the modulemap_generation branch from d6450e6 to c47c6b8 Compare April 5, 2016 07:53

aciidgh merged commit e93a62d into swiftlang:master Apr 6, 2016

aciidgh deleted the modulemap_generation branch April 6, 2016 07:29

JohnCoates mentioned this pull request May 11, 2016

Not compiling in Xcode with 2016-05-03 toolchain: library not found: CURIParser Zewo/Zewo#107

Closed

aciidgh pushed a commit to aciidgh/swift-package-manager that referenced this pull request Jan 11, 2019

Merge pull request swiftlang#219 from baujla/add-manifest-generator

91f2c5d

Implemented a build manifest generator to help stress test llbuild.

abertelrud mentioned this pull request Jul 20, 2020

[5.3] Add a warning for cases in which SwiftPM generates a module map for unsupported header layouts #2819

Closed

Auto generate module map file for C libs #219

Auto generate module map file for C libs #219

Uh oh!

Conversation

aciidgh commented Mar 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Mar 23, 2016

Uh oh!

mxcl commented Mar 31, 2016

Uh oh!

aciidgh commented Mar 31, 2016

Uh oh!

mxcl commented Mar 31, 2016

Uh oh!

ddunbar commented Mar 31, 2016

Uh oh!

mxcl commented Mar 31, 2016

Uh oh!

ddunbar commented Apr 1, 2016

Uh oh!

aciidgh commented Apr 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Apr 4, 2016

Uh oh!

ddunbar commented Apr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddunbar commented Apr 4, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aciidgh commented Apr 5, 2016

Uh oh!

ddunbar commented Apr 5, 2016

Uh oh!

ddunbar commented Apr 5, 2016

Uh oh!

aciidgh commented Apr 5, 2016

Uh oh!

aciidgh commented Apr 5, 2016

Uh oh!

ddunbar commented Apr 5, 2016

Uh oh!

aciidgh commented Apr 5, 2016

Uh oh!

aciidgh commented Apr 6, 2016

Uh oh!

ddunbar commented Apr 6, 2016

Uh oh!

ddunbar commented Apr 6, 2016

Uh oh!

ddunbar commented Apr 6, 2016

Uh oh!

Uh oh!