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

Add basic support for building C via swiftpm #183

Merged
merged 16 commits into from Mar 12, 2016

Conversation

Projects
None yet
4 participants
@aciidb0mb3r
Member

aciidb0mb3r commented Mar 9, 2016

Basic implementation of proposal https://github.com/apple/swift-evolution/blob/master/proposals/0038-swiftpm-c-language-targets.md

  • Swiftpm can be used to create and build C libraries.
  • Same rules apply for layouts except that include/module.modulemap should be present for each C lang module
  • If a dir contains .swift and .c, it will error out.
  • Right now only creates shared libraries and not executable.
  • Assumes module.modulemap will be available inside a include directory, doesn't generates it automatically.
  • Adds Test cases for possible layouts and mixing of swift and c code.

Remaining features will be added incrementally.

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 10, 2016

Member

@mxcl @ddunbar would love inputs on this

Member

aciidb0mb3r commented Mar 10, 2016

@mxcl @ddunbar would love inputs on this

@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Mar 10, 2016

Contributor

Looks like a great start and really: great job getting this going.

I'll review it more thoroughly in the morning.

Contributor

mxcl commented Mar 10, 2016

Looks like a great start and really: great job getting this going.

I'll review it more thoroughly in the morning.

@ddunbar

This comment has been minimized.

Show comment
Hide comment
@ddunbar

ddunbar Mar 10, 2016

Member

OMG awesome! Will review ASAP

Member

ddunbar commented Mar 10, 2016

OMG awesome! Will review ASAP

@ddunbar

This comment has been minimized.

Show comment
Hide comment
@ddunbar

ddunbar Mar 10, 2016

Member

