[1/1] package/ntp: remove host shell check
diff mbox series

Message ID 0102016e898693a5-82db32f2-a59d-42c1-996b-97661b367c25-000000@eu-west-1.amazonses.com
State Superseded
Headers show
Series
  • [1/1] package/ntp: remove host shell check
Related show

Commit Message

James Byrne Nov. 20, 2019, 3:55 p.m. UTC
This adds a patch which removes the autoconf check for which shell is
installed on the host and fixes the shell as '/bin/sh'. Since we are
cross-compiling, we cannot assume that the target uses the same shell.
Also, it can prevent builds being reproducible because a different host
environment will result in a different target binary.

Signed-off-by: James Byrne <james.byrne@origamienergy.com>
---
 package/ntp/0003-force-sh.patch | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)
 create mode 100644 package/ntp/0003-force-sh.patch

Comments

Yann E. MORIN Nov. 23, 2019, 10:30 a.m. UTC | #1
James, All,

On 2019-11-20 15:55 +0000, James Byrne spake thusly:
> This adds a patch which removes the autoconf check for which shell is
> installed on the host and fixes the shell as '/bin/sh'. Since we are
> cross-compiling, we cannot assume that the target uses the same shell.
> Also, it can prevent builds being reproducible because a different host
> environment will result in a different target binary.
> 
> Signed-off-by: James Byrne <james.byrne@origamienergy.com>
> ---
>  package/ntp/0003-force-sh.patch | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
>  create mode 100644 package/ntp/0003-force-sh.patch
> 
> diff --git a/package/ntp/0003-force-sh.patch b/package/ntp/0003-force-sh.patch
> new file mode 100644
> index 0000000000..4c2628e04a
> --- /dev/null
> +++ b/package/ntp/0003-force-sh.patch
> @@ -0,0 +1,30 @@
> +We're cross compiling, don't use the presence of shells on the host to decide
> +anything about the target...
> +
> +diff --git a/sntp/libopts/m4/libopts.m4 b/sntp/libopts/m4/libopts.m4
> +--- a/sntp/libopts/m4/libopts.m4
> ++++ b/sntp/libopts/m4/libopts.m4
> +@@ -112,22 +112,7 @@
> +   AC_CHECK_FUNCS([mmap canonicalize_file_name snprintf strdup strchr \
> +                  strrchr strsignal fchmod fstat chmod])
> +   AC_PROG_SED
> +-  [while :
> +-  do
> +-      POSIX_SHELL=`which bash`
> +-      test -x "$POSIX_SHELL" && break
> +-      POSIX_SHELL=`which dash`
> +-      test -x "$POSIX_SHELL" && break
> +-      POSIX_SHELL=/usr/xpg4/bin/sh
> +-      test -x "$POSIX_SHELL" && break
> +-      POSIX_SHELL=`/bin/sh -c '
> +-          exec 2>/dev/null
> +-          if ! true ; then exit 1 ; fi
> +-          echo /bin/sh'`
> +-      test -x "$POSIX_SHELL" && break
> +-      ]AC_MSG_ERROR([cannot locate a working POSIX shell])[
> +-  done]
> +-  AC_DEFINE_UNQUOTED([POSIX_SHELL], ["${POSIX_SHELL}"],
> ++  AC_DEFINE_UNQUOTED([POSIX_SHELL], ["/bin/sh"],

This is not nice, as this patch can't be upstreamed, and we'll have tio
live with it for the eons to come.

What about using AC_CACHE_CHECK [0] to do the check, which can be
overriden on the command line. For example:

    [
    AC_CACHE_CHECK([for a posix-compliant shell], [ntp_ac_cv_posix_shell],
    [while :
    do
        blablabla
    done
    ])
    AC_DEFINE_UNQUOTED([POSIX_SHELL], ["${ntp_ac_cv_posix_shell}"],
      [define to a working POSIX compliant shell])
    ]

And then in ntp.mk we could do (in addition to the existing one, of
course):

    NTP_CONF_ENV = ntp_ac_cv_posix_shell=/bin/sh

And such a patch will probably be more acceptable for upstream.

[0] https://www.gnu.org/savannah-checkouts/gnu/autoconf/manual/autoconf-2.69/autoconf.html#Caching-Results

Regards,
Yann E. MORIN.

> +            [define to a working POSIX compliant shell])
> +   AC_SUBST([POSIX_SHELL])
> + ])
> -- 
> 2.24.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
James Byrne Nov. 26, 2019, 11:24 a.m. UTC | #2
Hi Yann,

On 23/11/2019 10:30, Yann E. MORIN wrote:
> This is not nice, as this patch can't be upstreamed, and we'll have tio
> live with it for the eons to come.
> 
> What about using AC_CACHE_CHECK [0] to do the check, which can be
> overriden on the command line. For example:
> 
>      [
>      AC_CACHE_CHECK([for a posix-compliant shell], [ntp_ac_cv_posix_shell],
>      [while :
>      do
>          blablabla
>      done
>      ])
>      AC_DEFINE_UNQUOTED([POSIX_SHELL], ["${ntp_ac_cv_posix_shell}"],
>        [define to a working POSIX compliant shell])
>      ]
> 
> And then in ntp.mk we could do (in addition to the existing one, of
> course):
> 
>      NTP_CONF_ENV = ntp_ac_cv_posix_shell=/bin/sh
> 
> And such a patch will probably be more acceptable for upstream.

Your comments prompted me to do some more digging, and it turns out that 
the issue has been addressed in a later version of AutoGen, which is 
where the issue comes from - see the change to libopts.def in:

https://sourceforge.net/p/autogen/code/ci/e51f2a781500236b2ba92a4ccd854771a7c28b61/

I will resubmit a V2 patch based on pulling in this change, then if the 
upstream ntp package is updated to use a newer version of AutoGen at 
some point in the future, the patch can go away.

Regards,

James

Patch
diff mbox series

diff --git a/package/ntp/0003-force-sh.patch b/package/ntp/0003-force-sh.patch
new file mode 100644
index 0000000000..4c2628e04a
--- /dev/null
+++ b/package/ntp/0003-force-sh.patch
@@ -0,0 +1,30 @@ 
+We're cross compiling, don't use the presence of shells on the host to decide
+anything about the target...
+
+diff --git a/sntp/libopts/m4/libopts.m4 b/sntp/libopts/m4/libopts.m4
+--- a/sntp/libopts/m4/libopts.m4
++++ b/sntp/libopts/m4/libopts.m4
+@@ -112,22 +112,7 @@
+   AC_CHECK_FUNCS([mmap canonicalize_file_name snprintf strdup strchr \
+                  strrchr strsignal fchmod fstat chmod])
+   AC_PROG_SED
+-  [while :
+-  do
+-      POSIX_SHELL=`which bash`
+-      test -x "$POSIX_SHELL" && break
+-      POSIX_SHELL=`which dash`
+-      test -x "$POSIX_SHELL" && break
+-      POSIX_SHELL=/usr/xpg4/bin/sh
+-      test -x "$POSIX_SHELL" && break
+-      POSIX_SHELL=`/bin/sh -c '
+-          exec 2>/dev/null
+-          if ! true ; then exit 1 ; fi
+-          echo /bin/sh'`
+-      test -x "$POSIX_SHELL" && break
+-      ]AC_MSG_ERROR([cannot locate a working POSIX shell])[
+-  done]
+-  AC_DEFINE_UNQUOTED([POSIX_SHELL], ["${POSIX_SHELL}"],
++  AC_DEFINE_UNQUOTED([POSIX_SHELL], ["/bin/sh"],
+            [define to a working POSIX compliant shell])
+   AC_SUBST([POSIX_SHELL])
+ ])