[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Adrien Gallouët]

A new option is added to build all functions.

This option was implicitly used before, so it is activated by default to
avoid unpleasant surprises.

Signed-off-by: Adrien Gallou?t <adrien@gallouet.fr>

---
Changes v3 -> v4:
  - better commit log (suggested by Thomas Petazzoni)

Changes v2 -> v3:
  - add a better help (suggested by Arnout Vandecappelle)

Changes v1 -> v2:
  - full build (suggested by Arnout Vandecappelle)
---
 package/libsodium/Config.in    | 12 ++++++++++++
 package/libsodium/libsodium.mk |  4 ++++
 2 files changed, 16 insertions(+)

diff --git a/package/libsodium/Config.in b/package/libsodium/Config.in
index 47b9bb350c..518b0532ed 100644
--- a/package/libsodium/Config.in
+++ b/package/libsodium/Config.in
@@ -4,3 +4,15 @@ config BR2_PACKAGE_LIBSODIUM
 	  A modern and easy-to-use crypto library.
 
 	  http://libsodium.org/
+
+if BR2_PACKAGE_LIBSODIUM
+
+config BR2_PACKAGE_LIBSODIUM_FULL
+	bool "all functions"
+	default y
+	help
+	  Build all seldom used and obsolete functions. Without
+	  this option, only the minimal set of normally used
+	  functions is built.
+
+endif
diff --git a/package/libsodium/libsodium.mk b/package/libsodium/libsodium.mk
index a94a8271c5..9afa09ff37 100644
--- a/package/libsodium/libsodium.mk
+++ b/package/libsodium/libsodium.mk
@@ -14,5 +14,9 @@ ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
 LIBSODIUM_CONF_OPTS += --disable-pie
 endif
 
+ifeq ($(BR2_PACKAGE_LIBSODIUM_FULL),)
+LIBSODIUM_CONF_OPTS += --enable-minimal
+endif
+
 $(eval $(autotools-package))
 $(eval $(host-autotools-package))
-- 
2.19.1

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Thomas Petazzoni]

Hello,

On Mon, 23 Sep 2019 08:29:03 +0000
Adrien Gallou?t <adrien@gallouet.fr> wrote:

> +config BR2_PACKAGE_LIBSODIUM_FULL
> +	bool "all functions"
> +	default y
> +	help
> +	  Build all seldom used and obsolete functions. Without
> +	  this option, only the minimal set of normally used
> +	  functions is built.
> +
> +endif
> diff --git a/package/libsodium/libsodium.mk b/package/libsodium/libsodium.mk
> index a94a8271c5..9afa09ff37 100644
> --- a/package/libsodium/libsodium.mk
> +++ b/package/libsodium/libsodium.mk
> @@ -14,5 +14,9 @@ ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
>  LIBSODIUM_CONF_OPTS += --disable-pie
>  endif
>  
> +ifeq ($(BR2_PACKAGE_LIBSODIUM_FULL),)
> +LIBSODIUM_CONF_OPTS += --enable-minimal

But then, does it make sense to invert the option and name it
BR2_PACKAGE_LIBSODIUM_MINIMAL ? It would match the
--enable-minimal/--disable-minimal autoconf options of libsodium.

Also, we need to pass --disable-minimal in an "else" clause.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Adrien Gallouët]

Hello,

On Sat, Sep 28, 2019 at 11:19 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Mon, 23 Sep 2019 08:29:03 +0000
> Adrien Gallou?t <adrien@gallouet.fr> wrote:
>
> > +config BR2_PACKAGE_LIBSODIUM_FULL
> > +     bool "all functions"
> > +     default y
> > +     help
> > +       Build all seldom used and obsolete functions. Without
> > +       this option, only the minimal set of normally used
> > +       functions is built.
> > +
> > +endif
> > diff --git a/package/libsodium/libsodium.mk b/package/libsodium/libsodium.mk
> > index a94a8271c5..9afa09ff37 100644
> > --- a/package/libsodium/libsodium.mk
> > +++ b/package/libsodium/libsodium.mk
> > @@ -14,5 +14,9 @@ ifeq ($(BR2_TOOLCHAIN_SUPPORTS_PIE),)
> >  LIBSODIUM_CONF_OPTS += --disable-pie
> >  endif
> >
> > +ifeq ($(BR2_PACKAGE_LIBSODIUM_FULL),)
> > +LIBSODIUM_CONF_OPTS += --enable-minimal
>
> But then, does it make sense to invert the option and name it
> BR2_PACKAGE_LIBSODIUM_MINIMAL ? It would match the
> --enable-minimal/--disable-minimal autoconf options of libsodium.

That was my first patch :) Do you want me to resubmit it ?

>
> Also, we need to pass --disable-minimal in an "else" clause.
>
> Thanks,
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com

Regards,
Adrien

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Thomas Petazzoni]

On Sat, 28 Sep 2019 23:49:56 +0200
Adrien Gallou?t <adrien@gallouet.fr> wrote:

> > But then, does it make sense to invert the option and name it
> > BR2_PACKAGE_LIBSODIUM_MINIMAL ? It would match the
> > --enable-minimal/--disable-minimal autoconf options of libsodium.  
> 
> That was my first patch :) Do you want me to resubmit it ?

I don't have a very clear-cut opinion.

On one hand, it is weird to have a Config.in option to reduce/remove
something. But on the other hand, it makes sense to have Config.in
options that are as much as possible mapped to the corresponding
options provided by the package build system.

Arnout, Peter, any preference ?

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Arnout Vandecappelle]

On 29/09/2019 09:04, Thomas Petazzoni wrote:
> On Sat, 28 Sep 2019 23:49:56 +0200
> Adrien Gallou?t <adrien@gallouet.fr> wrote:
> 
>>> But then, does it make sense to invert the option and name it
>>> BR2_PACKAGE_LIBSODIUM_MINIMAL ? It would match the
>>> --enable-minimal/--disable-minimal autoconf options of libsodium.  
>>
>> That was my first patch :) Do you want me to resubmit it ?
> 
> I don't have a very clear-cut opinion.
> 
> On one hand, it is weird to have a Config.in option to reduce/remove
> something. But on the other hand, it makes sense to have Config.in
> options that are as much as possible mapped to the corresponding
> options provided by the package build system.
> 
> Arnout, Peter, any preference ?

 It was me who suggested to turn around the logic.

 I believe we have had problems in the past where there was some MINIMAL option,
but then some other package needed something from the non-minimal set. Then it
has to do a 'depends on' instead of a 'select'.

 So it's not a matter of being weird, it's a matter of making sure we don't
create problems in the future.


>> Also, we need to pass --disable-minimal in an "else" clause.

 This is still true. Adrien, I believe I did say something to that effect in an
earlier iteration, and I also believe you replied to it. But my memory is
worthless :-)

 Regards,
 Arnout

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> Arnout, Peter, any preference ?

 >  It was me who suggested to turn around the logic.

 >  I believe we have had problems in the past where there was some MINIMAL option,
 > but then some other package needed something from the non-minimal set. Then it
 > has to do a 'depends on' instead of a 'select'.

 >  So it's not a matter of being weird, it's a matter of making sure we don't
 > create problems in the future.

Yes, indeed - I would prefer to keep it like this (a "positive" option).

 >>> Also, we need to pass --disable-minimal in an "else" clause.

 >  This is still true. Adrien, I believe I did say something to that effect in an
 > earlier iteration, and I also believe you replied to it. But my memory is
 > worthless :-)

Heh ;)

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Thomas Petazzoni]

On Sun, 29 Sep 2019 12:51:35 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:

>  I believe we have had problems in the past where there was some MINIMAL option,
> but then some other package needed something from the non-minimal set. Then it
> has to do a 'depends on' instead of a 'select'.

That makes a lot of sense indeed. So we should always have option that
"add" more stuff, and never option that "remove" stuff for that reason.

Thanks for the feedback, I hadn't thought about this.

Thomas
-- 
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH v4 1/1] package/libsodium: add config for full build

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >> I don't have a very clear-cut opinion.
 >> 
 >> On one hand, it is weird to have a Config.in option to reduce/remove
 >> something. But on the other hand, it makes sense to have Config.in
 >> options that are as much as possible mapped to the corresponding
 >> options provided by the package build system.
 >> 
 >> Arnout, Peter, any preference ?

 >  It was me who suggested to turn around the logic.

 >  I believe we have had problems in the past where there was some MINIMAL option,
 > but then some other package needed something from the non-minimal set. Then it
 > has to do a 'depends on' instead of a 'select'.

 >  So it's not a matter of being weird, it's a matter of making sure we don't
 > create problems in the future.

Agreed, having options to disable stuff gets tricky when you have other
packages needing the disabled stuff.

 >>> Also, we need to pass --disable-minimal in an "else" clause.

 >  This is still true. Adrien, I believe I did say something to that effect in an
 > earlier iteration, and I also believe you replied to it. But my memory is
 > worthless :-)

Getting old? :P

-- 
Bye, Peter Korsgaard