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

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

Open
iXerol opened this issue Feb 1, 2024 · 24 comments
Labels
enhancement New feature or request

Comments

@iXerol
Copy link

iXerol commented Feb 1, 2024

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 cgrindel added bug Something isn't working enhancement New feature or request and removed bug Something isn't working labels Feb 1, 2024
@cgrindel
Copy link
Owner

cgrindel commented Feb 1, 2024

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
Copy link
Author

iXerol commented Feb 1, 2024

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
Copy link
Owner

cgrindel commented Feb 4, 2024

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
Copy link
Collaborator

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
Copy link
Author

iXerol commented Feb 4, 2024

@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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

iXerol commented Feb 8, 2024

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
Copy link
Owner

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

@iXerol
Copy link
Author

iXerol commented Mar 4, 2024

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
Copy link

kersson commented Mar 27, 2024

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
Copy link

kersson commented Mar 27, 2024

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
Copy link

kersson commented Mar 29, 2024

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 pointfreeco/swift-composable-architecture#2909 is released.

@brentleyjones
Copy link
Collaborator

brentleyjones commented Apr 4, 2024

I'm planning on submitting bazelbuild/bazel@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
Copy link

kersson commented Apr 6, 2024

@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:

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
Copy link
Collaborator

@kersson: bazelbuild/bazel#21991

@brentleyjones
Copy link
Collaborator

@kersson see bazelbuild/bazel#21991 (comment).

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
Copy link
Collaborator

@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
Copy link
Collaborator

The last I heard was about 5 more weeks.

@jpsim
Copy link
Collaborator

jpsim commented Aug 8, 2024

Does anyone know what the next steps are to get this done?

@brentleyjones
Copy link
Collaborator

  1. Depends on Bazel 8
  2. Need to do this and then merge that PR
  3. Then we need to create something like what is described in Add *_sdk_minimum_os attributes to xcode_version and XcodeProperties bazelbuild/bazel#21991 (comment)

@jpsim
Copy link
Collaborator

jpsim commented Sep 27, 2024

Does anyone have suggestions for reasonable short-term workarounds? I'm looking to set minimum_os_version = 17.0 for my project in the next week so I need a more short-term solution that what's outlined above.

Patching either rulesets or TCA is fine if it the patch scope can be relatively small.

@kersson
Copy link

kersson commented Sep 27, 2024

Here are my updated patches that should apply to the latest rules_swift (2.1.1) and rules_swift_package_manager (0.39.0). We've been using them successfully. It's a pain to keep maintaining them, but it sounds like upstreaming them depends on a lot of stuff that won't be ready for a while 😞

rules_swift-set-platform-version.patch
rules_swift_package_manager-set-platform-version.patch

@jpsim
Copy link
Collaborator

jpsim commented Sep 27, 2024

Thanks @kersson, that's really helpful. I also needed this small change:

diff --git a/config_settings/spm/platform/platforms.bzl b/config_settings/spm/platform/platforms.bzl
index 3766536..6fc29df 100644
--- a/config_settings/spm/platform/platforms.bzl
+++ b/config_settings/spm/platform/platforms.bzl
@@ -67,6 +67,8 @@ def _label(name):
         fail("Unsupported swift package manager platform: maccatalyst.")
     if name == "driverkit":
         name = "macos"
+    if name == "visionOS":
+        name = "visionos"
     return "@rules_swift_package_manager//config_settings/spm/platform:{}".format(name)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

6 participants