Skip to content

Conversation

@noahpistilli
Copy link
Contributor

Adds compilation support for visionOS. All tests pass. Closes #150.

I was unsure if I had to edit Utilities/expand-availability.py, so I didn't touch it.

@glessard
Copy link
Contributor

I would prefer to do this by setting a compilation variable in the package manifest. This os||os||os||os||os list was already unwieldy, and it's only getting worse.

@noahpistilli
Copy link
Contributor Author

noahpistilli commented Feb 27, 2024

Perhaps something like

swiftSettings: [
      .define("APPLE", .when(platforms: [Platform.macOS, Platform.iOS, Platform.watchOS, Platform.tvOS, Platform.visionOS])),

in the package manifest?

@rauhul
Copy link
Contributor

rauhul commented Feb 27, 2024

Perhaps something like

swiftSettings: [

      .define("APPLE", .when(platforms: [Platform.macOS, Platform.iOS, Platform.watchOS, Platform.tvOS, Platform.visionOS])),

in the package manifest?

If you do go this route, note that Apple has non Darwin uses of Swift such as the triple: armv7em-apple-none-macho.

You could name this define DARWIN instead?

@noahpistilli noahpistilli force-pushed the visionPro branch 2 times, most recently from 73de625 to dda06cd Compare February 28, 2024 19:17
Copy link
Contributor

@glessard glessard left a comment

Choose a reason for hiding this comment

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

This is great, thanks for helping with this!

@glessard
Copy link
Contributor

glessard commented Mar 5, 2024

@swift-ci please test

@glessard
Copy link
Contributor

glessard commented Mar 5, 2024

The testing failure involves a change in Swift 6.0. Will need to fix this separately.

@glessard
Copy link
Contributor

glessard commented Mar 8, 2024

@noahpistilli I think the approach is good, but the symbol "DARWIN" may be a bit too prone to collisions, given that it could possibly be passed from the command-line by any package that uses this. May I suggest something less likely have collisions, such as "SYSTEM_PACKAGE_DARWIN"?

@glessard
Copy link
Contributor

glessard commented Mar 8, 2024

@swift-ci please test

@karwa
Copy link

karwa commented Mar 8, 2024

Wouldn't it be better to use canImport(Darwin)?

Especially since a lot of these are literally guarding imports of the Darwin module and uses of symbols exported by it...

@rauhul
Copy link
Contributor

rauhul commented Mar 8, 2024

Wouldn't it be better to use canImport(Darwin)?

Especially since a lot of these are literally guarding imports of the Darwin module and uses of symbols exported by it...

This seems abundantly obvious now that you say it...

@glessard
Copy link
Contributor

glessard commented Mar 8, 2024

@swift-ci please test

@glessard
Copy link
Contributor

glessard commented Mar 8, 2024

#if canImport() has a namespacing issue; nothing particular prevents a Linux distribution from having a module named Darwin. We've avoided using that approach for that reason. It would be nicer to be able to check specifically for the thing we need, and to be honest in this package Darwin isn't it.

@karwa
Copy link

karwa commented Mar 8, 2024

#if canImport() has a namespacing issue; nothing particular prevents a Linux distribution from having a module named Darwin. We've avoided using that approach for that reason.

It’s only an issue if this package were to introduce a dependency on another package named “Darwin”, or if a third-party toolchain were to include a Swift module named “Darwin” in its standard installation.

That’s sufficiently unlikely that I don’t think it needs to be a concern. Such a toolchain would quickly encounter widespread problems across the Swift ecosystem and would likely choose a different name.

Importantly, the official toolchain would not encounter issues when building for Apple platforms. That’s the only thing I think we need to be concerned about right now.

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.

Does not compile for visionOS

4 participants