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

Avoid renameat2 on Android #2627

Merged
merged 1 commit into from Jan 19, 2024
Merged

Avoid renameat2 on Android #2627

merged 1 commit into from Jan 19, 2024

Conversation

glbrntt
Copy link
Contributor

@glbrntt glbrntt commented Jan 17, 2024

Motivation:

Until very recently Android didn't support the renameat2 syscall, this means that on most systems NIO will fail to compile. renameat2 is used to delay the materialization of files on Linux. On Darwin a different code path is used which we can make use of on Android too.

Modifications:

  • Update a few if-defs to force Android onto the alternative path

Result:

NIO builds on Android

@glbrntt
Copy link
Contributor Author

glbrntt commented Jan 17, 2024

@finagolfin could you verify this fixes your build?

@glbrntt glbrntt mentioned this pull request Jan 17, 2024
@finagolfin
Copy link
Contributor

I'll apply it locally and let you know in a little bit, thanks.

@glbrntt glbrntt added the patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1) label Jan 17, 2024
@finagolfin
Copy link
Contributor

This isn't building either, some issues with optionals, will try modifying this patch and let you know within an hour.

@glbrntt
Copy link
Contributor Author

glbrntt commented Jan 17, 2024

This isn't building either, some issues with optionals, will try modifying this patch and let you know within an hour.

Suspect you'll need this commit too: 600a210

@finagolfin
Copy link
Contributor

Another problem is that Bionic doesn't have confstr() at all:

/home/swift-nio/Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift:389:12: error: cannot find 'confstr' in scope
    return confstr(name, buffer, size)                                                                                                                                        ^~~~~~~

I think this pull works to fix the renameat2 issue, but then other compilation errors from your large pull pop up on Android, looking into them now.

@glbrntt
Copy link
Contributor Author

glbrntt commented Jan 17, 2024

Another problem is that Bionic doesn't have confstr() at all:

/home/swift-nio/Sources/NIOFileSystem/Internal/System Calls/Syscalls.swift:389:12: error: cannot find 'confstr' in scope
    return confstr(name, buffer, size)                                                                                                                                        ^~~~~~~

We can workaround confstr too as it looks like we're only using it on Darwin.

I think this pull works to fix the renameat2 issue, but then other compilation errors from your large pull pop up on Android, looking into them now.

Great, thanks for looking!

@finagolfin
Copy link
Contributor

I don't see other issues related to renameat2, so I think this pull works. If you can ifdef confstr() out on Android too, please go ahead and merge these changes.

I will work through the remaining compilation issues related to optionals, because the latest Android NDK added nullability annotations all over Bionic that your pull will have to be slightly modified for, and submit another pull for that later.

@glbrntt
Copy link
Contributor Author

glbrntt commented Jan 17, 2024

I don't see other issues related to renameat2, so I think this pull works. If you can ifdef confstr() out on Android too, please go ahead and merge these changes.

Great, will do!

I will work through the remaining compilation issues related to optionals, because the latest Android NDK added nullability annotations all over Bionic that your pull will have to be slightly modified for, and submit another pull for that later.

Do you have any timeline on this? We'd like to tag a release and would rather not break you!

from: createdPath,
to: desiredPath,
options: materialization.exclusive ? [.exclusive] : []
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Turns out this doesn't compile on Android, because rename(from:to:options:) is only defined on Darwin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, yes, this variant is Darwin only as it has the .exclusive flag. I think we can avoid this by not supporting "atomic" file creation on Android when exclusive create is set (when it isn't set we can do a normal rename without the exclusive flag).

@finagolfin
Copy link
Contributor

Do you have any timeline on this?

Most likely later today.

We'd like to tag a release and would rather not break you!

I mostly build trunk right now, so shouldn't be an issue.

@finagolfin
Copy link
Contributor

Applied this locally and all the remaining errors appear to be type and optional mismatches related to slightly different Android declarations, so you can go ahead and merge this when you like.

@glbrntt glbrntt marked this pull request as ready for review January 17, 2024 15:57
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

I just want to sync up here a bit: musl also lacks renameat2 as a libc function, but it does have it as a system call. I'm thinking that we'll want to omit the call through libc and instead make the system call directly. @finagolfin do we think that's gonna cause any problems on android?

@finagolfin
Copy link
Contributor

No, I think that should work, as I don't think that syscall is filtered out on Android.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 18, 2024

See #2628

@finagolfin
Copy link
Contributor

Tried that Musl syscall pull out, seems to work well after adding my patch for the type/optional mismatches and the confstr() commit from this pull, except I see several tests failing because they call linkat(), not sure where that's coming from.

@finagolfin
Copy link
Contributor

I will look into the linkat() testing issues on Android in the next couple days. Meanwhile, if @glbrntt would reduce this pull to the confstr() commit and merge, that would be helpful.

Copy link
Contributor

@finagolfin finagolfin left a comment

Choose a reason for hiding this comment

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

Thanks

@glbrntt glbrntt requested a review from Lukasa January 19, 2024 09:10
@Lukasa Lukasa merged commit 635b258 into apple:main Jan 19, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-version-bump-only For PRs that when merged will only cause a bump of the patch version, ie. 1.0.x -> 1.0.(x+1)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants