Re: [PATCH 2/3] configure.ac,trace.c: check for CLOCK_MONOTONIC

[Eric Sunshine <sunshine@sunshineco.com>]

On Sun, Dec 21, 2014 at 1:53 PM, Reuben Hawkins <reubenhwk@gmail.com> wrote:
> CLOCK_MONOTONIC isn't available on RHEL3, but there are still RHEL3
> systems being used in production.  This change makes compiling git
> less tedious on older platforms.

Missing sign-off.

> ---
> diff --git a/configure.ac b/configure.ac
> index 3cfdd51..3900044 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -736,8 +736,10 @@ GIT_UNSTASH_FLAGS($ICONVDIR)
>
>  GIT_CONF_SUBST([OLD_ICONV])
>
> +
>  ## Checks for typedefs, structures, and compiler characteristics.
>  AC_MSG_NOTICE([CHECKS for typedefs, structures, and compiler characteristics])
> +

Sneaking in a couple whitespace changes unrelated the patch's stated
purpose. Such cleanup should be done typically as a separate
preparatory patch (though they probably aren't warranted in this
case).

>  #
>  TYPE_SOCKLEN_T
>  case $ac_cv_type_socklen_t in
> @@ -930,6 +932,30 @@ AC_CHECK_LIB([iconv], [locale_charset],
>                       [CHARSET_LIB=-lcharset])])
>  GIT_CONF_SUBST([CHARSET_LIB])
>  #
> +# Define NO_CLOCK_GETTIME if you don't have clock_gettime.

The comment talks about NO_CLOCK_GETTIME, however, the code names it
HAVE_CLOCK_GETTIME.

> +GIT_CHECK_FUNC(clock_gettime,
> +[HAVE_CLOCK_GETTIME=Yes],

For consistency with other build values: s/Yes/YesPlease/

> +[HAVE_CLOCK_GETTIME=])
> +GIT_CONF_SUBST([HAVE_CLOCK_GETTIME])
> +
> +AC_DEFUN([CLOCK_MONOTONIC_SRC], [
> +AC_LANG_PROGRAM([[
> +#include <time.h>
> +clockid_t id = CLOCK_MONOTONIC;
> +]], [])])
> +
> +#
> +# Define NO_CLOCK_MONOTONIC on really old systems that are still in production
> +# if you need GIT to compile but can't update the machine otherwise.

The comment talks about NO_CLOCK_MONOTONIC, however, the code names it
HAVE_CLOCK_MONOTONIC.

The "old systems .. need GIT to compile ... can't update" commentary
is unnecessary. That's pretty well implied by the mere use of
Autoconf. It would be sufficient to say "Define ... if you don't have
CLOCK_MONOTONIC."

> +AC_MSG_CHECKING([for CLOCK_MONOTONIC])
> +AC_COMPILE_IFELSE([CLOCK_MONOTONIC_SRC],
> +       [AC_MSG_RESULT([yes])
> +       HAVE_CLOCK_MONOTONIC=Yes],

Consistency: s/Yes/YesPlease/

> +       [AC_MSG_RESULT([no])
> +       HAVE_CLOCK_MONOTONIC=])
> +
> +GIT_CONF_SUBST([HAVE_CLOCK_MONOTONIC])

Be aware that this does not actually #define HAVE_CLOCK_MONOTONIC
anywhere. It only sets a Makefile variable in config.mak.autogen.
(Consequently, this patch it actually breaks support for this feature
in trace.c for people who build without running the configure script.)
You need to do extra work to get the #define.

In particular, add a comment to Makefile describing the
HAVE_CLOCK_MONOTONIC setting. (Placing it just below the existing
comment for HAVE_CLOCK_GETTIME would make loads of sense.)

Next, add code to the Makefile to actually #define
HAVE_CLOCK_MONOTONIC. Something like this (alongside the existing
HAVE_CLOCK_GETTIME code):

    ifdef HAVE_CLOCK_MONOTONIC
        BASIC_CFLAGS += -DHAVE_CLOCK_MONOTONIC
    endif

Finally, update config.make.uname to set
HAVE_CLOCK_MONOTONIC=YesPlease on platforms which are likely to have
it (for people who don't run the configure script). At the very least,
it should be enabled by default for Linux.

> +#
>  # Define NO_SETITIMER if you don't have setitimer.
>  GIT_CHECK_FUNC(setitimer,
>  [NO_SETITIMER=],
> diff --git a/trace.c b/trace.c
> index 4778608..bfbd48f 100644
> --- a/trace.c
> +++ b/trace.c
> @@ -324,7 +324,7 @@ int trace_want(struct trace_key *key)
>         return !!get_trace_fd(key);
>  }
>
> -#ifdef HAVE_CLOCK_GETTIME
> +#if defined(HAVE_CLOCK_GETTIME) && defined(HAVE_CLOCK_MONOTONIC)
>
>  static inline uint64_t highres_nanos(void)
>  {
> --
> 2.2.0.GIT