Skip to content

build: remove TARGET_SDKS from the runtime #5942

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 6, 2016
Merged

build: remove TARGET_SDKS from the runtime #5942

merged 1 commit into from
Dec 6, 2016

Conversation

compnerd
Copy link
Member

Replace this paragraph with a description of your changes and rationale. Provide links to external references/discussions if appropriate.

Resolves SR-NNNN.

The runtime and stubs are built for ALL targets, not specific ones. This allows
us to configure when cross-compiling to Windows again.

@compnerd
Copy link
Member Author

@swift-ci please test and merge

2 similar comments
@compnerd
Copy link
Member Author

@swift-ci please test and merge

@compnerd
Copy link
Member Author

@swift-ci please test and merge

@hughbe hughbe mentioned this pull request Nov 28, 2016
Merged
jrose-apple
jrose-apple suggested changes Nov 28, 2016
View reviewed changes
Copy link
Contributor

@jrose-apple jrose-apple left a comment

Choose a reason for hiding this comment

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

This is wrong. There's another target just above for ALL_APPLE_PLATFORMS. We just don't have an ALL_NON_APPLE_PLATFORMS.

@compnerd compnerd force-pushed the windows-target-sdks branch from 84ec644 to 956ebe8 Compare November 29, 2016 04:51
@compnerd
Copy link
Member Author

Didn't notice that originally. It is silly to have the dual add_library for this. Ive collapsed the two into a single build.

@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 84ec6445d208a727c418ff9922f9d346aa8296da
Test requested by - @compnerd

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 84ec6445d208a727c418ff9922f9d346aa8296da
Test requested by - @compnerd

@compnerd compnerd force-pushed the windows-target-sdks branch from 956ebe8 to c2a9443 Compare November 29, 2016 05:06
@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - 956ebe80e853957fac184732d62a31230f4378a6
Test requested by - @compnerd

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 956ebe80e853957fac184732d62a31230f4378a6
Test requested by - @compnerd

@compnerd compnerd force-pushed the windows-target-sdks branch from c2a9443 to 1586d61 Compare November 29, 2016 06:02
@compnerd
Copy link
Member Author

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test OS X Platform
Git Commit - c2a9443a385213cee83072115d9d4999408462e5
Test requested by - @compnerd

@swift-ci
Copy link
Contributor

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - c2a9443a385213cee83072115d9d4999408462e5
Test requested by - @compnerd

@jrose-apple
Copy link
Contributor

CC @modocache, @erg, @jckarter

I don't know if this is really "simpler". Different platforms really do need different options sometimes.

@modocache
Copy link
Contributor

modocache commented Nov 30, 2016 •
edited
Loading

I agree that I imagine some targets will need different options in some cases, but this seems like a simplification for at least the runtime. I like it!

I just pulled it down to try building for Android; I'll let you know how that goes. And it works great on Android, too.

@compnerd
build: remove TARGET_SDKS from the runtime
c67a33f 
The runtime and stubs are built for ALL targets, not specific ones.  This allows
us to configure when cross-compiling to Windows again.  Collapse the dual
addition of the swiftRuntime into a single build.  This unifies the runtime
build for the apple and non-Apple SDKs.  The difference here was the ObjC
interop sources.  In order to deal with that unification add a CPP macro to
indicate whether the interop sources should be included or not.
@compnerd compnerd force-pushed the windows-target-sdks branch from 1586d61 to c67a33f Compare December 4, 2016 04:26
@compnerd
Copy link
Member Author

compnerd commented Dec 5, 2016

@jrose-apple can we get some closure on this?

@compnerd
Copy link
Member Author

compnerd commented Dec 5, 2016

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 5, 2016

Build failed
Jenkins build - Swift Test Linux Platform
Git Commit - 1586d6127a9e50eaba870dc47debf576658e9cd0
Test requested by - @compnerd

jrose-apple
jrose-apple previously approved these changes Dec 5, 2016
View reviewed changes
@jrose-apple jrose-apple dismissed their stale review December 5, 2016 20:42

Not really my call

@jrose-apple
Copy link
Contributor

@jckarter touches the runtime files the most, so I guess he should answer. I still feel a little weird having .mm files even opened on Linux, though.

@jckarter
Copy link
Contributor

jckarter commented Dec 5, 2016

I'm a bit concerned too. I'm surprised clang even knows what to do with a .mm file on Linux.

@jckarter
Copy link
Contributor

jckarter commented Dec 5, 2016

I do think that having the conditionalization in the source files is a bit nicer than having it in CMake, since our CMake code is already pretty complicated and CMake strikes me as a worse language than C preprocessor for this kind of this.

@compnerd
Copy link
Member Author

compnerd commented Dec 6, 2016

@swift-ci please smoke test os x

@compnerd
Copy link
Member Author

compnerd commented Dec 6, 2016

@swift-ci please smoke test and merge

@compnerd compnerd merged commit 3328592 into swiftlang:master Dec 6, 2016
@compnerd compnerd deleted the windows-target-sdks branch December 6, 2016 16:02
@antonmes antonmes mentioned this pull request Oct 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrose-apple jrose-apple jrose-apple left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@compnerd @swift-ci @jrose-apple @modocache @jckarter