Overall seems like a great start to me. There are a couple things I'd like to see fixed before merge (mostly having to do with erroring out on unsupported things, or not erroring out on things which do work but aren't yet complete (like not require the module.modulemap to be written, even though we don't synthesize it yet)), but once @mxcl is happy with it I'd be glad to see it go in and then build in the other things like module map synthesis or incremental build support.

Member

ddunbar commented Mar 10, 2016

Overall seems like a great start to me. There are a couple things I'd like to see fixed before merge (mostly having to do with erroring out on unsupported things, or not erroring out on things which do work but aren't yet complete (like not require the module.modulemap to be written, even though we don't synthesize it yet)), but once @mxcl is happy with it I'd be glad to see it go in and then build in the other things like module map synthesis or incremental build support.

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 10, 2016

Member

@ddunbar I think I was able to resolve almost all of the things pointed by you. You can re-review the changes here: https://github.com/apple/swift-package-manager/pull/183/files

Member

aciidb0mb3r commented Mar 10, 2016

@ddunbar I think I was able to resolve almost all of the things pointed by you. You can re-review the changes here: https://github.com/apple/swift-package-manager/pull/183/files

var args: [String] = []
#if os(Linux)
args += ["-fPIC"]

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Mar 10, 2016

Member

Tests were failing on linux and clang suggested to recompile using this. Not sure if this is correct? @mxcl @ddunbar

@aciidb0mb3r

aciidb0mb3r Mar 10, 2016

Member

Tests were failing on linux and clang suggested to recompile using this. Not sure if this is correct? @mxcl @ddunbar

This comment has been minimized.

@ddunbar

ddunbar Mar 10, 2016

Member

That is probably correct behavior, I would want to test it with Swift interop to know for sure.

On Mar 10, 2016, at 11:04 AM, Ankit Agarwal notifications@github.com wrote:

In Sources/Build/describe().swift #183 (comment):

  •    //if it not present
    
  •    if !module.moduleMapPath.isFile {
    
  •        try mkdir(module.moduleMapPath.parentDirectory)
    
  •        try fopen(module.moduleMapPath, mode: .Write) { fp in
    
  •            try fputs("\n", fp)
    
  •        }
    
  •    }
    
  •    let inputs = module.dependencies.map{ $0.targetName } + module.sources.paths
    
  •    let productPath = Path.join(prefix, "lib(module.c99name).so")
    
  •    let wd = Path.join(prefix, "(module.c99name).build")
    
  •    mkdirs.insert(wd)
    
  •    var args: [String] = []
    
  • #if os(Linux)
  •     args += ["-fPIC"]
    
    Tests were failing on linux and clang suggested to recompile using this. Not sure if this is correct? @mxcl https://github.com/mxcl @ddunbar https://github.com/ddunbar

    Reply to this email directly or view it on GitHub https://github.com/apple/swift-package-manager/pull/183/files#r55731599.
@ddunbar

ddunbar Mar 10, 2016

Member

That is probably correct behavior, I would want to test it with Swift interop to know for sure.

On Mar 10, 2016, at 11:04 AM, Ankit Agarwal notifications@github.com wrote:

In Sources/Build/describe().swift #183 (comment):

  •    //if it not present
    
  •    if !module.moduleMapPath.isFile {
    
  •        try mkdir(module.moduleMapPath.parentDirectory)
    
  •        try fopen(module.moduleMapPath, mode: .Write) { fp in
    
  •            try fputs("\n", fp)
    
  •        }
    
  •    }
    
  •    let inputs = module.dependencies.map{ $0.targetName } + module.sources.paths
    
  •    let productPath = Path.join(prefix, "lib(module.c99name).so")
    
  •    let wd = Path.join(prefix, "(module.c99name).build")
    
  •    mkdirs.insert(wd)
    
  •    var args: [String] = []
    
  • #if os(Linux)
  •     args += ["-fPIC"]
    
    Tests were failing on linux and clang suggested to recompile using this. Not sure if this is correct? @mxcl https://github.com/mxcl @ddunbar https://github.com/ddunbar

    Reply to this email directly or view it on GitHub https://github.com/apple/swift-package-manager/pull/183/files#r55731599.

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Mar 11, 2016

Member

I've tests in this PR calling c methods from swift.

@aciidb0mb3r

aciidb0mb3r Mar 11, 2016

Member

I've tests in this PR calling c methods from swift.

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 10, 2016

Member

Also I am waiting for a new toolchain to arrive before I resolve conflicts. Its just one small method rename I think.

Member

aciidb0mb3r commented Mar 10, 2016

Also I am waiting for a new toolchain to arrive before I resolve conflicts. Its just one small method rename I think.

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 11, 2016

Member

I succeeded in building toolchain from latest masters and then use that for developing this. Now rebased correctly. 😬

Member

aciidb0mb3r commented Mar 11, 2016

I succeeded in building toolchain from latest masters and then use that for developing this. Now rebased correctly. 😬

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 11, 2016

Member

@mxcl CI on this? though probably review first.

Member

aciidb0mb3r commented Mar 11, 2016

@mxcl CI on this? though probably review first.

@ddunbar

This comment has been minimized.

Show comment
Hide comment
@ddunbar

ddunbar Mar 11, 2016

Member

Let's try it.

@swift-ci please test

Member

ddunbar commented Mar 11, 2016

Let's try it.

@swift-ci please test

mxcl added a commit that referenced this pull request Mar 12, 2016

Merge pull request #183 from aciidb0mb3r/c_support
Add basic support for building C via swiftpm

@mxcl mxcl merged commit 57c869a into apple:master Mar 12, 2016

2 checks passed

Swift Test Linux Platform Build finished. 7995 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform Build finished. 32144 tests run, 0 skipped, 0 failed.
Details
@mxcl

This comment has been minimized.

Show comment
Hide comment
@mxcl

mxcl Mar 12, 2016

Contributor

A key point in our evolution, many thanks everyone 👊🏻

Contributor

mxcl commented Mar 12, 2016

A key point in our evolution, many thanks everyone 👊🏻

@ddunbar

This comment has been minimized.

Show comment
Hide comment
@ddunbar

ddunbar Mar 12, 2016

Member

Woot!

Member

ddunbar commented Mar 12, 2016

Woot!

@aciidb0mb3r

This comment has been minimized.

Show comment
Hide comment
@aciidb0mb3r

aciidb0mb3r Mar 12, 2016

Member

😱😱 didn't expect a merge so soon. Thanks! It was fun doing this. Will try adding other remaining things from proposal😀

Member

aciidb0mb3r commented Mar 12, 2016

😱😱 didn't expect a merge so soon. Thanks! It was fun doing this. Will try adding other remaining things from proposal😀

if !cSources.isEmpty {
guard swiftSources.isEmpty else { throw Module.Error.MixedSources(path) }
//FIXME: Support executables for C languages
guard !cSources.contains({ $0.hasSuffix("main.c") }) else { throw Module.Error.CExecutableNotSupportedYet(path) }

This comment has been minimized.

@mxcl

mxcl Mar 15, 2016

Contributor

Should check the basename.

@mxcl

mxcl Mar 15, 2016

Contributor

Should check the basename.

This comment has been minimized.

@aciidb0mb3r

aciidb0mb3r Mar 15, 2016

Member

raised #199 for this

@aciidb0mb3r

aciidb0mb3r Mar 15, 2016

Member

raised #199 for this

@aciidb0mb3r aciidb0mb3r deleted the aciidb0mb3r:c_support branch Apr 1, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment