[stubs] Update CMake to use TARGET_SDKS (NFC) #5002

modocache · 2016-09-25T02:32:51Z

The add_swift_library CMake function takes an optional TARGET_SDKS parameter. When used, only CMake targets for the specified SDKs are added.

Refactor stdlib/public/stubs to use this parameter. This also eliminates logic that determines additional flags or source files to include based on SWIFT_HOST_VARIANT, which makes it easier for hosts to add targets for different platforms.

This pull request was split out of #4972, which addresses SR-1362.

gottesmm · 2016-09-25T02:42:46Z

So I guess what is happening here is that instead of doing tests using if statements, we are using TARGET_SDK and creating two different libraries to decide what to conditionally compile?

gottesmm · 2016-09-25T02:44:04Z

Yes. I did understand this correctly. It is a little weird since I think we usually use such an IN_SDK thing for individual libraries that are only on one platform or the other IIRC.

gottesmm

Looks good overall, just want some updated comments.

gottesmm · 2016-09-25T02:45:16Z

stdlib/public/stubs/CMakeLists.txt

+    SwiftNativeNSXXXBase.mm.gyb)
+set(swift_stubs_unicode_normalization_sources
+    UnicodeNormalization.cpp)
+set(LLVM_OPTIONAL_SOURCES


Isn't LLVM_OPTIONAL_SOURCES dead?

Removing this results in a CMake error, so I don't think so.

gottesmm · 2016-09-25T02:49:10Z

stdlib/public/stubs/CMakeLists.txt

@@ -1,33 +1,40 @@
-set(swift_stubs_objc_sources)
-set(swift_stubs_unicode_normalization_sources)
+set(swift_stubs_sources


Can you add comments for each group of these sources. Maybe saying something like:

// Platform Independent Sources
set(swift_stubs_platform_independent_sources ...)

// Darwin Only ObjC Sources. This consists of objective-c related stubs.
...

// Sources for Android, Cygwin, FreeBSD, Linux. This consists currently of just ICU sources. [[INSERT EXPLANATION OF WHY WE DO NOT DO THIS ON DARWIN]].
...

I forgot the story with ICU on Darwin, but IIRC @aschwaighofer knows. Arnold, do you remember.

On Darwin we don't use ICU for String functions (hashing, comparison) but call to Foundation's NSString/CFString functions. On non-darwin platforms we call to ICU for this functionality.
The stubs for calling to ICU are in UnicodeNormalization.cpp.

modocache · 2016-09-25T03:03:34Z

It is a little weird since I think we usually use such an IN_SDK thing for individual libraries that are only on one platform or the other IIRC.

That's fair. I think the closest example of TARGET_SDKS being used this way is in stdlib/public/Platform, although the target names are different: swiftDarwin, swiftGlibc, and swiftMSVCRT. I guess it's unconventional, but it does work remarkably well for #4972... 😇

gottesmm · 2016-09-25T04:35:47Z

maybe the way to do it is to just change the names of the libraries. I.e. maybe: swiftStubsDarwin. But I am not sure what you would use for the others... NonDarwin?

modocache · 2016-09-25T15:10:36Z

Even if they're given the same names, the libraries do end up in their own subfolders -- macOS's in macosx/x86_84/libswiftCore.dylib, Android's in android/amv7/libswiftCore.so, and so on. I feel like that's ideal. It seems like naming them differently will make it harder to specify them as dependencies -- swiftGlibc will need to depend on swiftCoreAndroid, swiftDarwin will need to depend on swiftCore or swiftCoreDarwin, and so on. On the other hand, is there a compelling reason to name them differently? If this isn't the current convention, maybe it could become the new convention...? :)

modocache · 2016-09-26T14:51:53Z

@gottesmm New comments added! Let me know what you think.

gottesmm · 2016-09-28T22:25:04Z

I think this is fine. Resolve the conflict and let er rip.

The `add_swift_library` CMake function takes an optional `TARGET_SDKS` parameter. When used, only CMake targets for the specified SDKs are added. Refactor `stdlib/public/stubs` to use this parameter. This also eliminates logic that determines additional flags or source files to include based on `SWIFT_HOST_VARIANT`, which makes it easier for hosts to add targets for different platforms.

modocache · 2016-09-29T02:42:44Z

@swift-ci please smoke test and merge

modocache · 2016-09-29T02:43:24Z

@swift-ci Please smoke test and merge

tfiala · 2016-09-29T15:53:56Z

We have several builders that are red after this. Two external ones are here:
https://ci.swift.org/job/oss-lldb-incremental-linux-ubuntu-15_10/6238/
https://ci.swift.org/job/oss-lldb-incremental-linux-ubuntu-16_04/83/

I'm going to try a clean build. If that doesn't fix it, we need this removed.

gottesmm · 2016-09-29T16:17:39Z

@modocache @tfiala If we need to revert this, this just shows how in light of cmake changes, we need to be sure that we use a clean test.

tfiala · 2016-09-29T16:17:41Z

It appears all that is needed here is a build directory wipe. Two of the CI nodes I saw failing made it past the CMake issue in the Swift portion after doing a clean build.

tfiala · 2016-09-29T16:20:08Z

(i.e. this is fine).

@gottesmm yes, I think we may want smarts to detect cmake changes, and when present, maybe force a clean build. We'll have to think about that and look at how many clean builds that would have triggered. I don't know the portion of changelists that have CMake-related changes, but it's something we can look at.

tfiala · 2016-09-29T16:25:57Z

It gets a bit trickier than that, though, since we have several build slaves that are eligible for a job. So it's not good enough to just scan the new changes since the last build for a build job. We'd have to scan the new change lists since the last build of the job per slave, since the build slave has its own local directory.

Maybe the better thing to do here would be to wipe the build dir on all eligible build slaves at the time of detection. That simplifies the "how to delete" step, but complicates the "find and execute" brains of it. Anyways, we'll take this offline.

modocache · 2016-09-29T17:03:29Z

Oomph, sorry for the build failures, and sorry about not responding sooner. I'll use the Swift CI clean build options for the other CMake pull requests I have queued up. Let me know if there's also something I can do beyond that.

tfiala · 2016-09-29T17:05:48Z

@modocache Oh no worries, I think it exposes something we probably need to address at the infrastructure level. (Not to mention, CMake probably shouldn't be failing here, but I'm not sure if that's an issue with our application of it or an issue of it not flushing out something it should have flushed out).

gottesmm · 2016-09-29T18:02:44Z

@tfiala I already have some thoughts in this area, but it would be good to also involve @llvm-beanz as well.

The issue that I have found to cause this the most often is with respect to assumptions around default values being added to the cmake cache. So for instance, someone makes a change, provides a default that will work, but in the incremental build there is already a cached value that does not work with the default. So things get out of wack.

tfiala · 2016-09-29T18:42:33Z

@tfiala I already have some thoughts in this area, but it would be good to also involve @llvm-beanz as well.

Okay. As long as we're working towards a solution, that sounds good.

llvm-beanz · 2016-09-29T22:11:24Z

Looking at this, it is almost certainly a situation where swift's build is using cached variables and shouldn't be.

Since the commits didn't directly change the value of a cached variable there should be no need to clean out the caches, unless there are cached variables being used in unholy ways (which I think is the case here).

For example it looks like library dependencies are being cached as internal variables. That is not right.

gottesmm reviewed Sep 25, 2016

View reviewed changes

modocache force-pushed the stdlib-stubs-cmake-target-sdks branch from 1d569fd to f04ef12 Compare September 26, 2016 14:51

modocache force-pushed the stdlib-stubs-cmake-target-sdks branch from f04ef12 to b86125a Compare September 29, 2016 02:42

swift-ci merged commit ba1127f into swiftlang:master Sep 29, 2016

modocache deleted the stdlib-stubs-cmake-target-sdks branch September 29, 2016 17:03

modocache mentioned this pull request Oct 5, 2016

[core] Update CMake to use TARGET_SDKS for Android #5095

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[stubs] Update CMake to use TARGET_SDKS (NFC) #5002

[stubs] Update CMake to use TARGET_SDKS (NFC) #5002

modocache commented Sep 25, 2016

gottesmm commented Sep 25, 2016

gottesmm commented Sep 25, 2016

gottesmm left a comment

gottesmm Sep 25, 2016 •

edited

Loading

modocache Sep 26, 2016

gottesmm Sep 25, 2016 •

edited

Loading

aschwaighofer Sep 25, 2016

modocache commented Sep 25, 2016 •

edited

Loading

gottesmm commented Sep 25, 2016

modocache commented Sep 25, 2016

modocache commented Sep 26, 2016 •

edited

Loading

gottesmm commented Sep 28, 2016

modocache commented Sep 29, 2016

modocache commented Sep 29, 2016

tfiala commented Sep 29, 2016

gottesmm commented Sep 29, 2016

tfiala commented Sep 29, 2016

tfiala commented Sep 29, 2016

tfiala commented Sep 29, 2016

modocache commented Sep 29, 2016

tfiala commented Sep 29, 2016

gottesmm commented Sep 29, 2016

tfiala commented Sep 29, 2016

llvm-beanz commented Sep 29, 2016

[stubs] Update CMake to use TARGET_SDKS (NFC) #5002

[stubs] Update CMake to use TARGET_SDKS (NFC) #5002

Conversation

modocache commented Sep 25, 2016

gottesmm commented Sep 25, 2016

gottesmm commented Sep 25, 2016

gottesmm left a comment

Choose a reason for hiding this comment

gottesmm Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

modocache Sep 26, 2016

Choose a reason for hiding this comment

gottesmm Sep 25, 2016 • edited Loading

Choose a reason for hiding this comment

aschwaighofer Sep 25, 2016

Choose a reason for hiding this comment

modocache commented Sep 25, 2016 • edited Loading

gottesmm commented Sep 25, 2016

modocache commented Sep 25, 2016

modocache commented Sep 26, 2016 • edited Loading

gottesmm commented Sep 28, 2016

modocache commented Sep 29, 2016

modocache commented Sep 29, 2016

tfiala commented Sep 29, 2016

gottesmm commented Sep 29, 2016

tfiala commented Sep 29, 2016

tfiala commented Sep 29, 2016

tfiala commented Sep 29, 2016

modocache commented Sep 29, 2016

tfiala commented Sep 29, 2016

gottesmm commented Sep 29, 2016

tfiala commented Sep 29, 2016

llvm-beanz commented Sep 29, 2016

gottesmm Sep 25, 2016 •

edited

Loading

gottesmm Sep 25, 2016 •

edited

Loading

modocache commented Sep 25, 2016 •

edited

Loading

modocache commented Sep 26, 2016 •

edited

Loading