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

[stdlib] Only add Apple reflection test targets #5175

Merged

Conversation

modocache
Copy link
Collaborator

When building on a macOS host, and when SWIFT_INCLUDE_TESTS is specified, the swiftSwiftReflectionTest target is added to all platforms. However, this target has a dependency upon Foundation, which is not available on non-Apple platforms.

Use add_swift_library's TARGET_SDKS parameter and other gating logic to ensure the target is only added for platforms that actually have Darwin available.

This was pulled out of the larger pull request #4972, which addresses SR-1362.

@modocache
Copy link
Collaborator Author

@swift-ci Please test

@moiseev
Copy link
Contributor

moiseev commented Oct 7, 2016

/cc @erg

"swiftSwiftReflectionTest${VARIANT_SUFFIX}"
"swift-reflection-test${VARIANT_SUFFIX}")
endforeach()
if("${SDK}" IN_LIST SWIFT_APPLE_PLATFORMS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Where does SWIFT_APPLE_PLATFORMS come from?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apologies, looks like I lost a line of source code when I split up #4972. I amended the commit with the missing CMake changes that add SWIFT_APPLE_PLATFORMS. 😨

When building on a macOS host, and when `SWIFT_INCLUDE_TESTS` is specified,
the `swiftSwiftReflectionTest` target is added to all platforms.
However, this target has a dependency upon Foundation, which is not
available on non-Apple platforms.

Use `add_swift_library`'s `TARGET_SDKS` parameter and other gating
logic to ensure the target is only added for platforms that actually
have Darwin available.
@modocache modocache force-pushed the cmake-reflection-test-macos-host-android branch from aef94ff to c42d0d2 Compare October 7, 2016 19:41
@erg
Copy link
Contributor

erg commented Oct 7, 2016

@swift-ci Please test

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2016

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 619b25dddfd030fb713d100d7209c62c3f0af0e9
Test requested by - @erg

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 619b25dddfd030fb713d100d7209c62c3f0af0e9
Test requested by - @erg

@erg
Copy link
Contributor

erg commented Oct 7, 2016

Looks good if it builds clean!

@swift-ci
Copy link
Collaborator

swift-ci commented Oct 7, 2016

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 619b25dddfd030fb713d100d7209c62c3f0af0e9
Test requested by - @modocache

@modocache
Copy link
Collaborator Author

@swift-ci Please clean test

@modocache modocache merged commit d9fe1d0 into apple:master Oct 8, 2016
@modocache modocache deleted the cmake-reflection-test-macos-host-android branch October 8, 2016 18:47
@modocache
Copy link
Collaborator Author

Thanks!

@jckarter
Copy link
Member

jckarter commented Oct 8, 2016

Is there any cross-platform subset of swiftSwiftReflectionTest that can be factored out, so we get some coverage on non-Apple platforms?

@gottesmm
Copy link
Member

gottesmm commented Oct 9, 2016

I really dislike that we have this target code in the Swift tools directory. We should consider sinking this into a stdlib/tools directory or something like that.

tools IMO should be meant only for "build tools", not for "target tools".

@gottesmm
Copy link
Member

gottesmm commented Oct 9, 2016

I hope that at some point we follow the separate cmake cross compilation model for the stdlib and then sink this code into there.

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.

None yet

7 participants