[Android] Add shims for getgrnam_r and getgrgid_r for API 23#1663
[Android] Add shims for getgrnam_r and getgrgid_r for API 23#1663jmschonfeld merged 5 commits intoswiftlang:mainfrom
getgrnam_r and getgrgid_r for API 23#1663Conversation
|
This isn't a polyfill, because it will be compiled in if the target API when compiling the SDK itself is 23 or earlier, regardless of the eventual Android runtime environment. These group functions seem pretty much useless on Android though, so I'd have been fine if you had just stubbed them out when compiling against such an old target API, but now that you've done this work, seems fine to include it, 👍 if the swift-foundation reviewers approve. |
|
@swift-ci test |
madsodgaard
changed the title
getgrnam_r and getgrgid_r for API 23getgrnam_r and getgrgid_r for API 23
|
@parkera, could you have someone take a look? |
| #include <string.h> | ||
| #include <errno.h> | ||
|
|
||
| static inline int _filemanager_shims_getgrgid_r(gid_t gid, struct group *grp, |
There was a problem hiding this comment.
Any reason why we can't do call these code directly in Swift? Yes, there will be some boilerplates, but I'm not sure if we should be adding code to these
There was a problem hiding this comment.
Yeah, in general this patch seems ok to me, but the goal of this project is to write these implementations in Swift rather than C, so unless we have a good reason to write this in C (because Swift cannot call these functions for some reason) then writing the shim in Swift seems like a better approach to me
There was a problem hiding this comment.
How do you propose we write this in Swift? This was written in C because these APIs are only unavailable for Android API 23 and before, so either you'd have to mimic the #if TARGET_OS_ANDROID && __ANDROID_API__ <= 23 guard above with a compile-time API version check in Swift, which the language does not provide so we'd have to pass it in manually through the Package.swift manifest and CMake config, or use the new #available(Android, ) feature, which we can't yet because it requires NDK 28 or later.
I believe Mads added these as C shims for those reasons and because he noticed these C shims here already. What we could do is get this in for now, then revisit later in Swift once #available(Android, ) is working with a new NDK on the official Android CI.
There was a problem hiding this comment.
Would be good to get this small pull in, let me know what you think, @itingliu.
There was a problem hiding this comment.
That sounds reasonable. Thanks
There was a problem hiding this comment.
Would you mind filing an issue for revisiting this to use #available(Android, ) when it's ready?
|
@swift-ci please test |
| grp->gr_passwd = (char *)""; | ||
| grp->gr_mem = NULL; |
There was a problem hiding this comment.
I know we don't use it right now, but should these two be copied over for consistency? Or does android define these as never set for one reason or another?
There was a problem hiding this comment.
The official Bionic impl does not set these fields. For passwd, its basically a null-pointer and for the group member list, its just a "synthetic list" of the group name and a null pointer
https://android.googlesource.com/platform/bionic/+/refs/heads/main/libc/bionic/grp_pwd.cpp#83
The passwd would be simple enough to copy as well, but the group members would require a bit more complex logic to deep copy all the strings. So, since Foundation does not use these and Bionic does not, I think leaving them out, seems like the best option.
I added a comment in the code.
There was a problem hiding this comment.
Thanks, the added comment is helpful to clarify that. It looks like you changed the returned gr_passwd to an empty string "for safety" - could you elaborate on what safety you're trying to guarantee here? I know we don't use it right now, but returning the group password as an empty string seems like it would be prone to issues if we did decide to use it (I wouldn't want us to end up performing any form of string comparison against that empty string for example). It seems better to me to leave this as setting it to NULL to preserve what the OS does instead of providing a fake value that doesn't match the OS behavior.
There was a problem hiding this comment.
Agreed. I've updated the shim to explicitly set gr_passwd to NULL
|
@swift-ci please test |
|
@swift-ci please test macOS |
|
@swift-ci test |
| return 0; | ||
| } | ||
|
|
||
| #elif !TARGET_OS_WINDOWS && !TARGET_OS_WASI |
There was a problem hiding this comment.
@madsodgaard would you be able to add a __has_include(<grp.h>) to this condition (or use that instead if neither Windows nor WASI has the header)? That would be a little more robust and ensures that this doesn't trip up anything in special circumstances like WASI where the API doesn't exist and is already excluded on the calling side?
There was a problem hiding this comment.
Sure! I replaced the conditions by the _has_include. Seems neither WASM nor Windows has the header
|
@swift-ci test |
5 participants
Adds simple shims for these two functions, such that
swift-foundationcan compile on API 23.Motivation:
Currently,
swift-foundationdoes not compile on Android API 23, because ofgetgrnam_randgetgrgid_r, which is only available on API 24.This is the only thing that was missing to make this repo compile on API 23.
An alternative solution, was guarding the calls to these with
if #available(Android 24, *), but I am not sure if we should "fail" silently in that case, and just not append the GID to the file on API 23. So, I guess this solution is at least better. Also, the shims are quite simple, since Foundation only needs the name and GID.