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

Android build fix & CMake fix #2609

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@bwalle
Contributor

bwalle commented May 27, 2018

See the individual patches.

@bagder

So what header does Android want included to get the getpwuid_r declaration? This added test seems weird to me as we already check for those headers.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov May 28, 2018

Member

@bwalle It would be great to see "how to reproduce" description for the issue.
Thank you in advance.

Member

snikulov commented May 28, 2018

@bwalle It would be great to see "how to reproduce" description for the issue.
Thank you in advance.

@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 28, 2018

Contributor

To reproduce:

CC=/home/builduser/toolchains/android/tc/ndk14b-api19-x86/bin/i686-linux-android-gcc  ./configure --host=arm-linux

The declaration is just missing in the header, the problem is that the configure check just checks if the function is available at link-time, not if it's declared.

Contents of pwd.h:

#if 0 /* MISSING FROM BIONIC */
int getpwnam_r(const char*, struct passwd*, char*, size_t, struct passwd**);
int getpwuid_r(uid_t, struct passwd*, char*, size_t, struct passwd**);
struct passwd* getpwent(void);
int setpwent(void);
#endif /* MISSING */

I know that the toolchain is a bit older, but we're still using that one.

Contributor

bwalle commented May 28, 2018

To reproduce:

CC=/home/builduser/toolchains/android/tc/ndk14b-api19-x86/bin/i686-linux-android-gcc  ./configure --host=arm-linux

The declaration is just missing in the header, the problem is that the configure check just checks if the function is available at link-time, not if it's declared.

Contents of pwd.h:

#if 0 /* MISSING FROM BIONIC */
int getpwnam_r(const char*, struct passwd*, char*, size_t, struct passwd**);
int getpwuid_r(uid_t, struct passwd*, char*, size_t, struct passwd**);
struct passwd* getpwent(void);
int setpwent(void);
#endif /* MISSING */

I know that the toolchain is a bit older, but we're still using that one.

@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 28, 2018

Contributor

But the cmake fix should be ok, isn't it?

Contributor

bwalle commented May 28, 2018

But the cmake fix should be ok, isn't it?

@MarcelRaad

This comment has been minimized.

Show comment
Hide comment
@MarcelRaad

MarcelRaad May 28, 2018

Member

See also #2058. This is a bug in the NDK fixed in later versions.

Member

MarcelRaad commented May 28, 2018

See also #2058. This is a bug in the NDK fixed in later versions.

@snikulov

This comment has been minimized.

Show comment
Hide comment
@snikulov

snikulov May 28, 2018

Member

@bwalle I see no CMake invocation in your "how to reproduce". Why did you add it to the subject?

Member

snikulov commented May 28, 2018

@bwalle I see no CMake invocation in your "how to reproduce". Why did you add it to the subject?

@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 28, 2018

Contributor

@snikulov Well, the CMake thing has nothing todo with Android. The check for getpwuid_r is just missing. There's no build error, but the function is not used then. I'm just using cmake .. on Linux.

I discovered the issue when checking whether the declaration of getpwuid_r is checked with CMake. But CMake checks both the declaration and definition, at least according to the documentation.

Contributor

bwalle commented May 28, 2018

@snikulov Well, the CMake thing has nothing todo with Android. The check for getpwuid_r is just missing. There's no build error, but the function is not used then. I'm just using cmake .. on Linux.

I discovered the issue when checking whether the declaration of getpwuid_r is checked with CMake. But CMake checks both the declaration and definition, at least according to the documentation.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 28, 2018

Member

@snikulov it seems to be an omission in the cmake build

@bwalle I think @MarcelRaad is spot on; this is an NDK issue that is already fixed. And if we were to add a work-around for this in our code, I don't see how this is a good such one!

Member

bagder commented May 28, 2018

@snikulov it seems to be an omission in the cmake build

@bwalle I think @MarcelRaad is spot on; this is an NDK issue that is already fixed. And if we were to add a work-around for this in our code, I don't see how this is a good such one!

@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 28, 2018

Contributor

@bagder I can remove the Android patch and keep it in my local build system until we update the toolchain. Would you merge the cmake part?

Contributor

bwalle commented May 28, 2018

@bagder I can remove the Android patch and keep it in my local build system until we update the toolchain. Would you merge the cmake part?

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 28, 2018

Member

I can remove the Android patch and keep it in my local build system until we update the toolchain

You can of course do what you want, but I still consider it a lame "fix". It just disables the use of the function so instead you'll use a function that isn't thread-safe. A more proper fix would be to instead provide a private prototype to avoid the warning when the correct function is used!

The cmake fix is valid and I'll merge!

Member

bagder commented May 28, 2018

I can remove the Android patch and keep it in my local build system until we update the toolchain

