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

Update OS and arch grammar according the open source Swift repo #225

Closed
wants to merge 1 commit into from

Conversation

Kyle-Ye
Copy link
Contributor

@Kyle-Ye Kyle-Ye commented Dec 28, 2023

Context:

I was adding some #if os(xx) to detect the pointer size on my package, and they failed to build on watchOS platform. I realized I have missed some arch for it. (eg. armv7k and arm64_32)

I checked the grammar page on TSPL. They are both not supported according to TSPL grammar here. And actually arm64_32 is supported and armv7k is not which will hit os(arm) here.

This PR updates the corresponding grammar rule according to https://github.com/apple/swift

I did not find the exact grammar parsing logic so there may be some missing item. But I have checked what I list here are all supported on Swift 5.9

visionOS is not added on upstream open source Swift yet. And they are only supported on Xcode's Swift toolchain. (eg. Xcode 14.0)

Building the following code on Linux with the latest Swift release(Swift 5.9.2) will give us a warning here.

#if os(visionOS)
...
#endif
DemoKit.swift:12:8: note: did you mean 'iOS'?
#if os(visionOS)
       ^~~~~~~~
       iOS

For Arch, I use the following source

For OS, I use the following source

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Dec 28, 2023

What's your idea on this? cc @amartini51

@amartini51
Copy link
Collaborator

I don't think we should make this change. As noted in the in-source comment below this grammar block:

Some of the OSes and architectures are listed [in the compiler's source] because there's experimental work to port Swift to them. We won't list them here until they're officially supported.

Are there officially supported OSes or architectures that are missing from TSPL's list?

It might be ok to add OSX which is a compatibility alias for macOS, but that probably needs some discussion.

As an aside: This PR would be easier to review if you added things first, and then re-ordered them in a separate commit.

@amartini51 amartini51 self-requested a review January 2, 2024 18:06
@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Jan 3, 2024

Answers

Are there officially supported OSes or architectures that are missing from TSPL's list?

I can see arm64_32 should be added as they are part of watchOS SDK.

As an aside: This PR would be easier to review if you added things first, and then re-ordered them in a separate commit.

Got it. The order is kept as how they appear in the source repository.

I'll update to keep the original item order and add other items after them in a separate commit.

Questions

Here is my questions:

  1. When people see a build failure on watchOS(due to the missing arch check), what is the expected site for people to check the arch grammer?

IMO They should either go to Apple's documentation or TSPL to check for all the possible arches Swift support to update their code instead of digging into the Swift(Swift Compiler) repo.

https://github.com/OpenCombine/OpenCombine/blob/1c6f02c7ed8140c0ba7a783aaddb6e0685a0037b/Tests/OpenCombineTests/DispatchTests/DispatchQueueSchedulerTests.swift#L245-L259

  1. We have content (eg. visionOS) listed in TSPL as if they are supported Swift grammar of 5.9.2. But they are actually only accepted by Apple's private Swift fork / Xcode bundled one instead of the open source one hosted on apple/swift

Building the following code on Linux with the latest Swift release(Swift 5.9.2) will give us a warning here.

#if os(visionOS)
...
#endif
DemoKit.swift:12:8: note: did you mean 'iOS'?
#if os(visionOS)
       ^~~~~~~~
       iOS

I can't find anything string about visionOS here in apple/swift. I guess it is reasonable since it is still beta and no release version of Xcode has visionSDK too.

https://docs.swift.org/swift-book/documentation/the-swift-programming-language/summaryofthegrammar#Statements

image

I think the content in TSPL should be aligned with apple/swift. If Apple has not merged such patch, we should not present it here too.

Copy link
Collaborator

@amartini51 amartini51 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only OSes and architectures that are officially supported should be listed.

@amartini51
Copy link
Collaborator

I'm getting confirmation on whether arm64_32 is officially supported. If it is, we should add it here.

OSX was intentionally removed from the list in 1f4cffc

@Kyle-Ye
Copy link
Contributor Author

Kyle-Ye commented Jan 6, 2024

Update:

  1. visionOS issue was clarified here Add visionOS extension to platform lists #227 (comment)

Only OSes and architectures that are officially supported should be listed.

For OSes: Only Apple's OS, Linux and Windows are officially supported. So I'll remove the others.

For architectures, i386, x86_64, arm and arm64 are officially supported. But if we only use those arch to make #if arch(xx) logic, we'll hit a uncovered case for watchOS platform. The missing one is arm64_32 and @amartini51 is confirming with it. Other arches should be removed since they are never officially supported.

@amartini51
Copy link
Collaborator

Initial feedback is that we shouldn't add arm64_32 and maybe we shouldn't even have this list in TSPL. I'll close this PR for now, and I've filed a GitHub issue to track this.

#245

@amartini51 amartini51 closed this Jan 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants