Apps who depends on `swift-composable-architecture` build failed if `minimum_os_version >= 17.0`

iXerol commented 5 months ago

I'm not sure whether it's a rules_swift_package_manager issue or rules_swift issue.

swift-composable-architecture has a dependency swift-perception. swift-perception is a backport library for Observation. Since Observation requires iOS 17+, some API in swift-perception are marked obsoleted on iOS 17. eg. Bindable

  @available(iOS, introduced: 13, obsoleted: 17)
  @available(macOS, introduced: 10.15, obsoleted: 14)
  @available(tvOS, introduced: 13, obsoleted: 17)
  @available(watchOS, introduced: 6, obsoleted: 10)
  @available(visionOS, unavailable)
  @dynamicMemberLookup
  @propertyWrapper
  public struct Bindable<Value>

the TCA Sample build succeeds when minimum_os_version = "16.0". However, if the line is changed to minimum_os_version = "17.0", errors like the following will occur.

error: emit-module command failed with exit code 1 (use -v to see invocation)
external/rules_swift_package_manager~override~swift_deps~swiftpkg_swift_perception/Sources/Perception/Bindable.swift:21:32: error: 'Bindable' is unavailable in iOS
    public var projectedValue: Bindable<Value> {
                               ^~~~~~~~
external/rules_swift_package_manager~override~swift_deps~swiftpkg_swift_perception/Sources/Perception/Bindable.swift:15:17: note: 'Bindable' was obsoleted in iOS 17
  public struct Bindable<Value> {
                ^
external/rules_swift_package_manager~override~swift_deps~swiftpkg_swift_perception/Sources/Perception/Bindable.swift:36:63: warning: 'Perceptible' was deprecated in iOS 17: renamed to 'Observable'
    public init(wrappedValue: Value) where Value: AnyObject & Perceptible {
                                                              ^
external/rules_swift_package_manager~override~swift_deps~swiftpkg_swift_perception/Sources/Perception/Bindable.swift:36:63: note: use 'Observable' instead
    public init(wrappedValue: Value) where Value: AnyObject & Perceptible {
                                                              ^~~~~~~~~~~
                                                              Observable

While the minimum deployments versions of some TCA examples are also >= 17.0, and they can build successfully in Xcode & Swift Packages.

cgrindel commented 5 months ago

I have dug into this. However, I noticed that swift-perception has a macro target. We have not added support for that yet. I wonder if the macros make it work in Xcode. 🤔

iXerol commented 5 months ago

But the generated BUILD file for swift-perception seems to be correct

swift_compiler_plugin(
    name = "Sources_PerceptionMacros",
    defines = ["SWIFT_PACKAGE"],
    deps = [
        "@swiftpkg_swift_syntax//:Sources_SwiftSyntaxMacros",
        "@swiftpkg_swift_syntax//:Sources_SwiftCompilerPlugin",
    ],
    module_name = "PerceptionMacros",
    srcs = [
        "Sources/PerceptionMacros/Availability.swift",
        "Sources/PerceptionMacros/Extensions.swift",
        "Sources/PerceptionMacros/PerceptibleMacro.swift",
        "Sources/PerceptionMacros/Plugins.swift",
    ],
    visibility = ["//visibility:public"],
)

cgrindel commented 4 months ago

TBH, I forgot that @jpsim added support for swift_compiler_plugin.

@jpsim @luispadron Do either of you have any thoughts or suggestions on this issue?

luispadron commented 4 months ago

This seems correct to me? If the API is marked unavailable for iOS 17 and you use a min of 17, should we not expect the error?

Do those examples you mentioned using iOS 17 import Observation at all?

iXerol commented 4 months ago

@luispadron They do not use Observation directly, but are using Store / StoreOf or ObservableState. TCA supports both swift-perception and Observation in its unified API.

Actually, if you create a new iOS 17 Xcode project, add the Swift file in TCA Example to the sources, and add swift-composable-architecture dependency using Swift Package, it can build successfully.

luispadron commented 4 months ago

hmm yah I tested it out and it seems to work in Xcode with minimum iOS set to 17.2. I'm not sure here since I do not know much about TCA.

I'd double check the Swift compiler plugins are being used correctly (if they are whats making this possible).

luispadron commented 4 months ago

I think next steps are to create a minimal repro of this, thinking a single swift_library with @available that we can file an issue to rules_swift for. I do not think we're doing anything wrong here in rules_swift_package_manager

iXerol commented 4 months ago

I have found the cause. Xcode and Swift Package Manager are able to build it because the Package.swift in swift-perception specifies the target platforms as follows:

  platforms: [
    .iOS(.v13),
    .macOS(.v10_15),
    .tvOS(.v13),
    .watchOS(.v6),
  ],

It can be built for iOS 13, as Package.swift described, but it cannot be built for iOS 17. However, we cannot determine the platforms in swift_library, the target platform depends on the upper-layer apps/frameworks/tests.

cgrindel commented 4 months ago

So, what do we think are the right next steps for this issue?

iXerol commented 4 months ago

In my opinion, users would hope that as long as a dependency can run successfully in SPM, it can be converted to Bazel through this project. Therefore, I believe we need to find a way to support multiple platforms and specify the target platform version in Package.swift with one library. swift_library cannot specify the target platform and version, so I think it is necessary to use XCFramework. In rules_apple, there are rules support XCFramework, but seems not matching our needs. apple_static_xcframework: It seems like what we need, but it doesn't support transitive swift_library dependencies. apple_xcframework: It requires bundle_id, infoplists, etc.

kersson commented 3 months ago

Hi all. We're running into this issue as well.

More broadly, this seems like a very fundamental difference between the build settings Xcode uses for transitive SPM packages and the settings rules_swift/rules_swift_package_manage uses.

If you create a vanilla Xcode project whose "Minimum Deployment" is "iOS 17.0" and add TCA as a package dependency, then when you build for simulator, for example, the project's direct source files will be compiled with:

-target arm64-apple-ios17.0-simulator
-target-sdk-version 17.4

whereas the TCA sources will be compiled with:

-target arm64-apple-ios13.0-simulator
-target-sdk-version 17.4

But when you build that same project using rules_apple's ios_application with minimum_os_version = "17.0" then all sources, both in the project and all the SPM packages, will be compiled with:

-target arm64-apple-ios17.0-simulator
-target-sdk-version 17.4

And even if you use minimum_os_version = "16.0", you still get tons of compiler warnings about using deprecated APIs in the TCA packages when you build with bazel, but those warnings don't show up when building with Xcode because it compiles those with different -target versions—i.e. the ones specified in the platforms field of the package's Package.swift.

This seems like a fundamental and potentially important difference between rules_swift_package_manager and vanilla Xcode.

Could the copts field of swift_library be used to override the -target platform based on what's specified in the Package.swift? I guess you'd have to use a select to pick the right platform (i.e. simulator vs device)?

Or does this require a more fundamental change to swift_library itself? How doable would something like that be?

kersson commented 3 months ago

I asked about this on the #apple channel of the Bazel Slack workspace, and @brentleyjones had an excellent suggestion of using transitions to apply the correct minimum OS version to the package's swift_librarys. I'll try messing around with that and will report back if we have time to put up a PR.

Edit: he later commented that copts might actually be the way to go since transitions might introduce other bugs as they propagate to transitive dependencies.

kersson commented 3 months ago

Here's my progress update.

Based on the Swift Package Manager SupportedPlatform spec, we need to implement the following behavior:

If a platform version is specified, then we need to use that version, overriding any minimum_os_version specified by the toolchain from the top-level ios_application.
If no platform version is specified, then we need to use the oldest deployment target version that the installed SDK supports for that platform. Except for macOS, whose minimum deployment target version starts from 10.10.

Because of that second requirement to use the SDK-dependent oldest deployment target version, I think the solution to this needs to involve modifications to swift_library and friends. We can't just encode it into the generated repository for the package because the SDK that will be used could change depending on the Xcode version that's selected at runtime. In fact, the apple_support ruleset uses a placeholder value for the SDK directory and only replaces it with the runtime version of SDKROOT as part of a wrapper script. So ultimately, I'm not even sure exactly how to query this information and make it available during the analysis phase, e.g. as part of the toolchain. Perhaps it can be queried and saved as part of the Xcode configuration/discovery code alongside the default_ios_sdk_version, for example? (see [0] and [1])

Here's an example of how you'd query this from the command-line:

jq '.SupportedTargets.iphoneos.MinimumDeploymentTarget' \
    $(xcrun --sdk iphoneos --show-sdk-path)/SDKSettings.json

In any case, let's assume that we'll eventually be able to solve this problem. For now (and for my team's purposes) we can simply hardcode a reasonable oldest support version.

Given this, I've put together a couple patches, one for rules_swift_package_manager and one for rules_swift to provide this functionality.

First, I propose adding a minimum_os_version attribute to swift_library and related rules. If we consider the TCA Swift package with the following platform versions defined in its Package.swift:

  name: "swift-composable-architecture",
  platforms: [
    .iOS(.v13),
    .macOS(.v10_15),
    .tvOS(.v13),
    .watchOS(.v6),
  ],

then ruels_swift_package_manager could generate the following swift_library:

swift_library(
    name = "ComposableArchitecture.rspm",
    minimum_os_version = select({
        "@rules_swift_package_manager//config_settings/spm/platform:ios": "13.0",
        "@rules_swift_package_manager//config_settings/spm/platform:macos": "10.15",
        "@rules_swift_package_manager//config_settings/spm/platform:tvos": "13.0",
        "@rules_swift_package_manager//config_settings/spm/platform:watchos": "6.0",
        "//conditions:default": "oldest",
    }),
    ...
)

Here are the two patches:

You'll see that in rules_swift I have hardcoded the oldest supported versions for macOS and iOS with a TODO that they should be obtained from the MinimumDeploymentTarget field of the platform SDK's SDKSettings file.

@brentleyjones said that he was going to look into this further when he's back from OOO next week.

In the meantime, I hope these patches help unblock anyone stuck on this issue!

Using them I was able to use minimum_os_version = "17.0" in our ios_application that uses TCA. It also nearly eliminated all the compiler warnings in the SPM packages. For us, the only remaining warnings are in TCA and will be fixed when https://github.com/pointfreeco/swift-composable-architecture/pull/2909 is released.

brentleyjones commented 2 months ago

I'm planning on submitting https://github.com/bazelbuild/bazel/compare/bj/fix-capitalization-of-apple-platforms-xcode-sdk-and-os...brentleyjones:bazel:bj/apple_sdk_minimum_os to Bazel. This would allow us to use ctx.attr._xcode_config[apple_common.XcodeProperties] in rules_swift to determine the minimum supported OS for each platform.

@kersson Can you create a local Bazel build with that change, adjust your rules_swift patch, and verify that it works for you?

kersson commented 2 months ago

@brentleyjones here's an updated rules_swift patch that swaps out the hardcoded oldest supported version with a toolchain configurator that uses your new *_sdk_minimum_os attributes:

rules_swift-set-platform-version.patch

Everything still works wonderfully in our TCA ios_application using minimum_os_version = "17.0"!

I can put up a PR for each patch in rules_swift_package_manager and rules_swift, respectively. Let me know when you have a Bazel PR up so I can reference it. I'll also work on adding unit tests for all this.

I'll be on PTO for the next week, so I may not get to this until I get back.

brentleyjones commented 2 months ago

@kersson: https://github.com/bazelbuild/bazel/pull/21991

brentleyjones commented 2 months ago

@kersson see https://github.com/bazelbuild/bazel/pull/21991#issuecomment-2057445522.

It will be a bit before we have a more holistic solution. Once we can make the required changes I'll take your patches here and implement something that uses them (and add you as co-author if you don't mind).

luispadron commented 2 months ago

@brentleyjones were we able to get an estimate of when they think those changes will be made/if they're being prioritized? I get wanting to do it the right way first if possible but this seems like a blocker for wider SPM support

brentleyjones commented 2 months ago

The last I heard was about 5 more weeks.

cgrindel / rules_swift_package_manager

Apps who depends on `swift-composable-architecture` build failed if `minimum_os_version >= 17.0` #892