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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 21, 2014, 20:54 UTC
Message-ID: <CAPig+cRu9SoFVmgmU2BF=oi4AUZWCs8RFsjEuKtEvm-wzRkk1g@mail.gmail.com>
In-Reply-To: <1419188016-26134-2-git-send-email-reubenhwk@gmail.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.

Show 13 quoted lines

> ---
> 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).

Show 8 quoted lines

>  #
>  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/

Show 12 quoted lines

> +[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.

Show 19 quoted lines

> +#
>  # 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