Re: [Buildroot] [PATCH v1] package/wget: bump version to 1.21.3

[Peter Seiderer <ps.report@gmx.net>]

Hello Thomas,

On Tue, 15 Mar 2022 22:05:31 +0100, Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> Hello Peter,
>
> On Fri, 11 Mar 2022 07:50:42 +0100
> Peter Seiderer <ps.report@gmx.net> wrote:
>
> > - explicit set some default options (--without-metalink, --enable-opie,
> >   --enable-digest, --enable-ntlm, --disable-debug, --disable-valgrind-tests,
> >   --disable-assert)
> >
> > - add optional libpsl dependency
> >
> > - remove legacy --with-libidn option (see [1]), replace with
> >   new --enable-iri option in case libiconv and libidn2 are available
> >
> > - use explicit --with-libuuid option
> >
> > - add optional c-ares dependency
> >
> > For details see [2].
> >
> > [1] https://git.savannah.gnu.org/cgit/wget.git/commit/configure.ac?id=a24e67e239ef949cc77a4c4e5a0beb703026a296
> > [2] https://lists.gnu.org/archive/html/info-gnu/2022-02/msg00017.html
> >
> > Signed-off-by: Peter Seiderer <ps.report@gmx.net>
>
> You're mixing up a minor version bump with a significant rework of the
> package, which is unrelated to the version bump. This really needs
> separate commit. Also, see below.

Started as a minor/patch-level version bump....and did take a look at the
configure.ac changes ;-), will split up the patch on next iteration...
>
>
> > +WGET_CONF_OPTS = \
> > +	--without-metalink \
> > +	--enable-opie \
> > +	--enable-digest \
> > +	--enable-ntlm \
>
> So opie, digest and ntlm enabled is the default?

Yes (this is what I wanted to express in the commit log by 'explicit set some
default options')...

>
> > +	--disable-debug \
> > +	--disable-valgrind-tests \
> > +	--disable-assert

Same here...

> > +
> > +ifeq ($(BR2_PACKAGE_LIBPSL),y)
> > +WGET_CONF_OPTS += --with-libpsl
> > +WGET_DEPENDENCIES += libpsl
> > +else
> > +WGET_CONF_OPTS += --without-libpsl
> > +endif
> > +
> >  ifeq ($(BR2_PACKAGE_GNUTLS),y)
> >  WGET_CONF_OPTS += --with-ssl=gnutls
> >  WGET_DEPENDENCIES += gnutls
> > @@ -22,19 +38,18 @@ else
> >  WGET_CONF_OPTS += --without-ssl
> >  endif
> >
> > -ifeq ($(BR2_PACKAGE_LIBICONV),y)
> > -WGET_DEPENDENCIES += libiconv
> > -endif
> > -
> > -ifeq ($(BR2_PACKAGE_LIBIDN2),y)
> > -WGET_CONF_OPTS += --with-libidn
> > -WGET_DEPENDENCIES += libidn2
> > +ifeq ($(BR2_PACKAGE_LIBICONV)$(BR2_PACKAGE_LIBIDN2),yy)
>
> This is very likely wrong. Indeed BR2_PACKAGE_LIBICONV=y is only
> possible when the toolchain does *not* have locale support. When the
> toolchain has locale support, iconv support is provided by the
> toolchain itself.

Ups, did misread the following configure.ac part:

 780 AS_IF([test "X$iri" != "Xno"],[
 781   if test "X$am_cv_func_iconv" != "Xyes"; then
 782     iri=no
 783     if test "X$force_iri" = "Xyes"; then
 784       AC_MSG_ERROR([Libiconv is required for IRIs support])
 785     else
 786       AC_MSG_NOTICE([disabling IRIs because libiconv wasn't found])
 787     fi
 788   fi
 789 ])

>
> Could you revisit this, and also clarify the interaction between
> --enable-iri/--disable-iri on one side and
> --with-libidn/--without-libidn on the other side?

The old 'with-libidn/without-libidn' option is gone, libidn2 is checked via
pkg-config, the new option is 'enable-iri/disable-iri' (or auto-detect)...

Will update the patch (soon)...., thanks for review!

Regards,
Peter

>
> Thanks!
>
> Thomas

_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot