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

bpo-11063: Add a configure check for uuid_generate_time_safe #4287

Merged
merged 5 commits into from Nov 8, 2017

Conversation

Projects
None yet
7 participants
@berkerpeksag
Copy link
Member

berkerpeksag commented Nov 5, 2017

@skrah

skrah approved these changes Nov 5, 2017

@@ -571,6 +571,9 @@
/* Define to 1 if you have the <libutil.h> header file. */
#undef HAVE_LIBUTIL_H

/* Define to 1 if you have the `uuid' library (-luuid). */

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

It's not the uuid library, it's specifically uuid_generate_time_safe() (which exists on Linux, but not on OS X, even though OS X does have libuuid).

This comment has been minimized.

Copy link
@skrah

skrah Nov 5, 2017

Member

That looks autogenerated. The configure check works here on Linux.

With the patch:

checking for uuid_generate_time_safe in -luuid... yes

Now use a non-existing function name:

AC_CHECK_LIB(uuid, uuid_generate_time_unsafe)

checking for uuid_generate_time_unsafe in -luuid... no

I did not look at the autogenerated configure, but it seems to work here.

configure Outdated
cat confdefs.h - <<_ACEOF >conftest.$ac_ext
/* end confdefs.h. */
/* Override any GCC internal prototype to avoid an error.

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

Is this comment copy-pasted from somewhere else? It doesn't seem to apply here...

configure Outdated
#ifdef __cplusplus
extern "C"
#endif
char uuid_generate_time_safe ();

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

The actual signature here is int uuid_generate_time_safe(uuid_t out).

configure Outdated
int
main ()
{
return uuid_generate_time_safe ();

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

This call should match the function's signature.

configure Outdated
$as_echo_n "(cached) " >&6
else
ac_check_lib_save_LIBS=$LIBS
LIBS="-luuid $LIBS"

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

setup.py has a bit of code to find libuuid in the proper place (self.compiler.find_library_file(lib_dirs, 'uuid')). It sounds like you should replicate that in the configure script, but it looks like you don't?

configure Outdated
#define HAVE_LIBUUID 1
_ACEOF

LIBS="-luuid $LIBS"

This comment has been minimized.

Copy link
@pitrou

pitrou Nov 5, 2017

Member

I'm not sure about this. -luuid is only needed for the _uuid extension module, and it's already added by setup.py.

static PyObject *
py_uuid_generate_time_safe(void)
{
#ifdef HAVE_TIME_SAFE
#if defined(HAVE_UUID_GENERATE_TIME_SAFE) && HAVE_UUID_GENERATE_TIME_SAFE

This comment has been minimized.

Copy link
@vstinner

vstinner Nov 6, 2017

Member

I don't think that it's correct to test the define value. Only check if it's defined:

#ifdef HAVE_UUID_GENERATE_TIME_SAFE

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Nov 6, 2017

Author Member

Yeah, I was about to write about that. I couldn't make autoconf set #define HAVE_UUID_GENERATE_TIME_SAFE 0 if uuid_generate_time_safe() isn't available yet. Working on it :)

This comment has been minimized.

Copy link
@skrah

skrah Nov 6, 2017

Member

What was broken about the original patch except that some autogenerated comments were suboptimal?

The existing AC_CHECK_LIB(dld, shl_load) should have all the same problems.

@berkerpeksag

This comment has been minimized.

Copy link
Member Author

berkerpeksag commented Nov 7, 2017

Ok, I think I've addressed most of Antoine's (and Victor's) comments:

  • Renamed HAVE_LIBUUID to HAVE_UUID_GENERATE_TIME_SAFE
  • Don't set -luuid in LIBS. setup.py already covers this.
  • Switched back to use #ifdef HAVE_UUID_GENERATE_TIME_SAFE in _uuidmodule.c
  • The uuid_generate_time_safe signature in the test program now matches the actual implementation
  • Removed the /* Override any GCC internal prototype to avoid an error. */ comment

@berkerpeksag berkerpeksag force-pushed the berkerpeksag:11063-configure branch from 57a020a to 0f5703a Nov 7, 2017

uuid_t out;
uuid_generate_time_safe(out);
])],
[ax_cv_uuid_generate_time_safe=yes],

This comment has been minimized.

Copy link
@vstinner

vstinner Nov 7, 2017

Member

Is it possible to put directly the AC_DEFINE() here, instead of using a variable?

[ax_cv_uuid_generate_time_safe=no] would be replaced with [] in that case.

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Nov 7, 2017

Author Member

I tried that but I got the following warning:

configure.ac:2688: warning: AC_CACHE_VAL(ax_cv_uuid_generate_time_safe, ...): suspicious presence of an AC_DEFINE in the second argument, where no actions should be taken

This comment has been minimized.

Copy link
@vstinner

vstinner Nov 7, 2017

Member

Oh, I hate autotools, mysterious tools... Ok, leave it as it is ;-)

@vstinner
Copy link
Member

vstinner left a comment

LGTM.

Thank you Berker, you are more brave than me :-)

@berkerpeksag berkerpeksag merged commit 9a10ff4 into python:master Nov 8, 2017

3 checks passed

bedevere/issue-number Issue number 11063 found
Details
bedevere/news "skip news" label found
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@berkerpeksag berkerpeksag deleted the berkerpeksag:11063-configure branch Nov 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.