| Message ID | 20230103124505.6611-2-pvorel@suse.cz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [1/2] configure.ac: Require 2.64 | expand |
Hello, Petr Vorel <pvorel@suse.cz> writes: > AC_PROG_AR was added much later: in v2.72a [1]. Also it looks like that > redefinition is not a problem thus not wrapping with m4_ifndef([AC_PROG_AR]. > > NOTE: missing 'ar' don't fail configure (isn't the check useless then?): > $ rm -rf autom4te.cache/; ma autotools && ./configure; echo $? > ... > configure:4878: checking for ar > configure:4913: result: no > ... > 0 > > [1] https://git.savannah.gnu.org/cgit/autoconf.git/commit/?id=c48fdb81191c8b7c7c0dde6141b861b178a6a284 > > Signed-off-by: Petr Vorel <pvorel@suse.cz> > --- > configure.ac | 3 +-- > testcases/realtime/configure.ac | 3 +-- > 2 files changed, 2 insertions(+), 4 deletions(-) > > diff --git a/configure.ac b/configure.ac > index c2b0f48e79..edf00687f2 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -20,8 +20,7 @@ AM_MAINTAINER_MODE([enable]) > AC_CANONICAL_HOST > > AC_PROG_CC > -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about > -# 2.62. > +# autoconf >= v2.72a This reads like we need the def for autoconf => v2.72a. How about # for autoconfig < v2.72a Also which distro(s) do we support which have < v2.72a? We maybe can include that here *if known*. > AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)]) > AC_PROG_AR > AC_PROG_RANLIB > diff --git a/testcases/realtime/configure.ac b/testcases/realtime/configure.ac > index 6f50f1490f..39f16f1779 100644 > --- a/testcases/realtime/configure.ac > +++ b/testcases/realtime/configure.ac > @@ -12,8 +12,7 @@ AC_CHECK_HEADERS_ONCE([ \ > AC_CANONICAL_HOST > > AC_PROG_CC > -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about > -# 2.62. > +# autoconf >= v2.72a > AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)]) > AC_PROG_AR > AC_PROG_RANLIB > -- > 2.39.0
Hi Richie, thanks for your review. > > AC_PROG_CC > > -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about > > -# 2.62. > > +# autoconf >= v2.72a > This reads like we need the def for autoconf => v2.72a. How about You're right. I probably thought this is defined since v2.72a, thus not needed. > # for autoconfig < v2.72a This is much better, I'll merge it as this (unless anybody objects). > Also which distro(s) do we support which have < v2.72a? We maybe can > include that here *if known*. I'm not sure if v2.72a (alpha?) is shipped to any distro. v2.72 haven't been released yet. The reason to update the comment was 1) don't confuse with very old version (it's still needed) 2) notify users once v2.72 is released and shipped in case of problems. Kind regards, Petr
On 2023-01-10 4:25 AM, Richard Palethorpe wrote: >>> AC_PROG_CC >>> -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about >>> -# 2.62. >>> +# autoconf >= v2.72a > >> This reads like we need the def for autoconf => v2.72a. How about > You're right. I probably thought this is defined since v2.72a, > thus not needed. Please don't use 'v2.72a' in any commentary or tests. That version doesn't exist yet and may never exist; if it does, it will be a short-lived beta test release of v2.72 that we don't want people to depend on. (Autoconf uses a very old version numbering convention in which beta tests for release X.Y are labeled X.Ya, X.Yb, X.Yc, etc.) Officially, AC_PROG_AR will be available as of version 2.72, and that's what you should reference in commentary. > Also it looks like that redefinition is not a problem thus > not wrapping with m4_ifndef([AC_PROG_AR]. Autoconf will let you do that, but it's bad practice. What if version 2.73 makes AC_PROG_AR expand to something other than AC_CHECK_TOOL(AR, ar, :) ? You'd be overwriting whatever bug fix that was. I suggest something like # AC_PROG_AR was added in autoconf 2.72. m4_ifndef([AC_PROG_AR], [AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)])]) > NOTE: missing 'ar' don't fail configure (isn't the check useless > then?) We don't know what you need `ar` for; it might not be appropriate to fail the build if it's missing. You can do AC_PROG_AR AS_IF([test x$AR = x:], [AC_MSG_FAILURE([no usable "ar" program detected])]) if you want to fail the build. zw
Hi Zack, thank you for all your comments, highly appreciated! > On 2023-01-10 4:25 AM, Richard Palethorpe wrote: > > > > AC_PROG_CC > > > > -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about > > > > -# 2.62. > > > > +# autoconf >= v2.72a > > > This reads like we need the def for autoconf => v2.72a. How about > > You're right. I probably thought this is defined since v2.72a, > > thus not needed. > Please don't use 'v2.72a' in any commentary or tests. That version doesn't > exist yet and may never exist; if it does, it will be a short-lived beta > test release of v2.72 that we don't want people to depend on. > (Autoconf uses a very old version numbering convention in which beta tests > for release X.Y are labeled X.Ya, X.Yb, X.Yc, etc.) > Officially, AC_PROG_AR will be available as of version 2.72, and that's what > you should reference in commentary. I understood v2.72a similarly as kernel -rc1 gained new version. But sure, makes perfect sense to use final version in the comment. > > Also it looks like that redefinition is not a problem thus > > not wrapping with m4_ifndef([AC_PROG_AR]. > Autoconf will let you do that, but it's bad practice. What if version 2.73 > makes AC_PROG_AR expand to something other than > AC_CHECK_TOOL(AR, ar, :) ? You'd be overwriting whatever bug fix that was. > I suggest something like > # AC_PROG_AR was added in autoconf 2.72. > m4_ifndef([AC_PROG_AR], > [AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)])]) > > NOTE: missing 'ar' don't fail configure (isn't the check useless > > then?) > We don't know what you need `ar` for; it might not be appropriate to fail > the build if it's missing. You can do > AC_PROG_AR > AS_IF([test x$AR = x:], > [AC_MSG_FAILURE([no usable "ar" program detected])]) > if you want to fail the build. @Richie @Li, we obviously need ar for libs/, I'd be for this. I also have look how other check works. Kind regards, Petr > zw
Hello, Petr Vorel <pvorel@suse.cz> writes: > Hi Zack, > > thank you for all your comments, highly appreciated! > >> On 2023-01-10 4:25 AM, Richard Palethorpe wrote: >> > > > AC_PROG_CC >> > > > -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about >> > > > -# 2.62. >> > > > +# autoconf >= v2.72a > >> > > This reads like we need the def for autoconf => v2.72a. How about >> > You're right. I probably thought this is defined since v2.72a, >> > thus not needed. > >> Please don't use 'v2.72a' in any commentary or tests. That version doesn't >> exist yet and may never exist; if it does, it will be a short-lived beta >> test release of v2.72 that we don't want people to depend on. >> (Autoconf uses a very old version numbering convention in which beta tests >> for release X.Y are labeled X.Ya, X.Yb, X.Yc, etc.) > >> Officially, AC_PROG_AR will be available as of version 2.72, and that's what >> you should reference in commentary. > > I understood v2.72a similarly as kernel -rc1 gained new version. > But sure, makes perfect sense to use final version in the comment. > >> > Also it looks like that redefinition is not a problem thus >> > not wrapping with m4_ifndef([AC_PROG_AR]. > >> Autoconf will let you do that, but it's bad practice. What if version 2.73 >> makes AC_PROG_AR expand to something other than >> AC_CHECK_TOOL(AR, ar, :) ? You'd be overwriting whatever bug fix that was. > >> I suggest something like > >> # AC_PROG_AR was added in autoconf 2.72. >> m4_ifndef([AC_PROG_AR], >> [AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)])]) > >> > NOTE: missing 'ar' don't fail configure (isn't the check useless >> > then?) > >> We don't know what you need `ar` for; it might not be appropriate to fail >> the build if it's missing. You can do > >> AC_PROG_AR >> AS_IF([test x$AR = x:], >> [AC_MSG_FAILURE([no usable "ar" program detected])]) > >> if you want to fail the build. > > @Richie @Li, we obviously need ar for libs/, I'd be for this. > I also have look how other check works. Sure, also I'll set this to "changes requested" in Patchwork. > > Kind regards, > Petr > >> zw
diff --git a/configure.ac b/configure.ac index c2b0f48e79..edf00687f2 100644 --- a/configure.ac +++ b/configure.ac @@ -20,8 +20,7 @@ AM_MAINTAINER_MODE([enable]) AC_CANONICAL_HOST AC_PROG_CC -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about -# 2.62. +# autoconf >= v2.72a AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)]) AC_PROG_AR AC_PROG_RANLIB diff --git a/testcases/realtime/configure.ac b/testcases/realtime/configure.ac index 6f50f1490f..39f16f1779 100644 --- a/testcases/realtime/configure.ac +++ b/testcases/realtime/configure.ac @@ -12,8 +12,7 @@ AC_CHECK_HEADERS_ONCE([ \ AC_CANONICAL_HOST AC_PROG_CC -# <= autoconf 2.61 doesn't have AC_PROG_AR, but 2.63 has it. Not sure about -# 2.62. +# autoconf >= v2.72a AC_DEFUN([AC_PROG_AR], [AC_CHECK_TOOL(AR, ar, :)]) AC_PROG_AR AC_PROG_RANLIB
AC_PROG_AR was added much later: in v2.72a [1]. Also it looks like that redefinition is not a problem thus not wrapping with m4_ifndef([AC_PROG_AR]. NOTE: missing 'ar' don't fail configure (isn't the check useless then?): $ rm -rf autom4te.cache/; ma autotools && ./configure; echo $? ... configure:4878: checking for ar configure:4913: result: no ... 0 [1] https://git.savannah.gnu.org/cgit/autoconf.git/commit/?id=c48fdb81191c8b7c7c0dde6141b861b178a6a284 Signed-off-by: Petr Vorel <pvorel@suse.cz> --- configure.ac | 3 +-- testcases/realtime/configure.ac | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-)