Skip to content

[Android] Use the Bionic module in more places #842

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Aug 15, 2024
Merged

[Android] Use the Bionic module in more places #842

merged 1 commit into from
Aug 15, 2024

Conversation

finagolfin
Copy link
Member

Also, use canImport() wherever importing APIs, reserving os(Android) for platform differences.

I've started porting this repo to Android: this is a first pull that keeps everything building while more narrowly scoping imports.

@compnerd and @hyp, try it out and let me know what you think.

finagolfin
finagolfin commented Aug 11, 2024
View reviewed changes
Sources/FoundationEssentials/FileManager/FileManager+Directories.swift
@@ -18,7 +18,7 @@ internal import os

#if canImport(Darwin)
import Darwin
#elseif os(Android)
#elseif canImport(Android)
import Android
import unistd
Copy link
Member Author

Choose a reason for hiding this comment

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

Isn't this redundant since the prior Android overlay includes it?

finagolfin
finagolfin commented Aug 11, 2024
View reviewed changes
Sources/FoundationEssentials/FileManager/FileManager+Files.swift
@@ -17,7 +17,7 @@ internal import DarwinPrivate.sys.content_protection

#if canImport(Darwin)
import Darwin
#elseif os(Android)
#elseif canImport(Android)
import Android
import posix_filesystem
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

finagolfin
finagolfin commented Aug 11, 2024
View reviewed changes
Sources/FoundationEssentials/FileManager/FileManager+SymbolicLinks.swift
@@ -12,7 +12,7 @@

#if canImport(Darwin)
import Darwin
#elseif os(Android)
#elseif canImport(Android)
import Android
import unistd
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

finagolfin
finagolfin commented Aug 11, 2024
View reviewed changes
Sources/FoundationEssentials/FileManager/FileOperations+Enumeration.swift
@@ -109,7 +109,7 @@ struct _Win32DirectoryContentsSequence: Sequence {

#if canImport(Darwin)
import Darwin
#elseif os(Android)
#elseif canImport(Android)
import Android
import posix_filesystem.dirent
Copy link
Member Author

Choose a reason for hiding this comment

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

Same here

iCharlesHu
iCharlesHu reviewed Aug 13, 2024
View reviewed changes
Sources/FoundationEssentials/Platform.swift
@@ -28,7 +28,7 @@ fileprivate let _pageSize: Int = {
#elseif os(WASI)
// WebAssembly defines a fixed page size
fileprivate let _pageSize: Int = 65_536
#elseif os(Android)
#elseif canImport(Android)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that we check canImport(Android) but actually importing Bionic below? Do canImport(Android) and canImport(Bionic) always return the same value on Android?

I agree with you that using canImport() is better than os(), but it seems we are using canImport(Android) and canImport(Bionic) interchangeably in place of os(Android). Can we standardize that to one?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it intentional that we check canImport(Android) but actually importing Bionic below?

Yes, as unistd comes from the Android overlay. I did not add those imports, so I merely used the most appropriate canImport() here, the one that includes them both, since Android is a superset of Bionic.

Do canImport(Android) and canImport(Bionic) always return the same value on Android?

Yes, they are interchangeable, as they are both defined in the same module map. However, import Bionic and import Android are not interchangeable, so I simply choose the canImport() to match the one we are actually importing.

we are using canImport(Android) and canImport(Bionic) interchangeably... Can we standardize that to one?

No need to, as they are interchangeable and one is simply chosen to match the non-interchangeable import used.

Sources/FoundationEssentials/ProcessInfo/ProcessInfo.swift
@@ -14,7 +14,7 @@ internal import _FoundationCShims

#if canImport(Darwin)
import Darwin
#elseif os(Android)
#elseif canImport(Android)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Same explanation as above

Sources/FoundationEssentials/_ThreadLocal.swift Outdated
@@ -24,7 +24,7 @@ internal import threads
#endif

struct _ThreadLocal {
#if canImport(Darwin) || os(Android) || canImport(Glibc)
#if canImport(Darwin) || canImport(Bionic) || canImport(Glibc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we are using canImport(Bionic) in place of os(Android) but we are using canImport(Android) above.

Copy link
Member Author

Choose a reason for hiding this comment

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

Unsure what you're referring to, this entire file uses canImport(Bionic) after this pull.

@compnerd
Copy link
Member

os(Android) does seem reasonable as the check to me as the libc (bionic) is part of the OS and we should be able to assume that the Android overlay is available.

@finagolfin
Copy link
Member Author

os(Android) does seem reasonable as the check to me as the libc (bionic) is part of the OS and we should be able to assume that the Android overlay is available.

It will work, but it is not correct as an indicator of what is happening. While both may be functionally interchangeable, we should be consistent with other platforms and only use canImport() when selecting platform C APIs and os(Android) for platform differences, as the few remaining uses of the latter I left do.

@finagolfin
Copy link
Member Author

Rebased and fixed merge conflict.

@iCharlesHu
Copy link
Contributor

@swift-ci please test

@finagolfin
[Android] Use the Bionic module in more places
d5613bc 
Also, use `canImport()` wherever importing APIs, reserving `os(Android)` for
platform differences.
@finagolfin
Copy link
Member Author

Rebased again and fixed merge conflicts.

@iCharlesHu
Copy link
Contributor

@swift-ci please test

@iCharlesHu iCharlesHu merged commit c15a5e1 into swiftlang:main Aug 15, 2024
3 checks passed
@finagolfin finagolfin deleted the import branch August 16, 2024 05:10
@finagolfin
Copy link
Member Author

Thanks @iCharlesHu, I was just able to run the tests for this repo natively on Android for the first time. 😄

Only five tests failed- two FileManager tests and three ProcessInfo tests- I will look into those and submit a pull for those next, before trying to build and run the full re-cored swift-corelibs-foundation tests natively on Android.

cthielen pushed a commit to cthielen/swift-foundation that referenced this pull request Nov 8, 2024
@finagolfin @cthielen
[Android] Use the Bionic module in more places (swiftlang#842)
d8f9dbd 
Also, use `canImport()` wherever importing APIs, reserving `os(Android)` for
platform differences.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iCharlesHu iCharlesHu iCharlesHu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@finagolfin @compnerd @iCharlesHu