-
Notifications
You must be signed in to change notification settings - Fork 32
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
Comments
I have dug into this. However, I noticed that |
But the generated BUILD file for 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"],
) |
TBH, I forgot that @jpsim added support for @jpsim @luispadron Do either of you have any thoughts or suggestions on this issue? |
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? |
@luispadron They do not use Observation directly, but are using Store / StoreOf or ObservableState. TCA supports both 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. |
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). |
I think next steps are to create a minimal repro of this, thinking a single |
I have found the cause.
It can be built for iOS 13, as |
So, what do we think are the right next steps for this issue? |
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 |
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 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:
whereas the TCA sources will be compiled with:
But when you build that same project using
And even if you use This seems like a fundamental and potentially important difference between Could the Or does this require a more fundamental change to |
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 Edit: he later commented that |
Here's my progress update. Based on the Swift Package Manager
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 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 First, I propose adding a name: "swift-composable-architecture",
platforms: [
.iOS(.v13),
.macOS(.v10_15),
.tvOS(.v13),
.watchOS(.v6),
], then 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 @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 |
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 @kersson Can you create a local Bazel build with that change, adjust your rules_swift patch, and verify that it works for you? |
@brentleyjones here's an updated rules_swift patch that swaps out the hardcoded oldest supported version with a toolchain configurator that uses your new Everything still works wonderfully in our TCA 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. |
@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). |
@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 |
The last I heard was about 5 more weeks. |
Does anyone know what the next steps are to get this done? |
|
Does anyone have suggestions for reasonable short-term workarounds? I'm looking to set Patching either rulesets or TCA is fine if it the patch scope can be relatively small. |
Here are my updated patches that should apply to the latest rules_swift-set-platform-version.patch |
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) |
I'm not sure whether it's a
rules_swift_package_manager
issue orrules_swift
issue.swift-composable-architecture
has a dependencyswift-perception
.swift-perception
is a backport library forObservation
. SinceObservation
requires iOS 17+, some API inswift-perception
are marked obsoleted on iOS 17. eg.Bindable
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.While the minimum deployments versions of some TCA examples are also >= 17.0, and they can build successfully in Xcode & Swift Packages.
The text was updated successfully, but these errors were encountered: