Skip to content

Conversation

modocache
Copy link
Contributor

What's in this pull request?

spawn.h isn't available on Android. Put its import behind an #if defined(__ANDROID__) in order to fix the Android build.

/cc @compnerd, who added the import in #2006. Does this modification work for you?

Resolved bug number: None


Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

@@ -13,13 +13,16 @@
// NOTE: preprocess away the availability information to allow use of
// unsupported APIs on certain targets (i.e. tvOS)
#define availability(...)

// Spawn is not available on Android.
#if !defined(__ANDROID__)
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move it above the #define above?

@gribozavr
Copy link
Contributor

Triggering the CI, but waiting for a response from @compnerd before merging.

@swift-ci Please test

@compnerd
Copy link
Member

Just me or isn't this Subprocess.swift also built on android? (There seems to be a os(Android) check). If so, that should also go through the stubs. Otherwise, why not just remove building this subdirectory for the Android build?

@modocache
Copy link
Contributor Author

why not just remove building this subdirectory for the Android build?

We include this directory on Android for the spawnChild() function, not for the spawn.h shims. The spawnChild() function is defined in Subprocess.swift and is used by StdlibUnittest. spawn.h isn't available on Android, so when targeting Android, we include an implementation of spawnChild() in Subprocess.swift that doesn't rely on spawn.h.

@compnerd
Copy link
Member

Oh, I see. Yeah, Id just wrap the entire file (including the macro which is currently not guarded) with it. Alternatively, you could just not add it to the source list in the CMakeList.txt. But the change itself seems fine to me.

@modocache
Copy link
Contributor Author

Great! Looks like the tests passed as well. I'll move #if !defined(__ANDROID__) above the #define availability(...), then this should be good to go.

@gribozavr
Copy link
Contributor

@modocache Sounds good! Please merge directly when you are ready!

@modocache modocache force-pushed the swiftandroid-subprocess-fix branch from 6ff954e to dc06b96 Compare April 19, 2016 21:33
`spawn.h` isn't available on Android. Put its import behind an
`#if defined(__ANDROID__)` in order to fix the Android build.
@modocache modocache force-pushed the swiftandroid-subprocess-fix branch from dc06b96 to b2cf7b5 Compare April 19, 2016 21:35
@modocache modocache merged commit 77f1671 into swiftlang:master Apr 19, 2016
@modocache modocache deleted the swiftandroid-subprocess-fix branch April 19, 2016 21:35
@modocache
Copy link
Contributor Author

Thanks everyone! 🙇

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants