[android] fix the android build#757
Conversation
|
@swift-ci please test |
|
@swift-ci please test |
| guard let stream = fts_open(dirList.baseAddress!, opts, nil) else { | ||
| state = [Optional(UnsafeMutablePointer(mutating: path)), nil].withUnsafeBufferPointer { dirList in | ||
| #if canImport(Android) | ||
| let baseAddress = unsafeBitCast(dirList.baseAddress!, to: UnsafePointer<UnsafeMutablePointer<CChar>>.self) |
There was a problem hiding this comment.
Could you elaborate a bit more on what error this change resolves? I would have assumed that the above optional wrapping wouldn't be necessary since it'd be inferred by the compiler, and it seems that this unsafeBitCast of an optional pointer to a non-optional pointer would be a trap waiting for a caller that tries to read that final nil element, right?
There was a problem hiding this comment.
This is needed because the Android NDK has a nullability annotation on fts_open:
FTS* _Nullable fts_open(char* _Nonnull const* _Nonnull __path, int __options, int (* _Nullable __comparator)(const FTSENT* _Nonnull * _Nonnull __lhs, const FTSENT* _Nonnull * _Nonnull __rhs)) __INTRODUCED_IN(21);
This makes the path pointer nonnullable . Therefore the type inference is then unable to infer the type for the array, and thus I need to explicitly make the first element optional, so that the second one can be null. Then the pointer need to be cast to match the type, as we still want to have a nil element in the array, but have non null pointer types.
There was a problem hiding this comment.
In general this stems from the fact that this specific nullability annotation in the NDK is probably incorrect, as fts_open is documented to take an array that has NULL as the last element.
There was a problem hiding this comment.
I added a comment in source.
There was a problem hiding this comment.
This change should no longer be needed after swiftlang/swift#74829, as noted on your earlier commit weeks ago.
There was a problem hiding this comment.
Good point, I need to retry with that change.
There was a problem hiding this comment.
Note the additional linux change that I mentioned there, we'll need to do something like that for that change to register on the linux CI.
There was a problem hiding this comment.
Oh wait, that pull isn't needed at all on the linux CI, as this function signature only varies in Bionic.
In that case, this pull can just be closed, if #714 gets macros working and another small apinotes pull avoids the need for this change.
|
@swift-ci please test |
|
I think that we should consider #714 as an alternative here as that is a broader fix. |
|
This now drops the fts_open change - as posix_filesystem.apinotes are now picked up correctly : swiftlang/swift#75494 |
Macros can't yet be built as there's no host build support yet
|
@swift-ci please test |
1 similar comment
|
@swift-ci please test |
|
Ok, macros work for android with: compnerd#1 If we land host macros first, we can drop this PR. wdyt @compnerd, or should this land first . |
|
I talked to @compnerd and I think we can hold off from this one, as the host macros changes should land soon. |
|
@hyp I think that this can be closed now? |
|
I think the changes here are either no longer necessary or have been applied via other PRs so I'm going to go ahead and close this. If any of these changes are still relevant, feel free to update the PR and re-open |
4 participants
Macros can't yet be built as there's no host build support yet