You can of course do what you want, but I still consider it a lame "fix". It just disables the use of the function so instead you'll use a function that isn't thread-safe. A more proper fix would be to instead provide a private prototype to avoid the warning when the correct function is used!

The cmake fix is valid and I'll merge!

bagder added a commit that referenced this pull request May 28, 2018

cmake: check for getpwuid_r
The autotools-based build system does it, so we do it also in CMake.

Bug: #2609
Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 29, 2018

Member

The cmake commit is merged, the configure commit is not and needs another approach or an NDK update.

If you rather stick with that patch locally, we might just as well close this PR. What do you think @bwalle ?

Member

bagder commented May 29, 2018

The cmake commit is merged, the configure commit is not and needs another approach or an NDK update.

If you rather stick with that patch locally, we might just as well close this PR. What do you think @bwalle ?

@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 29, 2018

Contributor

I can create a new patch for the NDK issue if you advise me how you want to get it fixed, but I'm also fine with closing that PR.

Contributor

bwalle commented May 29, 2018

I can create a new patch for the NDK issue if you advise me how you want to get it fixed, but I'm also fine with closing that PR.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 29, 2018

Member

I believe a proper fix for this broken NDK would be as following (assuming I've understood this correctly):

1 - detect the lack of getpwuid_r declaration, similar to how you did it already. If there's no proto for it, set a custom define (HAVE_NO_GETPWUID_R_PROTO perhaps?)
2 - in lib/netrc.c and lib/curl_ntlm.c, add code in the top of the files similar to:

#ifdef HAVE_NO_GETPWUID_R_PROTO
int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
               size_t buflen, struct passwd **result);
#endif
Member

bagder commented May 29, 2018

I believe a proper fix for this broken NDK would be as following (assuming I've understood this correctly):

1 - detect the lack of getpwuid_r declaration, similar to how you did it already. If there's no proto for it, set a custom define (HAVE_NO_GETPWUID_R_PROTO perhaps?)
2 - in lib/netrc.c and lib/curl_ntlm.c, add code in the top of the files similar to:

#ifdef HAVE_NO_GETPWUID_R_PROTO
int getpwuid_r(uid_t uid, struct passwd *pwd, char *buf,
               size_t buflen, struct passwd **result);
#endif
@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 29, 2018

Contributor

I've updated the branch.

Contributor

bwalle commented May 29, 2018

I've updated the branch.

@bagder

bagder approved these changes May 29, 2018

Looks good. I'll just let the CI tests complete first before merging to make sure there's nothing subtle there we missed...

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 30, 2018

Member

And yes, it figured out this:

lib/curl_setup.h:804:34: error: "HAVE_DECL_GETPWUID_R" is not defined [-Werror=undef]
 #if defined(HAVE_GETPWUID_R) && !HAVE_DECL_GETPWUID_R
                                  ^

It highlights this problem: only the configure script will define HAVE_DECL_GETPWUID_R for the time being, so all other builds (primarily the cmake one) will use this custom prototype. I'm not 100% sure that is a real problem but I suspect it might become one.

I would prefer if the configure script defines name for the reverse: when the prototype is found to be missing as then we could be sure that only systems for which configure has detected a missing prototype would use this custom one.

Member

bagder commented May 30, 2018

And yes, it figured out this:

lib/curl_setup.h:804:34: error: "HAVE_DECL_GETPWUID_R" is not defined [-Werror=undef]
 #if defined(HAVE_GETPWUID_R) && !HAVE_DECL_GETPWUID_R
                                  ^

It highlights this problem: only the configure script will define HAVE_DECL_GETPWUID_R for the time being, so all other builds (primarily the cmake one) will use this custom prototype. I'm not 100% sure that is a real problem but I suspect it might become one.

I would prefer if the configure script defines name for the reverse: when the prototype is found to be missing as then we could be sure that only systems for which configure has detected a missing prototype would use this custom one.

Check for declaration of getpwuid_r
On our x86 Android toolchain, getpwuid_r is implemented but the header
is missing:

 netrc.c:81:7: error: implicit declaration of function 'getpwuid_r' [-Werror=implicit-function-declaration]
        if(!getpwuid_r(geteuid(), &pw, pwbuf, sizeof(pwbuf), &pw_res)
        ^

Unfortunately, the function is used in curl_ntlm_wb.c, too, so I moved
the prototype to curl_setup.h.

Signed-off-by: Bernhard Walle <bernhard@bwalle.de>
@bwalle

This comment has been minimized.

Show comment
Hide comment
@bwalle

bwalle May 30, 2018

Contributor

I've updated my patch.

Contributor

bwalle commented May 30, 2018

I've updated my patch.

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder May 31, 2018

Member

Thanks!

Member

bagder commented May 31, 2018

Thanks!

@bagder bagder closed this in 9c33813 May 31, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 29, 2018

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