[core] Update CMake to use TARGET_SDKS for Android

[modocache]

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

Change stdlib/public/core to use this parameter for Android, which prevents link flags intended for macOS/iOS targets from being used when building for Android.

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

[jrose-apple]

I think there are still more things that are similar than different here. I'd prefer you conditionalize the existing variables instead.

[modocache]

Variables like swift_core_private_link_libraries are conditionalized based on CMAKE_SYSTEM_NAME, a technique that doesn't work when cross-compiling from macOS to Android. Is there a better approach I could use here?

[jrose-apple]

It seems like a lot of those conditionals really want to be asking questions about the target platform, not the host platform.

[modocache]

Absolutely. add_swift_library and its TARGET_SDKS parameter work really well in that sense, since the function loops over each specified target, removing the ones that aren't being built.

I'll try to think of a different way to accomplish this, but I wonder if it'll involve duplicating some of that logic.

[modocache]

@jrose-apple Just to clarify: are you saying that the conditionals using CMAKE_SYSTEM_NAME in this file should be changed? Or just the ones that affect Android?

[jrose-apple]

Well, let's see:

• This one is about both the linker being used and the target system (what to link against).
• This one is about the target system, but depends on a Cygwin toolchain.
• This one is only about the target system.
• The actual change to what libraries are linked looks a lot like the earlier check that adds ICU for Linux.

[modocache]

I took another look at what part of these changes were really necessary to get things building. Some of the linker flags, like -all_load, would be nice to exclude, but the Android linker simply ignores them, so we get off with a warning.

The one important thing to get the Android runtime building on macOS is to prevent framework dependencies on Foundation and CoreFoundation. I amended the pull request to share all parameters across the two target sets, but not set FRAMEWORK_DEPENDS for non-Darwin targets.

This is based on @gottesmm's comment in #5004, advising I reduce the scope of the changes.

@jrose-apple, is this closer to what you had in mind?

[jrose-apple]

That new version doesn't handle ICU the way you wanted, correct?

I really just want one call to add_swift_library. At this point the only difference is the FRAMEWORK_DEPENDS, and those are definitely based on the target SDK.

[modocache]

Ah, OK, thanks! So basically we want this, but without using TARGET_SDKS and two calls to add_swift_library, right?

[modocache]

I really just want one call to add_swift_library.

Just for the record, #5002 and #5003 make multiple calls to add_swift_library. 😅

[jrose-apple]

Well then I'm not happy about those either. :-)

I'm not sure what the right answer is in general. I mostly like the SWIFT_MODULE_DEPENDS model, where we ended up adding variants for all the Apple platforms, but that has to be done manually for every argument and every platform variant right now. (And it doesn't allow groups like "all Apple platforms".)

[jrose-apple]

I feel like options should be added to all libraries by default, though, and that you should have to stop and think when it turns out they're only applicable to some.

[modocache]

One option would be adding FRAMEWORK_DEPENDS_OSX, FRAMEWORK_DEPENDS_IOS, and so on. These could work similarly to how SWIFT_MODULE_DEPENDS works.

[gottesmm]

@modocache @jrose-apple I think that sounds like a reasonable solution. In general, I dislike declaring the same library multiple times.

It feels like it is screaming to be refactored.

[gottesmm]

It may be good to look into the other cases where we used the TARGET_SDKS for Android and see if we can do similar refactoring.

[modocache]

I don't need this for building static libraries, so I'll close this for now. After #4972 is merged, I'll send another pull request with a different approach for dynamic libraries.

[modocache]

Thanks for the reviews, though! Very much appreciated!