Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Import new Bionic module from Android overlay instead where possible #7755

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Jul 4, 2024

Also, add the import in Environment.swift too

@rauhul, you added that last file just after I got these imports in with #7615, so I recently had to add this too to keep it building on Android.

@@ -19,6 +19,8 @@ import Musl
#elseif os(Windows)
import CRT
import WinSDK
#elseif canImport(Android)
import Android
Copy link
Member

@compnerd compnerd Jul 4, 2024

Choose a reason for hiding this comment

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

Can we use Bionic here instead? Note that Darwin is using Darwin.C and Windows is using CRT. Of course if we need more than the C interfaces, Android is fine, but would be nice to keep this scoped to the same level as the other platforms.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll check if that's enough.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Also, add the import in `Environment.swift` too
@finagolfin finagolfin changed the title Import new Android overlay in Environment too Import new Bionic module from Android overlay instead where possible Jul 8, 2024
@finagolfin
Copy link
Member Author

This now makes the changes suggested in #7752, along with the original change here modified to Bionic. All except one test fixture from #7615 worked with import Bionic.

@compnerd, please review, since you asked for this change.

@finagolfin
Copy link
Member Author

@rauhul, a CI run and we can get this in.

@bnbarham
Copy link
Contributor

bnbarham commented Jul 8, 2024

@swift-ci please test

@finagolfin
Copy link
Member Author

@kateinoigakukun, passed CI, ready for merge.

@bnbarham bnbarham merged commit 02f37e4 into swiftlang:main Jul 9, 2024
5 checks passed
@finagolfin finagolfin deleted the import branch July 9, 2024 16:18
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants