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

Fix building on Android by avoiding getpwent() #3441

Merged
merged 4 commits into from Oct 16, 2016

Conversation

Projects
None yet
4 participants
@fornwall
Contributor

fornwall commented Oct 10, 2016

The getpwent() function does not link when building for Android, and user names on that platform are not interesting anyway.

This patch is part of an effort to upstream patches used by the fish package in Termux.

Fix building on Android by avoiding getpwent()
The getpwent() function does not link when building for Android,
and user names on that platform are not interesting anyway.
@floam

I don't really like the "return 0" thing going on here - and there still should be usernames on Android, interesting or not. I'd like if you checked getpwent linkability in autoconf and provided a workaround of some sort, if practical.

Show outdated Hide outdated src/complete.cpp
@@ -1187,6 +1187,10 @@ bool completer_t::try_complete_variable(const wcstring &str) {
///
/// \return 0 if unable to complete, 1 otherwise
bool completer_t::try_complete_user(const wcstring &str) {
#ifdef __ANDROID__
// The getpwent() call does not exist on Android (and user names are not interesting).
return 0;

This comment has been minimized.

@krader1961

krader1961 Oct 11, 2016

Contributor

While this is strictly speaking correct and functionally equivalent to return false; please do the latter for clarity. Having said that, I agree with @floam that since Android is derived from Linux it presumably does utilize UIDs and we should therefore be returning something meaningful on that platform if that is possible. If everything doesn't run as UID zero, and there is no other way to map a UID to name, perhaps this should simply synthesize a user name by doing something like return sprintf(buf, "user#%d", getuid());. That is obviously just pseudo-code but you get the idea.

@krader1961

krader1961 Oct 11, 2016

Contributor

While this is strictly speaking correct and functionally equivalent to return false; please do the latter for clarity. Having said that, I agree with @floam that since Android is derived from Linux it presumably does utilize UIDs and we should therefore be returning something meaningful on that platform if that is possible. If everything doesn't run as UID zero, and there is no other way to map a UID to name, perhaps this should simply synthesize a user name by doing something like return sprintf(buf, "user#%d", getuid());. That is obviously just pseudo-code but you get the idea.

@fornwall

This comment has been minimized.

Show comment
Hide comment
@fornwall

fornwall Oct 12, 2016

Contributor

Thanks a lot for the feedback! I've updated from return 0 to return false.

As for a fallback, a Linux user on Android isn't really a user - each installed app gets an UID assigned. Listing all UID:s is not possible without root access, and doing a ~USER type expansion does not make sense since every app is sandboxed and can't access eachother. So I think a fallback is not really possible.

As for an autoconf check if getpwent links instead of ifdef __ANDROID__: Sounds reasonable, can do that after we settle for the fallback behaviour.

Contributor

fornwall commented Oct 12, 2016

Thanks a lot for the feedback! I've updated from return 0 to return false.

As for a fallback, a Linux user on Android isn't really a user - each installed app gets an UID assigned. Listing all UID:s is not possible without root access, and doing a ~USER type expansion does not make sense since every app is sandboxed and can't access eachother. So I think a fallback is not really possible.

As for an autoconf check if getpwent links instead of ifdef __ANDROID__: Sounds reasonable, can do that after we settle for the fallback behaviour.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 13, 2016

Contributor

Okay, given that in Android OS there really isn't any such thing as usernames I'm okay with just returning false for that completer. Go ahead and make the behavior dependent on whether there is a usable getpwent() and I think you're done.

Contributor

krader1961 commented Oct 13, 2016

Okay, given that in Android OS there really isn't any such thing as usernames I'm okay with just returning false for that completer. Go ahead and make the behavior dependent on whether there is a usable getpwent() and I think you're done.

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 13, 2016

Member

I found this stuff interesting:

http://androidxref.com/4.2.2_r1/xref/bionic/libc/docs/OVERVIEW.TXT#231

http://androidxref.com/5.0.0_r2/xref/bionic/libc/bionic/stubs.cpp#357

Maybe we could do similar. There are some mappings defined in a header file as well.

Member

floam commented Oct 13, 2016

I found this stuff interesting:

http://androidxref.com/4.2.2_r1/xref/bionic/libc/docs/OVERVIEW.TXT#231

http://androidxref.com/5.0.0_r2/xref/bionic/libc/bionic/stubs.cpp#357

Maybe we could do similar. There are some mappings defined in a header file as well.

@krader1961

This comment has been minimized.

Show comment
Hide comment
@krader1961

krader1961 Oct 14, 2016

Contributor

For the record I would prefer an autoconf check for whether getpwent() exists. But if that is impractical for some reason the current implementation is acceptable.

Contributor

krader1961 commented Oct 14, 2016

For the record I would prefer an autoconf check for whether getpwent() exists. But if that is impractical for some reason the current implementation is acceptable.

@floam

floam approved these changes Oct 16, 2016

@floam

This comment has been minimized.

Show comment
Hide comment
@floam

floam Oct 16, 2016

Member

Great, we can merge this.

Member

floam commented Oct 16, 2016

Great, we can merge this.

@fornwall

This comment has been minimized.

Show comment
Hide comment
@fornwall

fornwall Oct 16, 2016

Contributor

@krader1961 Changed it to an autoconf check instead of ifdef __ANDROID__.

I've left the Android-specific comments for now as it's the only relevant platform I know about lacking getpwent() - let me know if those comments should be removed/changed!

Contributor

fornwall commented Oct 16, 2016

@krader1961 Changed it to an autoconf check instead of ifdef __ANDROID__.

I've left the Android-specific comments for now as it's the only relevant platform I know about lacking getpwent() - let me know if those comments should be removed/changed!

@floam floam merged commit fe8727f into fish-shell:master Oct 16, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@floam floam added the release notes label Oct 16, 2016

@zanchey zanchey added this to the fish 2.4.0 milestone Oct 16, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment