[Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Dario Binacchi]

The patch expands the number of usable .m4 files by including those
contained in the autoconf-archive.

Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
---
 package/automake/automake.mk | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/package/automake/automake.mk b/package/automake/automake.mk
index fd7933dd74f9..fc235784641a 100644
--- a/package/automake/automake.mk
+++ b/package/automake/automake.mk
@@ -34,5 +34,6 @@ $(eval $(host-autotools-package))
 AUTOMAKE = $(HOST_DIR)/bin/automake
 ACLOCAL_DIR = $(STAGING_DIR)/usr/share/aclocal
 ACLOCAL = $(HOST_DIR)/bin/aclocal
-ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR)
+ACLOCAL_HOST_ARCHIVE_DIR = $(HOST_DIR)/share/autoconf-archive
+ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR):$(ACLOCAL_HOST_ARCHIVE_DIR)
 export ACLOCAL_PATH
-- 
2.32.0

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

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Thomas Petazzoni via buildroot]

Hello Dario,

On Sun, 28 May 2023 13:30:42 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> The patch expands the number of usable .m4 files by including those
> contained in the autoconf-archive.
> 
> Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> ---
>  package/automake/automake.mk | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/package/automake/automake.mk b/package/automake/automake.mk
> index fd7933dd74f9..fc235784641a 100644
> --- a/package/automake/automake.mk
> +++ b/package/automake/automake.mk
> @@ -34,5 +34,6 @@ $(eval $(host-autotools-package))
>  AUTOMAKE = $(HOST_DIR)/bin/automake
>  ACLOCAL_DIR = $(STAGING_DIR)/usr/share/aclocal
>  ACLOCAL = $(HOST_DIR)/bin/aclocal
> -ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR)
> +ACLOCAL_HOST_ARCHIVE_DIR = $(HOST_DIR)/share/autoconf-archive
> +ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR):$(ACLOCAL_HOST_ARCHIVE_DIR)
>  export ACLOCAL_PATH

Could you clarify the motivation? Do you have a specific case for which
this is needed?

Thanks!

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Dario Binacchi]

Hello Thomas,

On Fri, Jul 28, 2023 at 11:02 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello Dario,
>
> On Sun, 28 May 2023 13:30:42 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
>
> > The patch expands the number of usable .m4 files by including those
> > contained in the autoconf-archive.
> >
> > Signed-off-by: Dario Binacchi <dario.binacchi@amarulasolutions.com>
> > ---
> >  package/automake/automake.mk | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/package/automake/automake.mk b/package/automake/automake.mk
> > index fd7933dd74f9..fc235784641a 100644
> > --- a/package/automake/automake.mk
> > +++ b/package/automake/automake.mk
> > @@ -34,5 +34,6 @@ $(eval $(host-autotools-package))
> >  AUTOMAKE = $(HOST_DIR)/bin/automake
> >  ACLOCAL_DIR = $(STAGING_DIR)/usr/share/aclocal
> >  ACLOCAL = $(HOST_DIR)/bin/aclocal
> > -ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR)
> > +ACLOCAL_HOST_ARCHIVE_DIR = $(HOST_DIR)/share/autoconf-archive
> > +ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR):$(ACLOCAL_HOST_ARCHIVE_DIR)
> >  export ACLOCAL_PATH
>
> Could you clarify the motivation? Do you have a specific case for which
> this is needed?
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com

Following the suggestions of Yann E. MORIN in [1], I tried to fix the
issue by submitting a
patch to the ethtool project. During the various review stages, I sent
a version (much more
complicated than the one that was eventually accepted) that required a
specific M4
macro ([2]). From there, the idea for this patch was born.

[1] https://patchwork.ozlabs.org/project/buildroot/patch/20230520211246.3950131-1-dario.binacchi@amarulasolutions.com/
[2] https://marc.info/?l=linux-netdev&m=168486237131560&w=2

Thanks and regards,
Dario

-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

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

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Thomas Petazzoni via buildroot]

On Sat, 29 Jul 2023 16:47:14 +0200
Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:

> > > diff --git a/package/automake/automake.mk b/package/automake/automake.mk
> > > index fd7933dd74f9..fc235784641a 100644
> > > --- a/package/automake/automake.mk
> > > +++ b/package/automake/automake.mk
> > > @@ -34,5 +34,6 @@ $(eval $(host-autotools-package))
> > >  AUTOMAKE = $(HOST_DIR)/bin/automake
> > >  ACLOCAL_DIR = $(STAGING_DIR)/usr/share/aclocal
> > >  ACLOCAL = $(HOST_DIR)/bin/aclocal
> > > -ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR)
> > > +ACLOCAL_HOST_ARCHIVE_DIR = $(HOST_DIR)/share/autoconf-archive
> > > +ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR):$(ACLOCAL_HOST_ARCHIVE_DIR)
> > >  export ACLOCAL_PATH  
> >
> > Could you clarify the motivation? Do you have a specific case for which
> > this is needed?
> >
> Following the suggestions of Yann E. MORIN in [1], I tried to fix the
> issue by submitting a
> patch to the ethtool project. During the various review stages, I sent
> a version (much more
> complicated than the one that was eventually accepted) that required a
> specific M4
> macro ([2]). From there, the idea for this patch was born.

What's confusing to me is: how is autoconf-archive useful/working today
in Buildroot if aclocal isn't told that
$(HOST_DIR)/share/autoconf-archive contains a bunch of m4 macros?

Ah, I see:

LIBGPIOD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
NEARD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
PYTHON3_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
etc.

So it would indeed probably make sense to factorize this. But the
problem with your change is that $(HOST_DIR)/share/autoconf-archive
gets added to ACLOCAL_PATH even if autoconf-archive isn't
used/installed. We could decide it is OK, but it should be mentioned.
Another side effect is that once autoconf-archive has been installed,
*all* packages will see the m4 macros from autoconf-archive, not only
the few packages that explicitly need autoconf-archive. I don't know if
this is a problem, but it can potentially have some side effects.

Also your patch should drop all the <pkg>_AUTORECONF_OPTS =
--include=$(HOST_DIR)/share/autoconf-archive, because they become
redundant.

Full list is:

package/libgpiod/libgpiod.mk:LIBGPIOD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
package/neard/neard.mk:NEARD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
package/python3/python3.mk:PYTHON3_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
package/sdbusplus/sdbusplus.mk:SDBUSPLUS_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
package/sox/sox.mk:SOX_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
package/thermald/thermald.mk:THERMALD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive

Yann, thoughts?

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Yann E. MORIN]

Thomas, All,

On 2023-07-29 23:03 +0200, Thomas Petazzoni via buildroot spake thusly:
> On Sat, 29 Jul 2023 16:47:14 +0200
> Dario Binacchi <dario.binacchi@amarulasolutions.com> wrote:
> > > > diff --git a/package/automake/automake.mk b/package/automake/automake.mk
> > > > index fd7933dd74f9..fc235784641a 100644
> > > > --- a/package/automake/automake.mk
> > > > +++ b/package/automake/automake.mk
> > > > @@ -34,5 +34,6 @@ $(eval $(host-autotools-package))
> > > >  AUTOMAKE = $(HOST_DIR)/bin/automake
> > > >  ACLOCAL_DIR = $(STAGING_DIR)/usr/share/aclocal
> > > >  ACLOCAL = $(HOST_DIR)/bin/aclocal
> > > > -ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR)
> > > > +ACLOCAL_HOST_ARCHIVE_DIR = $(HOST_DIR)/share/autoconf-archive
> > > > +ACLOCAL_PATH = $(ACLOCAL_DIR):$(ACLOCAL_HOST_DIR):$(ACLOCAL_HOST_ARCHIVE_DIR)
> > > >  export ACLOCAL_PATH  
> > > Could you clarify the motivation? Do you have a specific case for which
> > > this is needed?
> > Following the suggestions of Yann E. MORIN in [1], I tried to fix the
> > issue by submitting a
> > patch to the ethtool project. During the various review stages, I sent
> > a version (much more
> > complicated than the one that was eventually accepted) that required a
> > specific M4
> > macro ([2]). From there, the idea for this patch was born.
> What's confusing to me is: how is autoconf-archive useful/working today
> in Buildroot if aclocal isn't told that
> $(HOST_DIR)/share/autoconf-archive contains a bunch of m4 macros?
> Ah, I see:
> LIBGPIOD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
> NEARD_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
> PYTHON3_AUTORECONF_OPTS = --include=$(HOST_DIR)/share/autoconf-archive
> etc.
> 
> So it would indeed probably make sense to factorize this. But the
> problem with your change is that $(HOST_DIR)/share/autoconf-archive
> gets added to ACLOCAL_PATH even if autoconf-archive isn't
> used/installed. We could decide it is OK, but it should be mentioned.
> Another side effect is that once autoconf-archive has been installed,
> *all* packages will see the m4 macros from autoconf-archive, not only
> the few packages that explicitly need autoconf-archive. I don't know if
> this is a problem, but it can potentially have some side effects.
> 
> Also your patch should drop all the <pkg>_AUTORECONF_OPTS =
> --include=$(HOST_DIR)/share/autoconf-archive, because they become
> redundant.

After thinking a bit on this, here's what I think we should try;

For packages that have _AUTORECONF = YES, we forcibly add
host-autoconf-archive to their _DEPENDENCIES, and always set the macros
search path as proposed here. Supposedly, having macros from
autoconf-archive available should not be a cause for failure to
successfully autorconf, otherwise that would not work on random systems
which have it installed for other reasons, like native builds on
standard distros; also, a missing directorry in the macro search list
should not be a cause for failure.

Consequently, we can drop the ad-hoc dependencies in the individual
packages, and drop the ad-hoc include directove as well.

host-autoconf-archive is a plain autocnf package, without dependencies
(save for the autoconf machinery), and it only ever installs a buncha
files, does not compile anything, so it is pretty fast. Adding it to all
autoreconfigured packages should have a minimal, barely noticeable
impact on the build time.

If the above causes too much breakage, then even this patch was going to
be incorrect, as it would unconditionally add the autocon-archive path
to the search list for all packages that indirectly have
autoconf-archive in their dependencies.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Thomas Petazzoni via buildroot]

Hello,

On Sun, 13 Aug 2023 14:40:37 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> After thinking a bit on this, here's what I think we should try;
> 
> For packages that have _AUTORECONF = YES, we forcibly add
> host-autoconf-archive to their _DEPENDENCIES, and always set the macros
> search path as proposed here. Supposedly, having macros from
> autoconf-archive available should not be a cause for failure to
> successfully autorconf, otherwise that would not work on random systems
> which have it installed for other reasons, like native builds on
> standard distros; also, a missing directorry in the macro search list
> should not be a cause for failure.
> 
> Consequently, we can drop the ad-hoc dependencies in the individual
> packages, and drop the ad-hoc include directove as well.
> 
> host-autoconf-archive is a plain autocnf package, without dependencies
> (save for the autoconf machinery), and it only ever installs a buncha
> files, does not compile anything, so it is pretty fast. Adding it to all
> autoreconfigured packages should have a minimal, barely noticeable
> impact on the build time.
> 
> If the above causes too much breakage, then even this patch was going to
> be incorrect, as it would unconditionally add the autocon-archive path
> to the search list for all packages that indirectly have
> autoconf-archive in their dependencies.

We have 336 packages that set AUTORECONF = YES. Out of these 336
packages, only 8 need host-autoconf-archive.

To me, it makes no sense to add host-autoconf-archive to those 336-8
packages that need autoreconf, but do not use any of the macros
provided by host-autoconf-archive. One of Buildroot's beauty is its
minimalism: it builds only what's needed, and every time it builds
something, there is a solid reason for it. I would really dislike if we
were to start building useless dependencies, even if admittedly
host-autoconf-archive is small and quick to build. But small and quick
to build is not the only thing, it's also about whether it makes sense.
I regularly stare at my build going on, and when something gets built
that I don't understand why its gets pulled into the build, I check
with "make graph-depends" why it is there, and sometimes investigate
further to make sure there's a good justification. To me, this is an
important property of Buildroot, and I would really like to keep this
aspect of Buildroot.

Especially, I don't see what problem we would solve by making
host-autoconf-archive a dependency of all packages that do AUTORECONF =
YES. What problem would this solve?

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [RFC PATCH 1/1] package/automake: include .m4 files of autoconf-archive

[Dario Binacchi]

Hi Yann and Thomas,

On Sun, Aug 13, 2023 at 10:41 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sun, 13 Aug 2023 14:40:37 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
>
> > After thinking a bit on this, here's what I think we should try;
> >
> > For packages that have _AUTORECONF = YES, we forcibly add
> > host-autoconf-archive to their _DEPENDENCIES, and always set the macros
> > search path as proposed here. Supposedly, having macros from
> > autoconf-archive available should not be a cause for failure to
> > successfully autorconf, otherwise that would not work on random systems
> > which have it installed for other reasons, like native builds on
> > standard distros; also, a missing directorry in the macro search list
> > should not be a cause for failure.
> >
> > Consequently, we can drop the ad-hoc dependencies in the individual
> > packages, and drop the ad-hoc include directove as well.
> >
> > host-autoconf-archive is a plain autocnf package, without dependencies
> > (save for the autoconf machinery), and it only ever installs a buncha
> > files, does not compile anything, so it is pretty fast. Adding it to all
> > autoreconfigured packages should have a minimal, barely noticeable
> > impact on the build time.
> >
> > If the above causes too much breakage, then even this patch was going to
> > be incorrect, as it would unconditionally add the autocon-archive path
> > to the search list for all packages that indirectly have
> > autoconf-archive in their dependencies.
>
> We have 336 packages that set AUTORECONF = YES. Out of these 336
> packages, only 8 need host-autoconf-archive.
>
> To me, it makes no sense to add host-autoconf-archive to those 336-8
> packages that need autoreconf, but do not use any of the macros
> provided by host-autoconf-archive. One of Buildroot's beauty is its
> minimalism: it builds only what's needed, and every time it builds
> something, there is a solid reason for it. I would really dislike if we
> were to start building useless dependencies, even if admittedly
> host-autoconf-archive is small and quick to build. But small and quick
> to build is not the only thing, it's also about whether it makes sense.
> I regularly stare at my build going on, and when something gets built
> that I don't understand why its gets pulled into the build, I check
> with "make graph-depends" why it is there, and sometimes investigate
> further to make sure there's a good justification. To me, this is an
> important property of Buildroot, and I would really like to keep this
> aspect of Buildroot.
>
> Especially, I don't see what problem we would solve by making
> host-autoconf-archive a dependency of all packages that do AUTORECONF =
> YES. What problem would this solve?
>

I am available to apply the changes suggested by Thomas in order to
submit version 2
of the patch. Alternatively, do you think it's better to stop here and
not proceed further?
As far as I'm concerned, this solution convinces me, but I also admit
that I don't have a
complete understanding of the topic and its implications.

Thanks and regards,

Dario

> Thomas
> --
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering and training
> https://bootlin.com



-- 

Dario Binacchi

Senior Embedded Linux Developer

dario.binacchi@amarulasolutions.com

__________________________________


Amarula Solutions SRL

Via Le Canevare 30, 31100 Treviso, Veneto, IT

T. +39 042 243 5310
info@amarulasolutions.com

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