[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Herve Codina]

Hi,

This series fixes some issues in the TLP build feature.
The starting point was the work done by Thomas Petazzoni on overwrites
detection (PATCH 1).

Based on this work, this series fixes some overwrites, introduces and uses
<PKG>_PER_PACKAGE_TWEAK_HOOKS. This variable is used to collect specific
per-package path tweaks needed by per-package build.
With this tweaks collected in this variable, they can be re-applied
when they are needed (<pkg>_{reconfigure,rebuild,reinstall}).

It also reworks the final {HOST,TARGET}_DIR rsync.
For each packages only files generated by the package are rsynced (PATCH 13).
This is done using an exclusion list computed in (PATCH 12).
In order to avoid modifications performed in target-finalize step to be seen
as overwrites on next build, the final {HOST,TARGET}_DIR do not contain any
hardlink anymore and are a full copy of expected files and directories (PATCH 14).

The last patch (PATCH 15) fixes <pkg>_{reconfigure,rebuild,reinstall} recreating
per-package {HOST,TARGET}_DIR from scratch.
Indeed, for a given package, re-generating the same file is detected as an overwrite.
The simplest way to avoid this is to fully discard previous per-package {HOST,TARGET}_DIR
directory and recreate them.

To have a TLP build fully fonctionnal, several things are still missing:
  - Be sure that all buildroot packages do not perform any overwrites.
  - Undo tweak stuff that have be done for per-package build (fixup-libtool-files,
    fixup-python-files, <PKG>_PER_PACKAGE_TWEAK_HOOKS) after collecting files
    in final {HOST,STAGING}_DIR.
    This mainly means reverting .../per-package/xxxxx/... path to point to
    the correct ones in final {HOST,STAGING}_DIR.

I hope this series will help in TLP build feature.

Best regards,
Herv? Codina

Herve Codina (14):
  package/e2fsprogs: fix fsck overwrite in HOST_DIR
  package/pkg-generic.mk: Remove Info documents dir entry
  package/pkg-generic.mk: Fix .la files overwrite detection
  package/pkg-generic.mk: Perform .la files fixup in per-package
    HOST_DIR
  package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS
  package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS
  package/apache: Move APACHE_FIXUP_APR_LIBTOOL to
    <PKG>_PER_PACKAGE_TWEAK_HOOKS
  package/pkg-python: Remove _sysconfigdata*.pyc files when
    _sysconfigdata*.py are changed
  package/pkg-generic.mk: Move python fixup to generic package
    infrastructure
  package/owfs: Remove Python sysconfigdata fixup
  package/pkg-generic.mk: Generate final rsync exclude file list
  Makefile: Rsync global {TARGET,HOST}_DIR using exclusion file list
  Makefile: Breaks hardlinks in global {TARGET,HOST}_DIR on per-package
    build
  package/pkg-generic.mk: Fix per-package
    <pkg>-{reconfigure,rebuild,reinstall}

Thomas Petazzoni (1):
  package/pkg-generic.mk: detect files overwritten in TARGET_DIR and
    HOST_DIR

 Makefile                       |  24 +++++++-
 package/apache/apache.mk       |   2 +-
 package/apr-util/apr-util.mk   |  12 ++--
 package/e2fsprogs/e2fsprogs.mk |   1 +
 package/owfs/owfs.mk           |   9 ---
 package/pkg-generic.mk         | 103 ++++++++++++++++++++++++++++++++-
 package/pkg-python.mk          |  10 ----
 7 files changed, 134 insertions(+), 27 deletions(-)

-- 
2.31.1

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Herve Codina]

From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Without per-package directory support, a package can happily overwrite
files installed by other packages. Indeed, because the build order
between packages is always guaranteed, Buildroot will always produce
the same output.

However, with per-package directory support, it is absolutely critical
that a given package does not overwrite files already installed by
another package, due to how the final aggregation is done to create
the complete target/, staging/ and host/ folders. Unfortunately, we
currently don't have anything in Buildroot that detects this
situation.

We used to have check-uniq-files, but it was dropped in commit
2496189a4207173e4cd5bbab90256f911175ee57.

This commit is a new implementation of such a detection, which is
based on calculating and verifying MD5 hashes of installed files: the
calculation is done at the beginning of the configure step, the
verification during the newly introduced "install" step that takes
place after all installation steps.

Since preventing file overwrites is really only needed when
per-package directory support is used, and due to this verification
having some overhead, it is only enabled when
BR2_PER_PACKAGE_DIRECTORIES=y. This additional verification cost is
however not too bad as on average, with per-package directory support,
the per-package target/ and host/ directories will contain less files
than with a build that doesn't use per-package directory support. This
helps a bit in mitigating the additional cost of this verification.

Note that we are not handling separately HOST_DIR and STAGING_DIR,
like we're doing with the pkg_size_{before,after} functions. Instead,
the verification on HOST_DIR walks down into the STAGING_DIR.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

This commit is retreived from Thomas's work.
The first version was discussed
https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-9-thomas.petazzoni at bootlin.com/
This new version was not already submitted by Thomas or I missed it.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 45589bcbb4..bb9ff4150a 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -102,6 +102,25 @@ define fixup-libtool-files
 endef
 endif
 
+# Functions to detect overwritten files
+
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+# $(1): base directory to search in
+# $(2): suffix of file (optional)
+define pkg_detect_overwrite_before
+	LC_ALL=C find $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;
+endef
+
+# $(1): base directory to search in
+# $(2): suffix of file (optional)
+define pkg_detect_overwrite_after
+	if test -s $($(PKG)_DIR)/.files$(2).md5 ; then \
+		LC_ALL=C md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5 || \
+		{ echo "ERROR: package $($(PKG)_NAME) has overwritten files installed by a previous package, aborting."; exit 1; } ; \
+	fi
+endef
+endif
+
 # Functions to collect statistics about installed files
 
 # $(1): base directory to search in
@@ -235,6 +254,8 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call pkg_size_before,$(TARGET_DIR))
 	@$(call pkg_size_before,$(STAGING_DIR),-staging)
 	@$(call pkg_size_before,$(HOST_DIR),-host)
+	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
+	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
 	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
@@ -360,6 +381,8 @@ $(BUILD_DIR)/%/.stamp_installed:
 	@$(call pkg_size_after,$(STAGING_DIR),-staging)
 	@$(call pkg_size_after,$(HOST_DIR),-host)
 	@$(call check_bin_arch)
+	@$(call pkg_detect_overwrite_after,$(TARGET_DIR))
+	@$(call pkg_detect_overwrite_after,$(HOST_DIR),-host)
 	$(Q)touch $@
 
 # Remove package sources
-- 
2.31.1

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> From: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
> Without per-package directory support, a package can happily overwrite
> files installed by other packages. Indeed, because the build order
> between packages is always guaranteed, Buildroot will always produce
> the same output.
> 
> However, with per-package directory support, it is absolutely critical
> that a given package does not overwrite files already installed by
> another package, due to how the final aggregation is done to create
> the complete target/, staging/ and host/ folders. Unfortunately, we
> currently don't have anything in Buildroot that detects this
> situation.
> 
> We used to have check-uniq-files, but it was dropped in commit
> 2496189a4207173e4cd5bbab90256f911175ee57.
> 
> This commit is a new implementation of such a detection, which is
> based on calculating and verifying MD5 hashes of installed files: the
> calculation is done at the beginning of the configure step, the
> verification during the newly introduced "install" step that takes
> place after all installation steps.
> 
> Since preventing file overwrites is really only needed when
> per-package directory support is used, and due to this verification
> having some overhead, it is only enabled when
> BR2_PER_PACKAGE_DIRECTORIES=y. This additional verification cost is
> however not too bad as on average, with per-package directory support,
> the per-package target/ and host/ directories will contain less files
> than with a build that doesn't use per-package directory support. This
> helps a bit in mitigating the additional cost of this verification.
> 
> Note that we are not handling separately HOST_DIR and STAGING_DIR,
> like we're doing with the pkg_size_{before,after} functions. Instead,
> the verification on HOST_DIR walks down into the STAGING_DIR.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

From here...

> This commit is retreived from Thomas's work.
> The first version was discussed
> https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-9-thomas.petazzoni at bootlin.com/
> This new version was not already submitted by Thomas or I missed it.

... to here: this should have been a post-commit note, after the ---
line...

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

... here.

Additionally, it would have been nice to summarise the changes made
between the original submission and htis new one, and how the previous
review was handled.

>  package/pkg-generic.mk | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 45589bcbb4..bb9ff4150a 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -102,6 +102,25 @@ define fixup-libtool-files
>  endef
>  endif
>  
> +# Functions to detect overwritten files
> +
> +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_detect_overwrite_before
> +	LC_ALL=C find $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;
> +endef
> +
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_detect_overwrite_after
> +	if test -s $($(PKG)_DIR)/.files$(2).md5 ; then \
> +		LC_ALL=C md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5 || \
> +		{ echo "ERROR: package $($(PKG)_NAME) has overwritten files installed by a previous package, aborting."; exit 1; } ; \
> +	fi
> +endef
> +endif

And now I see that only part of the problem is handled; it only tries
and detect files which content changes; it does not account for files
which type did change. I.e. a symlink that was onverted over to a file,
or the other way around.

So I think we could change the pkg_detect_overwrite_before macro thusly;

    LC_ALL=C find -L $(1) -type f -print0 |...

That should cover both cases, with just the esception that a symlink is
replaced with a file of the same content (or the other way around).
There's just a little quirk, though:

    find: File system loop detected; ?host/usr? is part of the same file
    system loop as ?host?.

Meh, that's starting to be a bit hairy... We can just ignore it
explicitly, maybe? For merged-usr in target, we should not have that
problem, but there is still the option for a custom skeleton, where
people could provide it as well...

So, to summarise: this patch does not cover all cases, but it is still
acceptable, and brings in the necessary infra that we can later extend
should the need arises.

Regards,
Yann E. MORIN.

>  # Functions to collect statistics about installed files
>  
>  # $(1): base directory to search in
> @@ -235,6 +254,8 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call pkg_size_before,$(TARGET_DIR))
>  	@$(call pkg_size_before,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
> +	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
> +	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
>  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
>  	$($(PKG)_CONFIGURE_CMDS)
> @@ -360,6 +381,8 @@ $(BUILD_DIR)/%/.stamp_installed:
>  	@$(call pkg_size_after,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_after,$(HOST_DIR),-host)
>  	@$(call check_bin_arch)
> +	@$(call pkg_detect_overwrite_after,$(TARGET_DIR))
> +	@$(call pkg_detect_overwrite_after,$(HOST_DIR),-host)
>  	$(Q)touch $@
>  
>  # Remove package sources
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 23:31:40 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

[...]
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>  
> 
> From here...
> 
> > This commit is retreived from Thomas's work.
> > The first version was discussed
> > https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-9-thomas.petazzoni at bootlin.com/
> > This new version was not already submitted by Thomas or I missed it.  
> 
> ... to here: this should have been a post-commit note, after the ---
> line...
> 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---  
> 
> ... here.
> 
> Additionally, it would have been nice to summarise the changes made
> between the original submission and htis new one, and how the previous
> review was handled.

I moved the note after a '---' line and added:

   Compared to the first version, this patch has an improved commit message and
   generates the md5sum snapshot using
     'find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'
   instead of
     'cd $(1); LC_ALL=C find . -type f -exec md5sum {} \; > $($(PKG)_DIR)/.files$(2).md5'

> 
> >  package/pkg-generic.mk | 23 +++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> > 
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 45589bcbb4..bb9ff4150a 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -102,6 +102,25 @@ define fixup-libtool-files
> >  endef
> >  endif
> >  
> > +# Functions to detect overwritten files
> > +
> > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> > +# $(1): base directory to search in
> > +# $(2): suffix of file (optional)
> > +define pkg_detect_overwrite_before
> > +	LC_ALL=C find $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;
> > +endef
> > +
> > +# $(1): base directory to search in
> > +# $(2): suffix of file (optional)
> > +define pkg_detect_overwrite_after
> > +	if test -s $($(PKG)_DIR)/.files$(2).md5 ; then \
> > +		LC_ALL=C md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5 || \
> > +		{ echo "ERROR: package $($(PKG)_NAME) has overwritten files installed by a previous package, aborting."; exit 1; } ; \
> > +	fi
> > +endef
> > +endif  
> 
> And now I see that only part of the problem is handled; it only tries
> and detect files which content changes; it does not account for files
> which type did change. I.e. a symlink that was onverted over to a file,
> or the other way around.
> 
> So I think we could change the pkg_detect_overwrite_before macro thusly;
> 
>     LC_ALL=C find -L $(1) -type f -print0 |...

I added '-L' option.

> 
> That should cover both cases, with just the esception that a symlink is
> replaced with a file of the same content (or the other way around).
> There's just a little quirk, though:
> 
>     find: File system loop detected; ?host/usr? is part of the same file
>     system loop as ?host?.
> 
> Meh, that's starting to be a bit hairy... We can just ignore it
> explicitly, maybe? For merged-usr in target, we should not have that
> problem, but there is still the option for a custom skeleton, where
> people could provide it as well...
> 
> So, to summarise: this patch does not cover all cases, but it is still
> acceptable, and brings in the necessary infra that we can later extend
> should the need arises.
> 

Ok, v2 with the changes I mentioned will be sent.

Thanks for the review,
Herv? Codina

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Thomas Petazzoni]

On Tue, 22 Jun 2021 09:40:52 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

>    Compared to the first version, this patch has an improved commit message and
>    generates the md5sum snapshot using
>      'find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'

But is this better ? Due to not cd-ing into the directory, you will
have full absolute paths in the .md5 file, and adding LC_ALL=C is quite
customary too. Ditto for using | xargs instead of -exec.

>    instead of
>      'cd $(1); LC_ALL=C find . -type f -exec md5sum {} \; > $($(PKG)_DIR)/.files$(2).md5'

All in all this version, looked better. Any reason for the change ?

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

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Nicolas Cavallari]

On 22/06/2021 11:30, Thomas Petazzoni wrote:
> On Tue, 22 Jun 2021 09:40:52 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
>>     Compared to the first version, this patch has an improved commit message and
>>     generates the md5sum snapshot using
>>       'find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'
> 
> But is this better ? Due to not cd-ing into the directory, you will
> have full absolute paths in the .md5 file, and adding LC_ALL=C is quite
> customary too. Ditto for using | xargs instead of -exec.

xargs will run only start one md5sum process if the command line size 
permits it, whereas "-exec md5sum {} ;" will start one md5sum per file.

An alternative is "-exec md5sum {} +", which is essentially xargs-like.

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Yann E. MORIN]

Nicolas, All,

On 2021-06-22 11:57 +0200, Nicolas Cavallari spake thusly:
> On 22/06/2021 11:30, Thomas Petazzoni wrote:
> >On Tue, 22 Jun 2021 09:40:52 +0200
> >Herve Codina <herve.codina@bootlin.com> wrote:
> >
> >>    Compared to the first version, this patch has an improved commit message and
> >>    generates the md5sum snapshot using
> >>      'find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'
> >
> >But is this better ? Due to not cd-ing into the directory, you will
> >have full absolute paths in the .md5 file,

But is that even an issue? Those paths are only used for one per-package
directory; they do not follow files around into another package's ppd.

So, relative or absolute, we don't much care.

> >and adding LC_ALL=C is quite
> >customary too.

In this case, do we also even care or need it? I pretty sure the output
of find is not sorted at all; it most probably depend on the order of
dentries in the directory, and those have absolutely no ordering
guarantee...

Note that if you really want to use LC_ALL, passing it in front of 'find'
will not make LC_ALL available to anythin after the pipeline, so you'd
still have xargs (and md5sum) run without it...

> > Ditto for using | xargs instead of -exec.
> xargs will run only start one md5sum process if the command line size
> permits it, whereas "-exec md5sum {} ;" will start one md5sum per file.

Agreed. But I think this is exactly what Thomas suggested to use, right?

> An alternative is "-exec md5sum {} +", which is essentially xargs-like.

I once used that in the past, and it breaks on non-GNU find, because '+'
is a GNUism. So I think using xargs is still better.

Regards,
Yann E. MORIN.

> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 12:24:33 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Nicolas, All,
> 
> On 2021-06-22 11:57 +0200, Nicolas Cavallari spake thusly:
> > On 22/06/2021 11:30, Thomas Petazzoni wrote:  
> > >On Tue, 22 Jun 2021 09:40:52 +0200
> > >Herve Codina <herve.codina@bootlin.com> wrote:
> > >  
> > >>    Compared to the first version, this patch has an improved commit message and
> > >>    generates the md5sum snapshot using
> > >>      'find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;'  
> > >
> > >But is this better ? Due to not cd-ing into the directory, you will
> > >have full absolute paths in the .md5 file,  
> 
> But is that even an issue? Those paths are only used for one per-package
> directory; they do not follow files around into another package's ppd.
> 
> So, relative or absolute, we don't much care.
> 
> > >and adding LC_ALL=C is quite
> > >customary too.  
> 
> In this case, do we also even care or need it? I pretty sure the output
> of find is not sorted at all; it most probably depend on the order of
> dentries in the directory, and those have absolutely no ordering
> guarantee...
> 
> Note that if you really want to use LC_ALL, passing it in front of 'find'
> will not make LC_ALL available to anythin after the pipeline, so you'd
> still have xargs (and md5sum) run without it...
> 
> > > Ditto for using | xargs instead of -exec.  
> > xargs will run only start one md5sum process if the command line size
> > permits it, whereas "-exec md5sum {} ;" will start one md5sum per file.  
> 
> Agreed. But I think this is exactly what Thomas suggested to use, right?
> 
> > An alternative is "-exec md5sum {} +", which is essentially xargs-like.  
> 
> I once used that in the past, and it breaks on non-GNU find, because '+'
> is a GNUism. So I think using xargs is still better.
> 

Well, according to the different exchanges,

I think I can remove LC_ALL (we don't care about any order), and I keep
'... | xargs -0 -r md5sum'.
'cd' is not needed (we do not care about absolute or relative path).

So, the snapshot is taken using:
find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;

and the detection is done using:
md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5

Is that ok for everyone ?


Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 01/15] package/pkg-generic.mk: detect files overwritten in TARGET_DIR and HOST_DIR

[Yann E. MORIN]

Herv?, All,

On 2021-06-24 16:09 +0200, Herve Codina spake thusly:
> On Tue, 22 Jun 2021 12:24:33 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
[--SNIP--]
> Well, according to the different exchanges,
> 
> I think I can remove LC_ALL (we don't care about any order), and I keep
> '... | xargs -0 -r md5sum'.
> 'cd' is not needed (we do not care about absolute or relative path).
> 
> So, the snapshot is taken using:
> find -L $(1) -type f -print0 | xargs -0 -r md5sum > $($(PKG)_DIR)/.files$(2).md5;

I spoke about -L as a suggestion to follow and monitor symlinks.
However, as I said, that does not work, at least for host/, because it
will emit a warning about a directory loop. Also, there is no telling
what users will install i their target/ and we may have a similar
situation for target/.

This warning is not nice as it will happen for each and every packages.

So, drop -L. At the very least, we have the infrastructure in place, and
we can extend it later should we need additional details.

But now that I think about it, the entry-changed-to-omething-else case
is already covered by the existing instrumenttion, as part of the
pkg_size_before/after hooks (find's %y printf directive)

So, yes, just drop the -L.

> and the detection is done using:
> md5sum --quiet -c $($(PKG)_DIR)/.files$(2).md5
> 
> Is that ok for everyone ?

Except for -L, yes, this is a proper summary from my point if view.

Thanks! :-)

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] [PATCH 02/15] package/e2fsprogs: fix fsck overwrite in HOST_DIR

[Herve Codina]

host-e2fsprog overwrites fsck program and some man related files
previously installed by host-util-linux.

This patch simply disable fsck in host-e2fsprog.

host-e2fsprog is used to build final ext{2,3,4} images.
The missing host-e2fsprog fsck tool (filesystem integrity check
tool) in HOST_DIR should not lead to issues.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/e2fsprogs/e2fsprogs.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/package/e2fsprogs/e2fsprogs.mk b/package/e2fsprogs/e2fsprogs.mk
index 0a69690e2f..bd147e737e 100644
--- a/package/e2fsprogs/e2fsprogs.mk
+++ b/package/e2fsprogs/e2fsprogs.mk
@@ -27,6 +27,7 @@ HOST_E2FSPROGS_CONF_OPTS = \
 	--disable-defrag \
 	--disable-e2initrd-helper \
 	--disable-fuse2fs \
+	--disable-fsck \
 	--disable-libblkid \
 	--disable-libuuid \
 	--disable-testio-debug \
-- 
2.31.1

[Buildroot] [PATCH 02/15] package/e2fsprogs: fix fsck overwrite in HOST_DIR

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:17 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> host-e2fsprog overwrites fsck program and some man related files
> previously installed by host-util-linux.

"The host-e2fsprogs package overwrites the fsck program and some
manpages previously installed by the host-util-linux package".

> 
> This patch simply disable fsck in host-e2fsprog.

disableS

host-e2fsprogs

> host-e2fsprog is used to build final ext{2,3,4} images.

host-e2fsprogs

> The missing host-e2fsprog fsck tool (filesystem integrity check

host-e2fsprogs

> tool) in HOST_DIR should not lead to issues.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

With that:

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

> ---
>  package/e2fsprogs/e2fsprogs.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/e2fsprogs/e2fsprogs.mk b/package/e2fsprogs/e2fsprogs.mk
> index 0a69690e2f..bd147e737e 100644
> --- a/package/e2fsprogs/e2fsprogs.mk
> +++ b/package/e2fsprogs/e2fsprogs.mk
> @@ -27,6 +27,7 @@ HOST_E2FSPROGS_CONF_OPTS = \
>  	--disable-defrag \
>  	--disable-e2initrd-helper \
>  	--disable-fuse2fs \
> +	--disable-fsck \
>  	--disable-libblkid \
>  	--disable-libuuid \
>  	--disable-testio-debug \



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

[Buildroot] [PATCH 02/15] package/e2fsprogs: fix fsck overwrite in HOST_DIR

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 22:52:54 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:17 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > host-e2fsprog overwrites fsck program and some man related files
> > previously installed by host-util-linux.  
> 
> "The host-e2fsprogs package overwrites the fsck program and some
> manpages previously installed by the host-util-linux package".
> 

Ok, changed

> > 
> > This patch simply disable fsck in host-e2fsprog.  
> 
> disableS
> 
> host-e2fsprogs

Changed and s/host-e2fsprog/host-e2fsprogs/g

> 
> > host-e2fsprog is used to build final ext{2,3,4} images.  
> 
> host-e2fsprogs
> 
> > The missing host-e2fsprog fsck tool (filesystem integrity check  
> 
> host-e2fsprogs
> 
> > tool) in HOST_DIR should not lead to issues.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> With that:
> 
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Added.
I will remove this Reviewed-by in my v2 patch if more modifications are needed
for this patch.

Thanks for the review,
Herv? Codina

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 02/15] package/e2fsprogs: fix fsck overwrite in HOST_DIR

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> host-e2fsprog overwrites fsck program and some man related files
> previously installed by host-util-linux.
> 
> This patch simply disable fsck in host-e2fsprog.
> 
> host-e2fsprog is used to build final ext{2,3,4} images.
> The missing host-e2fsprog fsck tool (filesystem integrity check
> tool) in HOST_DIR should not lead to issues.

And the target variant is not susceptible to that issue, because
e2fsprogs' fsck depends on !BR2_PACKAGE_UTIL_LINUX_FSCK. So we're clean
there.

Except for the comments by Thomas, you can carry my:

    Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

when you respin this patch.

Regards,
Yann E. MORIN.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/e2fsprogs/e2fsprogs.mk | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/package/e2fsprogs/e2fsprogs.mk b/package/e2fsprogs/e2fsprogs.mk
> index 0a69690e2f..bd147e737e 100644
> --- a/package/e2fsprogs/e2fsprogs.mk
> +++ b/package/e2fsprogs/e2fsprogs.mk
> @@ -27,6 +27,7 @@ HOST_E2FSPROGS_CONF_OPTS = \
>  	--disable-defrag \
>  	--disable-e2initrd-helper \
>  	--disable-fuse2fs \
> +	--disable-fsck \
>  	--disable-libblkid \
>  	--disable-libuuid \
>  	--disable-testio-debug \
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 02/15] package/e2fsprogs: fix fsck overwrite in HOST_DIR

[Herve Codina]

On Tue, 22 Jun 2021 21:40:24 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Herv?, All,
> 
> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > host-e2fsprog overwrites fsck program and some man related files
> > previously installed by host-util-linux.
> > 
> > This patch simply disable fsck in host-e2fsprog.
> > 
> > host-e2fsprog is used to build final ext{2,3,4} images.
> > The missing host-e2fsprog fsck tool (filesystem integrity check
> > tool) in HOST_DIR should not lead to issues.  
> 
> And the target variant is not susceptible to that issue, because
> e2fsprogs' fsck depends on !BR2_PACKAGE_UTIL_LINUX_FSCK. So we're clean
> there.
> 
> Except for the comments by Thomas, you can carry my:
> 
>     Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
> 
> when you respin this patch.

Ok,
Reviewed-by added.

Thanks for the review,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Herve Codina]

Some packages (autotools for instance) install documentation
files using install-info. This program adds an entry in
the Info directory file (share/info/dir) and this causes
TARGET_DIR and/or HOST_DIR overwrite.

In order to avoid this overwrite this patch removes the Info
directory file right after any installation.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index bb9ff4150a..2499c94746 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed:
 	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
+	$(Q) rm -f $(HOST_DIR)/share/info/dir
 	@$(call step_end,install-host)
 	$(Q)touch $@
 
@@ -309,6 +310,7 @@ $(BUILD_DIR)/%/.stamp_staging_installed:
 	$(foreach hook,$($(PKG)_PRE_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
 	+$($(PKG)_INSTALL_STAGING_CMDS)
 	$(foreach hook,$($(PKG)_POST_INSTALL_STAGING_HOOKS),$(call $(hook))$(sep))
+	$(Q) rm -f $(STAGING_DIR)/share/info/dir
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(call MESSAGE,"Fixing package configuration files") ;\
 			$(SED)  "s,$(HOST_DIR), at HOST_DIR@,g" \
@@ -368,6 +370,7 @@ $(BUILD_DIR)/%/.stamp_target_installed:
 		$(or $($(PKG)_INSTALL_INIT_OPENRC), \
 			$($(PKG)_INSTALL_INIT_SYSV)))
 	$(foreach hook,$($(PKG)_POST_INSTALL_TARGET_HOOKS),$(call $(hook))$(sep))
+	$(Q) rm -f $(TARGET_DIR)/share/info/dir
 	$(Q)if test -n "$($(PKG)_CONFIG_SCRIPTS)" ; then \
 		$(RM) -f $(addprefix $(TARGET_DIR)/usr/bin/,$($(PKG)_CONFIG_SCRIPTS)) ; \
 	fi
-- 
2.31.1

[Buildroot] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:18 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Some packages (autotools for instance) install documentation
> files using install-info. This program adds an entry in
> the Info directory file (share/info/dir) and this causes
> TARGET_DIR and/or HOST_DIR overwrite.
> 
> In order to avoid this overwrite this patch removes the Info
> directory file right after any installation.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index bb9ff4150a..2499c94746 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed:
>  	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
>  	+$($(PKG)_INSTALL_CMDS)
>  	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
> +	$(Q) rm -f $(HOST_DIR)/share/info/dir

No space between $(Q) and rm. This should perhaps use $(RM) in fact.

However, I'm not a huge fan of having this right in the middle of the
infrastructure. It feels like a small detail that gets handled in the
middle of super generic infrastructure code.

The issue is that I don't really have a good alternative proposal :-/

Instead of removing that file, ignore it in the overwrite detection,
perhaps?

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

[Buildroot] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 22:51:20 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:18 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > Some packages (autotools for instance) install documentation
> > files using install-info. This program adds an entry in
> > the Info directory file (share/info/dir) and this causes
> > TARGET_DIR and/or HOST_DIR overwrite.
> > 
> > In order to avoid this overwrite this patch removes the Info
> > directory file right after any installation.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  package/pkg-generic.mk | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index bb9ff4150a..2499c94746 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -280,6 +280,7 @@ $(BUILD_DIR)/%/.stamp_host_installed:
> >  	$(foreach hook,$($(PKG)_PRE_INSTALL_HOOKS),$(call $(hook))$(sep))
> >  	+$($(PKG)_INSTALL_CMDS)
> >  	$(foreach hook,$($(PKG)_POST_INSTALL_HOOKS),$(call $(hook))$(sep))
> > +	$(Q) rm -f $(HOST_DIR)/share/info/dir  
> 
> No space between $(Q) and rm. This should perhaps use $(RM) in fact.

Space removed

> 
> However, I'm not a huge fan of having this right in the middle of the
> infrastructure. It feels like a small detail that gets handled in the
> middle of super generic infrastructure code.
> 
> The issue is that I don't really have a good alternative proposal :-/

Maybe using a macro defined closed to fixup-libtool-files and calling
this macro here instead of '$(Q)rm ...' will help.
Do you think it will
be better ?

> 
> Instead of removing that file, ignore it in the overwrite detection,
> perhaps?

This add a little complexity in overwrite detection (filter out) and
I prefer having overwrite detection quite stupid. It checks for
overwrites without any exception.
Adding exception now in the detection mechanism is opening the door to
more and more exceptions.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Thomas Petazzoni]

On Tue, 22 Jun 2021 10:43:43 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> > However, I'm not a huge fan of having this right in the middle of the
> > infrastructure. It feels like a small detail that gets handled in the
> > middle of super generic infrastructure code.
> > 
> > The issue is that I don't really have a good alternative proposal :-/  
> 
> Maybe using a macro defined closed to fixup-libtool-files and calling
> this macro here instead of '$(Q)rm ...' will help.
> Do you think it will be better ?

Either a macro, or a list of files that are removed, perhaps?

> This add a little complexity in overwrite detection (filter out) and
> I prefer having overwrite detection quite stupid. It checks for
> overwrites without any exception.
> Adding exception now in the detection mechanism is opening the door to
> more and more exceptions.

Yes, I understand your argument. As I stated: I'm also not sure which
solution to propose here. I was just not a big fan of this removal of
one specific file, there, in the middle of a highly generic piece of
infrastructure.

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

[Buildroot] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Yann E. MORIN]

Thomas, Herv?, All,

On 2021-06-22 11:34 +0200, Thomas Petazzoni spake thusly:
> On Tue, 22 Jun 2021 10:43:43 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> > > However, I'm not a huge fan of having this right in the middle of the
> > > infrastructure. It feels like a small detail that gets handled in the
> > > middle of super generic infrastructure code.
> > > 
> > > The issue is that I don't really have a good alternative proposal :-/  

I too am not too fond of this, but I don't have an obvious better
solution in mind...

> > Maybe using a macro defined closed to fixup-libtool-files and calling
> > this macro here instead of '$(Q)rm ...' will help.
> > Do you think it will be better ?
> 
> Either a macro, or a list of files that are removed, perhaps?

I think a list+macro would be a better solution.

Alternatively, we could append to post-install hooks, like we do for
pre-configure hooks in the autotools infra for autoreconf et al., for
example...

    # Outside generic-package-inner:

    # $1: base directory (target, staging, host)
    define remove-conflicting-useless-files
        $(Q)$(RM) -rf $(patsubst %, $(1)/%, $($(PKG)_DROP_FILES_OR_DIRS))
    endef
    define REMOVE_CONFLICTING_USELESS_FILES_IN_HOST
        $(call remove-conflicting-useless-files,$(HOST_DIR))
    endef
    define REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING
        $(call remove-conflicting-useless-files,$(STAGING_DIR))
    endef
    define REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET
        $(call remove-conflicting-useless-files,$(TARGET_DIR))
    endef


    # In generic-package-inner:

    $(2)_DROP_FILES_OR_DIRS += /share/info/dir

    # For host packages:
    $(2)_POST_INSTALL_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_HOST

    # For target packages:
    $(2)_POST_INSTALL_STAGING_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING
    $(2)_POST_INSTALL_TARGET_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET

This way, it also paves the way to add other hooks to actually fix some
files instead of removing them...

For example, assume that packages (e.g. 'wonders') can register
themselves against another package (e.g. 'manager') by appending a
line to a text file, like so (over-simplified, of course):

    echo "name=wonders path=/usr/lib/wonders/v42" >>/usr/share/manager/registry

Then we could have a hook that detects that, extracts the delta
installed by the package, stash that somewhere in staging, and have a
target-finalise hook that aggregates all the packages in the end.

> > This add a little complexity in overwrite detection (filter out) and
> > I prefer having overwrite detection quite stupid. It checks for
> > overwrites without any exception.
> > Adding exception now in the detection mechanism is opening the door to
> > more and more exceptions.
> 
> Yes, I understand your argument. As I stated: I'm also not sure which
> solution to propose here. I was just not a big fan of this removal of
> one specific file, there, in the middle of a highly generic piece of
> infrastructure.

Yes, this bothered me too...

Regards,
Yann E. MORIN.

> Thomas
> -- 
> Thomas Petazzoni, co-owner and CEO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 03/15] package/pkg-generic.mk: Remove Info documents dir entry

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 22:18:18 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> I think a list+macro would be a better solution.
> 
> Alternatively, we could append to post-install hooks, like we do for
> pre-configure hooks in the autotools infra for autoreconf et al., for
> example...
> 
>     # Outside generic-package-inner:
> 
>     # $1: base directory (target, staging, host)
>     define remove-conflicting-useless-files
>         $(Q)$(RM) -rf $(patsubst %, $(1)/%, $($(PKG)_DROP_FILES_OR_DIRS))
>     endef
>     define REMOVE_CONFLICTING_USELESS_FILES_IN_HOST
>         $(call remove-conflicting-useless-files,$(HOST_DIR))
>     endef
>     define REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING
>         $(call remove-conflicting-useless-files,$(STAGING_DIR))
>     endef
>     define REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET
>         $(call remove-conflicting-useless-files,$(TARGET_DIR))
>     endef
> 
> 
>     # In generic-package-inner:
> 
>     $(2)_DROP_FILES_OR_DIRS += /share/info/dir
> 
>     # For host packages:
>     $(2)_POST_INSTALL_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_HOST
> 
>     # For target packages:
>     $(2)_POST_INSTALL_STAGING_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_STAGING
>     $(2)_POST_INSTALL_TARGET_HOOKS += REMOVE_CONFLICTING_USELESS_FILES_IN_TARGET
> 
> This way, it also paves the way to add other hooks to actually fix some
> files instead of removing them...
> 
> For example, assume that packages (e.g. 'wonders') can register
> themselves against another package (e.g. 'manager') by appending a
> line to a text file, like so (over-simplified, of course):
> 
>     echo "name=wonders path=/usr/lib/wonders/v42" >>/usr/share/manager/registry
> 
> Then we could have a hook that detects that, extracts the delta
> installed by the package, stash that somewhere in staging, and have a
> target-finalise hook that aggregates all the packages in the end.
> 

I like this solution.

If it is okay for everyone, I will rework my patch accordingly.

Thomas, your opinion ?


Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Herve Codina]

During per-package build, original .la files are modified by
fixup-libtool-files calls.
But since fixup-libtool-files modifies files using sed --in-place,
these modification are done using a temporary file and a call to
rename. Rename breaks the hardlink to the original file and leave the
temporary file in per-package TARGET dir.
As the original file is not modified, this is no longer considered as
an overwrite.

To fix this detection, this patch simply considers the what is done
by fixup-libtool-files is part of the original snapshot used to
detect overwrites. And so, the original snapshot is taken after
fixup-libtool-files call.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 2499c94746..f9564831cc 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -254,9 +254,9 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call pkg_size_before,$(TARGET_DIR))
 	@$(call pkg_size_before,$(STAGING_DIR),-staging)
 	@$(call pkg_size_before,$(HOST_DIR),-host)
+	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
 	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
 	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
-	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
 	$($(PKG)_CONFIGURE_CMDS)
 	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
-- 
2.31.1

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:19 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> During per-package build, original .la files are modified by
> fixup-libtool-files calls.
> But since fixup-libtool-files modifies files using sed --in-place,
> these modification are done using a temporary file and a call to
> rename. Rename breaks the hardlink to the original file and leave the
> temporary file in per-package TARGET dir.
> As the original file is not modified, this is no longer considered as
> an overwrite.
> 
> To fix this detection, this patch simply considers the what is done

"the what" -> "that what"

> by fixup-libtool-files is part of the original snapshot used to
> detect overwrites. And so, the original snapshot is taken after
> fixup-libtool-files call.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

With that:

Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

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

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Herve Codina]

On Mon, 21 Jun 2021 22:54:11 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:19 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > During per-package build, original .la files are modified by
> > fixup-libtool-files calls.
> > But since fixup-libtool-files modifies files using sed --in-place,
> > these modification are done using a temporary file and a call to
> > rename. Rename breaks the hardlink to the original file and leave the
> > temporary file in per-package TARGET dir.
> > As the original file is not modified, this is no longer considered as
> > an overwrite.
> > 
> > To fix this detection, this patch simply considers the what is done  
> 
> "the what" -> "that what"

Fixed.

> 
> > by fixup-libtool-files is part of the original snapshot used to
> > detect overwrites. And so, the original snapshot is taken after
> > fixup-libtool-files call.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> With that:
> 
> Reviewed-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com>


Thanks for this first review.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Yann E. MORIN]

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> During per-package build, original .la files are modified by
> fixup-libtool-files calls.
> But since fixup-libtool-files modifies files using sed --in-place,
> these modification are done using a temporary file and a call to
> rename. Rename breaks the hardlink to the original file and leave the
> temporary file in per-package TARGET dir.
> As the original file is not modified, this is no longer considered as
> an overwrite.
> 
> To fix this detection, this patch simply considers the what is done
> by fixup-libtool-files is part of the original snapshot used to
> detect overwrites. And so, the original snapshot is taken after
> fixup-libtool-files call.

Then this should be squashed together with the first patch, to avoid
introducing the issue just to fix it a few patches down the series.

You should however add a note about that in the commit log of the first
patch, of course, to explain why the overwrite ifnra is inserted after
the .la tweaks.

So, I agree with the explanations, which make sense, but I disagree that
it should be a separate patch...

Regards,
Yann E. MORIN.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 2499c94746..f9564831cc 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -254,9 +254,9 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call pkg_size_before,$(TARGET_DIR))
>  	@$(call pkg_size_before,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
> +	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
>  	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
>  	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
> -	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
>  	$($(PKG)_CONFIGURE_CMDS)
>  	$(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 23:42:23 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > During per-package build, original .la files are modified by
> > fixup-libtool-files calls.
> > But since fixup-libtool-files modifies files using sed --in-place,
> > these modification are done using a temporary file and a call to
> > rename. Rename breaks the hardlink to the original file and leave the
> > temporary file in per-package TARGET dir.
> > As the original file is not modified, this is no longer considered as
> > an overwrite.
> > 
> > To fix this detection, this patch simply considers the what is done
> > by fixup-libtool-files is part of the original snapshot used to
> > detect overwrites. And so, the original snapshot is taken after
> > fixup-libtool-files call.  
> 
> Then this should be squashed together with the first patch, to avoid
> introducing the issue just to fix it a few patches down the series.
> 
> You should however add a note about that in the commit log of the first
> patch, of course, to explain why the overwrite ifnra is inserted after
> the .la tweaks.
> 
> So, I agree with the explanations, which make sense, but I disagree that
> it should be a separate patch...
> 

Well, I have seen this when I created the patches.
I kept them separate because on the first patch, I introduced the tool
to check the overwrites and i would like it to take its snapshot as soon
as possible in the build sequence (ie right after collecting dependencies
files and taking snapshots for current package statistics).
Then I fixed the issue seen by the overwrites detection and I put at the
same level fixing host-e2fsprogs, fixing .la files or a bit later fixing
python with one patch per fix to detail (or try to detail) the issue and
the way I fixed it.

Squashing the 2 patches leads to one patch that introduces the tool and
fixes one of the issues detected by the tool. What about the others issues
detected ? Squash also together with the first patch ? I think it will
produce a huge patch quite complicate to understand even with all individual
commit message squashed.

However, that being said, I can squash this patch (Fix .la files overwrite
detection) with the 1st one (detect files overwritten in TARGET_DIR and
HOST_DIR) if you still think it will be better.

Regards,
Herv? Codina

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Yann E. MORIN]

Herv?, All,

On 2021-06-22 11:31 +0200, Herve Codina spake thusly:
> On Mon, 21 Jun 2021 23:42:23 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > > During per-package build, original .la files are modified by
> > > fixup-libtool-files calls.
> > > But since fixup-libtool-files modifies files using sed --in-place,
> > > these modification are done using a temporary file and a call to
> > > rename. Rename breaks the hardlink to the original file and leave the
> > > temporary file in per-package TARGET dir.
> > > As the original file is not modified, this is no longer considered as
> > > an overwrite.
> > > 
> > > To fix this detection, this patch simply considers the what is done
> > > by fixup-libtool-files is part of the original snapshot used to
> > > detect overwrites. And so, the original snapshot is taken after
> > > fixup-libtool-files call.  
> > Then this should be squashed together with the first patch, to avoid
> > introducing the issue just to fix it a few patches down the series.
> > 
> > You should however add a note about that in the commit log of the first
> > patch, of course, to explain why the overwrite ifnra is inserted after
> > the .la tweaks.
> > 
> > So, I agree with the explanations, which make sense, but I disagree that
> > it should be a separate patch...
> > 
> Well, I have seen this when I created the patches.
> I kept them separate because on the first patch, I introduced the tool
> to check the overwrites and i would like it to take its snapshot as soon
> as possible in the build sequence (ie right after collecting dependencies
> files and taking snapshots for current package statistics).
> Then I fixed the issue seen by the overwrites detection and I put at the
> same level fixing host-e2fsprogs, fixing .la files or a bit later fixing
> python with one patch per fix to detail (or try to detail) the issue and
> the way I fixed it.

But really, it *is* the first patch that introduces the issue, so it
should be fixed from the onset, rather than after-the-fact.

> Squashing the 2 patches leads to one patch that introduces the tool and
> fixes one of the issues detected by the tool.

Sorry, but this patch (4/15) is fixing an issue introduced by the first
patch, with:

        @$(call pkg_size_before,$(TARGET_DIR))
        @$(call pkg_size_before,$(STAGING_DIR),-staging)
        @$(call pkg_size_before,$(HOST_DIR),-host)
    +   @$(call pkg_detect_overwrite_before,$(TARGET_DIR))
    +   @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
        $(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
        $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
        $($(PKG)_CONFIGURE_CMDS)

... when it should directly be:

        @$(call pkg_size_before,$(STAGING_DIR),-staging)
        @$(call pkg_size_before,$(HOST_DIR),-host)
        $(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
    +   @$(call pkg_detect_overwrite_before,$(TARGET_DIR))
    +   @$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
        $(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
        $($(PKG)_CONFIGURE_CMDS)
        $(foreach hook,$($(PKG)_POST_CONFIGURE_HOOKS),$(call $(hook))$(sep))

And as I said, the commit log should explain why the 'overwrite' calls
are inserted after the .la fixup.

> What about the others issues
> detected ? Squash also together with the first patch ? I think it will
> produce a huge patch quite complicate to understand even with all individual
> commit message squashed.

Ideally, I would say a series should first fix the issues, then
introduce the tooling.

Otherwise, if only the first patch(es) are applied, the tree is broken:
indeed the tooling has been applied, and thus is used, but the issues
are still there.

Also, it is usually easier and less controversial to get fixes applied,
than new tooling.

> However, that being said, I can squash this patch (Fix .la files overwrite
> detection) with the 1st one (detect files overwritten in TARGET_DIR and
> HOST_DIR) if you still think it will be better.

Yes, I still think that it is better.

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] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Thomas Petazzoni]

On Tue, 22 Jun 2021 11:56:09 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Sorry, but this patch (4/15) is fixing an issue introduced by the first
> patch, with:

I agree in principle. But here, it's really a new requirement that we
are adding on packages, and we *know* that even with Herv? patches
there will be some other packages that will need fixing.

So to me it's not really like introducing a regression in a patch, and
fixing it later.

> > What about the others issues
> > detected ? Squash also together with the first patch ? I think it will
> > produce a huge patch quite complicate to understand even with all individual
> > commit message squashed.  
> 
> Ideally, I would say a series should first fix the issues, then
> introduce the tooling.

No, please keep one series, but having the fixes *before* we introduce
the check. And as stated above, the fixes will not fix all problems,
they will only fix the problems we know about. More problems will be
detected by the autobuilders thanks to the overwrite check.

> > However, that being said, I can squash this patch (Fix .la files overwrite
> > detection) with the 1st one (detect files overwritten in TARGET_DIR and
> > HOST_DIR) if you still think it will be better.  
> 
> Yes, I still think that it is better.

No, please don't squash, have fixes added before the overwrite check
instead.

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

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Yann E. MORIN]

Thomas, All,

On 2021-06-22 12:12 +0200, Thomas Petazzoni spake thusly:
> On Tue, 22 Jun 2021 11:56:09 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > Sorry, but this patch (4/15) is fixing an issue introduced by the first
> > patch, with:
> I agree in principle. But here, it's really a new requirement that we
> are adding on packages, and we *know* that even with Herv? patches
> there will be some other packages that will need fixing.

Sorry, but this patch is fixing the infra, not the packages.

> So to me it's not really like introducing a regression in a patch, and
> fixing it later.

I never said it was a regression. ;-)

> > > What about the others issues
> > > detected ? Squash also together with the first patch ? I think it will
> > > produce a huge patch quite complicate to understand even with all individual
> > > commit message squashed.  
> > 
> > Ideally, I would say a series should first fix the issues, then
> > introduce the tooling.
> 
> No, please keep one series, but having the fixes *before* we introduce
> the check.

Exactly; I never said to split the series into two series. It should
still be one.

> And as stated above, the fixes will not fix all problems,
> they will only fix the problems we know about. More problems will be
> detected by the autobuilders thanks to the overwrite check.

And this is totally OK. [0]

> > > However, that being said, I can squash this patch (Fix .la files overwrite
> > > detection) with the 1st one (detect files overwritten in TARGET_DIR and
> > > HOST_DIR) if you still think it will be better.  
> > 
> > Yes, I still think that it is better.
> 
> No, please don't squash, have fixes added before the overwrite check
> instead.

Well, I still disagree, because this patch really fixes an issue
introduced *in the infra* by the first patch.

Regards,
Yann E. MORIN.

[0] but brace for impact! ;-]

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

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 12:30:39 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> On 2021-06-22 12:12 +0200, Thomas Petazzoni spake thusly:
> > On Tue, 22 Jun 2021 11:56:09 +0200
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:  
> > > Sorry, but this patch (4/15) is fixing an issue introduced by the first
> > > patch, with:  
> > I agree in principle. But here, it's really a new requirement that we
> > are adding on packages, and we *know* that even with Herv? patches
> > there will be some other packages that will need fixing.  
> 
> Sorry, but this patch is fixing the infra, not the packages.
> 
> > So to me it's not really like introducing a regression in a patch, and
> > fixing it later.  
> 
> I never said it was a regression. ;-)
> 
> > > > What about the others issues
> > > > detected ? Squash also together with the first patch ? I think it will
> > > > produce a huge patch quite complicate to understand even with all individual
> > > > commit message squashed.    
> > > 
> > > Ideally, I would say a series should first fix the issues, then
> > > introduce the tooling.  
> > 
> > No, please keep one series, but having the fixes *before* we introduce
> > the check.  
> 
> Exactly; I never said to split the series into two series. It should
> still be one.
> 
> > And as stated above, the fixes will not fix all problems,
> > they will only fix the problems we know about. More problems will be
> > detected by the autobuilders thanks to the overwrite check.  
> 
> And this is totally OK. [0]
> 
> > > > However, that being said, I can squash this patch (Fix .la files overwrite
> > > > detection) with the 1st one (detect files overwritten in TARGET_DIR and
> > > > HOST_DIR) if you still think it will be better.    
> > > 
> > > Yes, I still think that it is better.  
> > 
> > No, please don't squash, have fixes added before the overwrite check
> > instead.  
> 
> Well, I still disagree, because this patch really fixes an issue
> introduced *in the infra* by the first patch.
> 

Well, my bad.

I think that the better thing to do is to fully rework the history.
First fixing overwrites and then introducing the overwrite detection tooling
(ie the actual PATCH 1 will be moved just before actual PATCH 12 and so
actual PATCH 4 simply disappears).

The patch introducing the tooling will explain in its commit message
why the calls to fixup-libtool-files and fixup-python-files are performed
before taking the overwrite snapshot and why it is safe (sed --in-place).

The variable <PKG>_PER_PACKAGE_TWEAK_HOOKS (or other name) will be introduced
before the tooling. The patches changing some packages to use this variable
(move tweaks from <PKG>_POST_CONFIGURE_HOOKS to <PKG>_PER_PACKAGE_TWEAK_HOOKS)
will also be introduced before the tooling even if these changes really make
sense after the tooling introduction.
Indeed, the notion of action done before taking the overwrite detection
snapshot (<PKG>_PER_PACKAGE_TWEAK_HOOKS) and action done after
(<PKG>_PRE_CONFIGURE_HOOKS, configure and <PKG>_POST_CONFIGURE_HOOKS)
makes sense only with the overwrite detection tool.

Is that seem better for everyone ?


Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 04/15] package/pkg-generic.mk: Fix .la files overwrite detection

[Yann E. MORIN]

Herv?, All,

On 2021-06-24 17:44 +0200, Herve Codina spake thusly:
> On Tue, 22 Jun 2021 12:30:39 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
[--SNIP--]
> > Well, I still disagree, because this patch really fixes an issue
> > introduced *in the infra* by the first patch.
> Well, my bad.

No worries, really! This series is quite the complex one. ;-)

> I think that the better thing to do is to fully rework the history.
> First fixing overwrites and then introducing the overwrite detection tooling
> (ie the actual PATCH 1 will be moved just before actual PATCH 12 and so
> actual PATCH 4 simply disappears).
> 
> The patch introducing the tooling will explain in its commit message
> why the calls to fixup-libtool-files and fixup-python-files are performed
> before taking the overwrite snapshot and why it is safe (sed --in-place).

Exactly! :-)

> The variable <PKG>_PER_PACKAGE_TWEAK_HOOKS (or other name) will be introduced
> before the tooling. The patches changing some packages to use this variable
> (move tweaks from <PKG>_POST_CONFIGURE_HOOKS to <PKG>_PER_PACKAGE_TWEAK_HOOKS)
> will also be introduced before the tooling even if these changes really make
> sense after the tooling introduction.
> Indeed, the notion of action done before taking the overwrite detection
> snapshot (<PKG>_PER_PACKAGE_TWEAK_HOOKS) and action done after
> (<PKG>_PRE_CONFIGURE_HOOKS, configure and <PKG>_POST_CONFIGURE_HOOKS)
> makes sense only with the overwrite detection tool.
> 
> Is that seem better for everyone ?

It looks a sane ordering to me, yes. THanks for working on this!

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] [PATCH 05/15] package/pkg-generic.mk: Perform .la files fixup in per-package HOST_DIR

[Herve Codina]

fixup-libtool-files was called on per-package STAGING_DIR.
Some host-xxxx packages have their .la files with directories
pointing outside their own per-package directory.

This commit, calling fixup-libtool-files on HOST_DIR, fixes this
issue.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index f9564831cc..e437050175 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -97,7 +97,8 @@ endif
 # $2: staging directory of the package
 ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
 define fixup-libtool-files
-	$(Q)find $(2)/usr/lib* -name "*.la" | xargs --no-run-if-empty \
+	$(Q)find $(2) \( -path '$(2)/lib*' -o -path '$(2)/usr/lib*' \) \
+		-name "*.la" | xargs --no-run-if-empty \
 		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$(1)/:g"
 endef
 endif
@@ -254,6 +255,7 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call pkg_size_before,$(TARGET_DIR))
 	@$(call pkg_size_before,$(STAGING_DIR),-staging)
 	@$(call pkg_size_before,$(HOST_DIR),-host)
+	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
 	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
 	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
 	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
-- 
2.31.1

[Buildroot] [PATCH 05/15] package/pkg-generic.mk: Perform .la files fixup in per-package HOST_DIR

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:20 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> fixup-libtool-files was called on per-package STAGING_DIR.
> Some host-xxxx packages have their .la files with directories
> pointing outside their own per-package directory.

Was this causing a practical issue, or is being done just to be cleaner?

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

[Buildroot] [PATCH 05/15] package/pkg-generic.mk: Perform .la files fixup in per-package HOST_DIR

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 22:48:12 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:20 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > fixup-libtool-files was called on per-package STAGING_DIR.
> > Some host-xxxx packages have their .la files with directories
> > pointing outside their own per-package directory.  
> 
> Was this causing a practical issue, or is being done just to be cleaner?
> 

I didn't see any practical issue on the builds I did and I didn't try to
find any use-case that leads to an issue.
So this patch is to be cleaner.

Herv? Codina

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 05/15] package/pkg-generic.mk: Perform .la files fixup in per-package HOST_DIR

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> fixup-libtool-files was called on per-package STAGING_DIR.
> Some host-xxxx packages have their .la files with directories
> pointing outside their own per-package directory.
> 
> This commit, calling fixup-libtool-files on HOST_DIR, fixes this
> issue.

Usually, I don't much like that we have a fix for some theoretical bug,
but in this case I think it just does make sense.

And because a .la file is only used to find the libraries at link time,
not runtime, this is has most probably gone unnoticed because, at build
time, the libraries are present in the out-of-package tree, and are
present at runtime in the per-package tree...

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index f9564831cc..e437050175 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -97,7 +97,8 @@ endif
>  # $2: staging directory of the package
>  ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
>  define fixup-libtool-files
> -	$(Q)find $(2)/usr/lib* -name "*.la" | xargs --no-run-if-empty \
> +	$(Q)find $(2) \( -path '$(2)/lib*' -o -path '$(2)/usr/lib*' \) \
> +		-name "*.la" | xargs --no-run-if-empty \
>  		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$(1)/:g"

While at it, can you switch over to using find's -print0 and xarg's -0 ?

>  endef
>  endif
> @@ -254,6 +255,7 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call pkg_size_before,$(TARGET_DIR))
>  	@$(call pkg_size_before,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
> +	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
>  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))

Regards,
Yann E. MORIN.

>  	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
>  	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 05/15] package/pkg-generic.mk: Perform .la files fixup in per-package HOST_DIR

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 12:12:02 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> >  # $2: staging directory of the package
> >  ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> >  define fixup-libtool-files
> > -	$(Q)find $(2)/usr/lib* -name "*.la" | xargs --no-run-if-empty \
> > +	$(Q)find $(2) \( -path '$(2)/lib*' -o -path '$(2)/usr/lib*' \) \
> > +		-name "*.la" | xargs --no-run-if-empty \
> >  		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$(1)/:g"  
> 
> While at it, can you switch over to using find's -print0 and xarg's -0 ?

Yes, changed.

I will also change PATCH 10 of this series. Same construction in fixup-python-files
same sanction.

Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

This hook will be used by package infrastucture or some packages to tweak
external files (ie files not provides by the package itself) when we build
with BR2_PER_PACKAGE_DIRECTORIES set.

This allows to have these tweaks in a specific hook instead of
<PKG>_POST_CONFIGURE_HOOK. And so we can call these specific tweaks
whenever we need them.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index e437050175..4069d2cf3c 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -257,6 +257,7 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call pkg_size_before,$(HOST_DIR),-host)
 	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
 	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
+	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))
 	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
 	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
 	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
@@ -844,6 +845,7 @@ $(2)_PRE_LEGAL_INFO_HOOKS       ?=
 $(2)_POST_LEGAL_INFO_HOOKS      ?=
 $(2)_TARGET_FINALIZE_HOOKS      ?=
 $(2)_ROOTFS_PRE_CMD_HOOKS       ?=
+$(2)_PER_PACKAGE_TWEAK_HOOKS    ?=
 
 ifeq ($$($(2)_TYPE),target)
 ifneq ($$(HOST_$(2)_KCONFIG_VAR),)
-- 
2.31.1

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:21 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> This hook will be used by package infrastucture or some packages to tweak
> external files (ie files not provides by the package itself) when we build
> with BR2_PER_PACKAGE_DIRECTORIES set.
> 
> This allows to have these tweaks in a specific hook instead of
> <PKG>_POST_CONFIGURE_HOOK. And so we can call these specific tweaks
> whenever we need them.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

I think this commit log needs an explanation as to why this is
necessary, i.e why isn't the current solution of POST_CONFIGURE_HOOKS
working.

Thanks!

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

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> This hook will be used by package infrastucture or some packages to tweak
> external files (ie files not provides by the package itself) when we build
> with BR2_PER_PACKAGE_DIRECTORIES set.
> 
> This allows to have these tweaks in a specific hook instead of
> <PKG>_POST_CONFIGURE_HOOK. And so we can call these specific tweaks
> whenever we need them.

I'm a bit confused, because those new hooks are even before
_PRE_CONFIGURE_HOOKS, so smeantically, they are different from
_POST_CONFIGURE_HOOKS...

So I fail to see how this articulates with POST_CONFIGURE_HOOKS.
Looking at the next patch, about apr-utils, does not explain why the
existing apr-utils fixups need to change, with this new hooks...

Ah, yes, yes, now I see. OK, so here's a suggestion for a commit log:

    package/pkg-generic: add early pre-configure hooks

    Currently, when a package needs to modify files it inherits from its
    dependencies, because they contain paths, we can only do that in a
    pre- or post-configure hook.

    However, whatever is done as part of those hooks, will be accounted
    to the package itself, and thus will trigger file-overwrite detection.

    So, we need a way to be able to actually modify files before we
    start monitoring for them.

    We introduce a new set of hooks that an individual package can set,
    or that a package infra can set, and that are called right before
    we snapshot the state of target, and host (to which staging belongs),

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index e437050175..4069d2cf3c 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -257,6 +257,7 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
>  	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
>  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
> +	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))

I am going to bikeshed on the naming of this variable (which is a good
sing that I am pretty much OK with the feature).

I think $(2)_EARLY_PRE_CONFIGURE_HOOKS is more sensible.

>  	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
>  	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
>  	$(foreach hook,$($(PKG)_PRE_CONFIGURE_HOOKS),$(call $(hook))$(sep))
> @@ -844,6 +845,7 @@ $(2)_PRE_LEGAL_INFO_HOOKS       ?=
>  $(2)_POST_LEGAL_INFO_HOOKS      ?=
>  $(2)_TARGET_FINALIZE_HOOKS      ?=
>  $(2)_ROOTFS_PRE_CMD_HOOKS       ?=
> +$(2)_PER_PACKAGE_TWEAK_HOOKS    ?=

Actually, this is totally useless: if the variable is unset, this would
set it to empty, which for 'make' is totally identical (an unset
variable can be expanded, and the expansion is the emopty string).

I know you are just adhering to existing code, and so you should indeed
keep it for the next series, but a cleanup in the infra would be welcome
too! ;-)

Regards,
Yann E. MORIN.

>  ifeq ($$($(2)_TYPE),target)
>  ifneq ($$(HOST_$(2)_KCONFIG_VAR),)
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Thomas Petazzoni]

On Tue, 22 Jun 2021 22:39:09 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Ah, yes, yes, now I see. OK, so here's a suggestion for a commit log:
> 
>     package/pkg-generic: add early pre-configure hooks
> 
>     Currently, when a package needs to modify files it inherits from its
>     dependencies, because they contain paths, we can only do that in a
>     pre- or post-configure hook.
> 
>     However, whatever is done as part of those hooks, will be accounted
>     to the package itself, and thus will trigger file-overwrite detection.
> 
>     So, we need a way to be able to actually modify files before we
>     start monitoring for them.

"before we start monitoring changes in those files".

>     We introduce a new set of hooks that an individual package can set,
>     or that a package infra can set, and that are called right before
>     we snapshot the state of target, and host (to which staging belongs),

But yes, you've perfectly understood the problem being solved here!

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

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Hi,

On Wed, 23 Jun 2021 14:40:20 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Tue, 22 Jun 2021 22:39:09 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> 
> > Ah, yes, yes, now I see. OK, so here's a suggestion for a commit log:
> > 
> >     package/pkg-generic: add early pre-configure hooks
> > 
> >     Currently, when a package needs to modify files it inherits from its
> >     dependencies, because they contain paths, we can only do that in a
> >     pre- or post-configure hook.
> > 
> >     However, whatever is done as part of those hooks, will be accounted
> >     to the package itself, and thus will trigger file-overwrite detection.
> > 
> >     So, we need a way to be able to actually modify files before we
> >     start monitoring for them.  
> 
> "before we start monitoring changes in those files".
> 
> >     We introduce a new set of hooks that an individual package can set,
> >     or that a package infra can set, and that are called right before
> >     we snapshot the state of target, and host (to which staging belongs),  
> 
> But yes, you've perfectly understood the problem being solved here!
> 

This will be the new commit log.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 22:39:09 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> >  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
> > +	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))  
> 
> I am going to bikeshed on the naming of this variable (which is a good
> sing that I am pretty much OK with the feature).
> 
> I think $(2)_EARLY_PRE_CONFIGURE_HOOKS is more sensible.

That's good for me.

Okay for everyone ?

Regards,
Herve

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Hi Yann, All

On Fri, 25 Jun 2021 09:21:20 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Hi,
> 
> On Tue, 22 Jun 2021 22:39:09 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> 
> > >  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
> > > +	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))    
> > 
> > I am going to bikeshed on the naming of this variable (which is a good
> > sing that I am pretty much OK with the feature).
> > 
> > I think $(2)_EARLY_PRE_CONFIGURE_HOOKS is more sensible.  
> 
> That's good for me.
> 
> Okay for everyone ?
> 

The more I read the code, the more I think $(2)_EARLY_PRE_CONFIGURE_HOOKS is not
a good name.

Actions done in this hook are for package preparation.
Yann talks about $(2)-prepare step and even if this step is not implemented as
a buildroot step (.stamp_xxxxxx) this is really a package preparation.

Indeed, we prepare the package for the next step (retrieving dependencies files
for per-package host and target dir) and finally we do the fixups.
Once done, the package is ready and so we take the snapshot for overwrite
detection.

What do you think about $(2)_POST_PREPARE_HOOKS ?

Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 06/15] package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Yann E. MORIN]

Herv?, All,

On 2021-07-02 09:18 +0200, Herve Codina spake thusly:
> On Fri, 25 Jun 2021 09:21:20 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> > On Tue, 22 Jun 2021 22:39:09 +0200
> > "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > > >  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
> > > > +	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))    
> > > I am going to bikeshed on the naming of this variable (which is a good
> > > sing that I am pretty much OK with the feature).
> > > I think $(2)_EARLY_PRE_CONFIGURE_HOOKS is more sensible.  
> > That's good for me.
> 
> Actions done in this hook are for package preparation.
> Yann talks about $(2)-prepare step and even if this step is not implemented as
> a buildroot step (.stamp_xxxxxx) this is really a package preparation.
> 
> Indeed, we prepare the package for the next step (retrieving dependencies files
> for per-package host and target dir) and finally we do the fixups.
> Once done, the package is ready and so we take the snapshot for overwrite
> detection.
> 
> What do you think about $(2)_POST_PREPARE_HOOKS ?

Works for me.

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] [PATCH 07/15] package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Original APR_UTIL_FIX_RULES_MK_LIBTOOL tweaked libtool and rules.mk.

libtool is provided by a dependency (apr). It needs to be tweaked
and, as an apr-util external file, this tweak is relevant in
<PKG>_PER_PACKAGE_TWEAK_HOOKS.

rules.mk is generated by apr-util configure step and it is private
to apr-util. The modification performed needs to be kept in
<PKG>_POST_CONFIGURE_HOOKS.

This commit splits original APR_UTIL_FIX_RULES_MK_LIBTOOL and
attaches each part to the correct hook.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/apr-util/apr-util.mk | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/package/apr-util/apr-util.mk b/package/apr-util/apr-util.mk
index db4df91564..b26d523265 100644
--- a/package/apr-util/apr-util.mk
+++ b/package/apr-util/apr-util.mk
@@ -18,13 +18,17 @@ APR_UTIL_CONF_OPTS = \
 APR_UTIL_CONFIG_SCRIPTS = apu-1-config
 
 ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
-define APR_UTIL_FIX_RULES_MK_LIBTOOL
-	$(SED) 's,$(PER_PACKAGE_DIR)/apr/,$(PER_PACKAGE_DIR)/apr-util/,g' \
-		$(@D)/build/rules.mk
+define APR_UTIL_FIX_LIBTOOL
 	$(SED) 's,$(PER_PACKAGE_DIR)/apr/,$(PER_PACKAGE_DIR)/apr-util/,g' \
 		$(STAGING_DIR)/usr/build-1/libtool
 endef
-APR_UTIL_POST_CONFIGURE_HOOKS += APR_UTIL_FIX_RULES_MK_LIBTOOL
+APR_UTIL_PER_PACKAGE_TWEAK_HOOKS += APR_UTIL_FIX_LIBTOOL
+
+define APR_UTIL_FIX_RULES_MK
+	$(SED) 's,$(PER_PACKAGE_DIR)/apr/,$(PER_PACKAGE_DIR)/apr-util/,g' \
+		$(@D)/build/rules.mk
+endef
+APR_UTIL_POST_CONFIGURE_HOOKS += APR_UTIL_FIX_RULES_MK
 endif
 
 # When iconv is available, then use it to provide charset conversion
-- 
2.31.1

[Buildroot] [PATCH 07/15] package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:22 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> rules.mk is generated by apr-util configure step and it is private
> to apr-util. The modification performed needs to be kept in
> <PKG>_POST_CONFIGURE_HOOKS.

Are you sure ? Both the tweak to build-1/libtool and rules.mk were
introduced in commit 84b4c19e551288911a230c2b73e96bc6e2ed12f9 to solve
per-package issues. So I would strongly suspect that both of them need
to be moved to APR_UTIL_PER_PACKAGE_TWEAK_HOOKS.

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

[Buildroot] [PATCH 07/15] package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 22:56:32 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:22 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > rules.mk is generated by apr-util configure step and it is private
> > to apr-util. The modification performed needs to be kept in
> > <PKG>_POST_CONFIGURE_HOOKS.  
> 
> Are you sure ? Both the tweak to build-1/libtool and rules.mk were
> introduced in commit 84b4c19e551288911a230c2b73e96bc6e2ed12f9 to solve
> per-package issues. So I would strongly suspect that both of them need
> to be moved to APR_UTIL_PER_PACKAGE_TWEAK_HOOKS.
> 

Yes, both of them need to be done but
rules.mk is generated by apr-util itself at configure step.
This file is not present (not yet generated) when APR_UTIL_PER_PACKAGE_TWEAK_HOOKS
are called.

XXX_PER_PACKAGE_TWEAK_HOOKS are for tweaking files external to current package
that leads to overwrite -> Ok for build-1/libtool

rules.mk is not correct for per-package build but it is generated by 
apr-util package for apr-util package without any overwrites
-> Keep tweak in <PKG>_POST_CONFIGURE_HOOKS.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 07/15] package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Yann E. MORIN]

Herv?, All,

On 2021-06-22 11:47 +0200, Herve Codina spake thusly:
> On Mon, 21 Jun 2021 22:56:32 +0200
> Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
> > On Mon, 21 Jun 2021 16:11:22 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> > 
> > > rules.mk is generated by apr-util configure step and it is private
> > > to apr-util. The modification performed needs to be kept in
> > > <PKG>_POST_CONFIGURE_HOOKS.  
> > 
> > Are you sure ? Both the tweak to build-1/libtool and rules.mk were
> > introduced in commit 84b4c19e551288911a230c2b73e96bc6e2ed12f9 to solve
> > per-package issues. So I would strongly suspect that both of them need
> > to be moved to APR_UTIL_PER_PACKAGE_TWEAK_HOOKS.
> > 
> 
> Yes, both of them need to be done but
> rules.mk is generated by apr-util itself at configure step.
> This file is not present (not yet generated) when APR_UTIL_PER_PACKAGE_TWEAK_HOOKS
> are called.
> 
> XXX_PER_PACKAGE_TWEAK_HOOKS are for tweaking files external to current package
> that leads to overwrite -> Ok for build-1/libtool
> 
> rules.mk is not correct for per-package build but it is generated by 
> apr-util package for apr-util package without any overwrites
> -> Keep tweak in <PKG>_POST_CONFIGURE_HOOKS.

Yes, this all makes sense now.

Also, for apr and apr-utils, we are fortunate enough that the tweak is
not required at configure time, but at build time, which so far allowed
us to use POST_CONFIGURE hooks.

However, for some other packages, we might not be so lucky, and we would
have needed pre- and -postconfigure hooks. The pre-hooks can't be used
anymore for such tweaks, because of file-overwrite detection, so we need
those earlier hook.

Makes sense, makes sense.

So, except for the naming of the variable, when you respin, you can
carry my:

    Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> Herv?
> 
> -- 
> Herv? Codina, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 07/15] package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 22:42:18 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:


> So, except for the naming of the variable, when you respin, you can
> carry my:
> 
>     Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
> 

This will be done.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 08/15] package/apache: Move APACHE_FIXUP_APR_LIBTOOL to <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Herve Codina]

APACHE_FIXUP_APR_LIBTOOL tweaks files for per package directory build.
This is typically the kind of operation expected to be in
<PKG>_PER_PACKAGE_TWEAK_HOOKS

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/apache/apache.mk | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/apache/apache.mk b/package/apache/apache.mk
index 7dbd1a4512..f496333174 100644
--- a/package/apache/apache.mk
+++ b/package/apache/apache.mk
@@ -23,7 +23,7 @@ define APACHE_FIXUP_APR_LIBTOOL
 	$(SED) "s@$(PER_PACKAGE_DIR)/[^/]\+/@$(PER_PACKAGE_DIR)/apache/@g" \
 		$(STAGING_DIR)/usr/build-1/libtool
 endef
-APACHE_POST_CONFIGURE_HOOKS += APACHE_FIXUP_APR_LIBTOOL
+APACHE_PER_PACKAGE_TWEAK_HOOKS += APACHE_FIXUP_APR_LIBTOOL
 endif
 
 APACHE_CONF_ENV= \
-- 
2.31.1

[Buildroot] [PATCH 08/15] package/apache: Move APACHE_FIXUP_APR_LIBTOOL to <PKG>_PER_PACKAGE_TWEAK_HOOKS

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> APACHE_FIXUP_APR_LIBTOOL tweaks files for per package directory build.
> This is typically the kind of operation expected to be in
> <PKG>_PER_PACKAGE_TWEAK_HOOKS
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/apache/apache.mk | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/apache/apache.mk b/package/apache/apache.mk
> index 7dbd1a4512..f496333174 100644
> --- a/package/apache/apache.mk
> +++ b/package/apache/apache.mk
> @@ -23,7 +23,7 @@ define APACHE_FIXUP_APR_LIBTOOL
>  	$(SED) "s@$(PER_PACKAGE_DIR)/[^/]\+/@$(PER_PACKAGE_DIR)/apache/@g" \
>  		$(STAGING_DIR)/usr/build-1/libtool
>  endef
> -APACHE_POST_CONFIGURE_HOOKS += APACHE_FIXUP_APR_LIBTOOL
> +APACHE_PER_PACKAGE_TWEAK_HOOKS += APACHE_FIXUP_APR_LIBTOOL

Same, with the variable name:

    Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

>  endif
>  
>  APACHE_CONF_ENV= \
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 09/15] package/pkg-python: Remove _sysconfigdata*.pyc files when _sysconfigdata*.py are changed

[Herve Codina]

In order to avoid _sysconfigdata*.pyc overwrites when they are generated based on
_sysconfigdata*.py changes, this commit simply removes _sysconfigdata*.pyc
whenever _sysconfigdata*.py are changed.

As they are removed, overwrite detection will no longer trig and coherency between
the two files (.py and .pyc) is ensured.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-python.mk | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/package/pkg-python.mk b/package/pkg-python.mk
index 59a48e5a87..b3fde77da5 100644
--- a/package/pkg-python.mk
+++ b/package/pkg-python.mk
@@ -96,7 +96,9 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
 define PKG_PYTHON_FIXUP_SYSCONFIGDATA
 	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
 		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
-		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g"
+		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\
+	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
+		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f
 endef
 endif
 
-- 
2.31.1

[Buildroot] [PATCH 09/15] package/pkg-python: Remove _sysconfigdata*.pyc files when _sysconfigdata*.py are changed

[Thomas Petazzoni]

On Mon, 21 Jun 2021 16:11:24 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> In order to avoid _sysconfigdata*.pyc overwrites when they are generated based on
> _sysconfigdata*.py changes, this commit simply removes _sysconfigdata*.pyc
> whenever _sysconfigdata*.py are changed.
> 
> As they are removed, overwrite detection will no longer trig and coherency between
> the two files (.py and .pyc) is ensured.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-python.mk | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> index 59a48e5a87..b3fde77da5 100644
> --- a/package/pkg-python.mk
> +++ b/package/pkg-python.mk
> @@ -96,7 +96,9 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
>  define PKG_PYTHON_FIXUP_SYSCONFIGDATA
>  	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
>  		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> -		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g"
> +		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\

The semicolon + backslash is not needed, these two commands can (I
believe) be executed as separate shell commands.

> +	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> +		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f

Do we need a --no-run-if-empty on the xargs ? Or perhaps it should be
using find directly, i.e:

	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
		-name "_sysconfigdata*.pyc" -exec rm -f {} \;

(or something like that)

Thanks!

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

[Buildroot] [PATCH 09/15] package/pkg-python: Remove _sysconfigdata*.pyc files when _sysconfigdata*.py are changed

[Herve Codina]

On Mon, 21 Jun 2021 17:12:02 +0200
Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:

> On Mon, 21 Jun 2021 16:11:24 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > In order to avoid _sysconfigdata*.pyc overwrites when they are generated based on
> > _sysconfigdata*.py changes, this commit simply removes _sysconfigdata*.pyc
> > whenever _sysconfigdata*.py are changed.
> > 
> > As they are removed, overwrite detection will no longer trig and coherency between
> > the two files (.py and .pyc) is ensured.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  package/pkg-python.mk | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> > index 59a48e5a87..b3fde77da5 100644
> > --- a/package/pkg-python.mk
> > +++ b/package/pkg-python.mk
> > @@ -96,7 +96,9 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> >  define PKG_PYTHON_FIXUP_SYSCONFIGDATA
> >  	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> >  		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> > -		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g"
> > +		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\  
> 
> The semicolon + backslash is not needed, these two commands can (I
> believe) be executed as separate shell commands.

Yes, they can be executed in separate shell commands
-> I removed the semicolon and backslash.

> 
> > +	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> > +		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f  
> 
> Do we need a --no-run-if-empty on the xargs ? Or perhaps it should be

It is cleaner to avoid running rm is there is nothing to remove.
So, I prefer keeping the xargs -r option.

> using find directly, i.e:
> 
> 	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> 		-name "_sysconfigdata*.pyc" -exec rm -f {} \;
> 
> (or something like that)

Using "-exec rm -f {} \;" will lead to start as many rm as there are files
to remove whereas xargs will run only one rm.


Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 09/15] package/pkg-python: Remove _sysconfigdata*.pyc files when _sysconfigdata*.py are changed

[Yann E. MORIN]

Herv?, All,

On 2021-06-22 19:52 +0200, Herve Codina spake thusly:
> On Mon, 21 Jun 2021 17:12:02 +0200
> Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote:
> > On Mon, 21 Jun 2021 16:11:24 +0200
> > Herve Codina <herve.codina@bootlin.com> wrote:
> > > In order to avoid _sysconfigdata*.pyc overwrites when they are generated based on
> > > _sysconfigdata*.py changes, this commit simply removes _sysconfigdata*.pyc
> > > whenever _sysconfigdata*.py are changed.
> > > 
> > > As they are removed, overwrite detection will no longer trig and coherency between
> > > the two files (.py and .pyc) is ensured.
> > > 
> > > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > > ---
> > >  package/pkg-python.mk | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> > > index 59a48e5a87..b3fde77da5 100644
> > > --- a/package/pkg-python.mk
> > > +++ b/package/pkg-python.mk
> > > @@ -96,7 +96,9 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> > >  define PKG_PYTHON_FIXUP_SYSCONFIGDATA
> > >  	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> > >  		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> > > -		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g"
> > > +		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\  
[--SNIP--]
> > > +	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> > > +		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f  
> > Do we need a --no-run-if-empty on the xargs ? Or perhaps it should be
> It is cleaner to avoid running rm is there is nothing to remove.
> So, I prefer keeping the xargs -r option.
> 
> > using find directly, i.e:
> > 
> > 	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> > 		-name "_sysconfigdata*.pyc" -exec rm -f {} \;
> > 
> > (or something like that)
> 
> Using "-exec rm -f {} \;" will lead to start as many rm as there are files
> to remove whereas xargs will run only one rm.

So, 1: wrong. xargs may have to exec more than one rm if there are too
many arguments, or the command line is too long. But of course, this is
inconsequential, because even on a huge build, this would be just a few
exec instead of tens of thousands.

And 2: why not using find's -delete option to begin with:

    find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
        -name "_sysconfigdata*.pyc" -delete

There, not even a pipe, or an exec of anything else...

Regards,
Yann E. MORIN.

> 
> Herv?
> 
> -- 
> Herv? Codina, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 09/15] package/pkg-python: Remove _sysconfigdata*.pyc files when _sysconfigdata*.py are changed

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 22:50:40 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> And 2: why not using find's -delete option to begin with:
> 
>     find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
>         -name "_sysconfigdata*.pyc" -delete
> 
> There, not even a pipe, or an exec of anything else...
> 

Yes, indeed.
I just didn't think about '-delete' when I wrote the "find ...".

Changed to '-delete' option.


Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 10/15] package/pkg-generic.mk: Move python fixup to generic package infrastructure

[Herve Codina]

Fixing _sysconfigdata*.{py,pyc} was previously done by python package
infrastructure. Some packages use python stuff without using python
package infrastructure.
These packages perform overwrites and need the specific python fixup
to fix them.

In order to be sure to fix all of these packages, the python fixup
is moved to the generic package infrastructure and applied to all
packages.
This follows the same principle as for the .la libtool files fixup.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 17 +++++++++++++++++
 package/pkg-python.mk  | 12 ------------
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 4069d2cf3c..918e2381af 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -103,6 +103,21 @@ define fixup-libtool-files
 endef
 endif
 
+# Make sure python _sysconfigdata*.py files only reference the current
+# per-package directory.
+
+# $1: package name (lower case)
+# $2: staging or host directory of the package
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+define fixup-python-files
+	$(Q)find $(2) \( -path '$(2)/lib/python*' -o -path '$(2)/usr/lib/python*' \) \
+		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
+		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$(1)/:g" ;\
+	find $(2) \( -path '$(2)/lib/python*' -o -path '$(2)/usr/lib/python*' \) \
+		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f
+endef
+endif
+
 # Functions to detect overwritten files
 
 ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
@@ -257,6 +272,8 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call pkg_size_before,$(HOST_DIR),-host)
 	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
 	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
+	$(call fixup-python-files,$(NAME),$(HOST_DIR))
+	$(call fixup-python-files,$(NAME),$(STAGING_DIR))
 	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))
 	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
 	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
diff --git a/package/pkg-python.mk b/package/pkg-python.mk
index b3fde77da5..e6b81bdfd3 100644
--- a/package/pkg-python.mk
+++ b/package/pkg-python.mk
@@ -92,16 +92,6 @@ HOST_PKG_PYTHON_SETUPTOOLS_INSTALL_OPTS = \
 	--root=/ \
 	--single-version-externally-managed
 
-ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
-define PKG_PYTHON_FIXUP_SYSCONFIGDATA
-	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
-		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
-		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\
-	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
-		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f
-endef
-endif
-
 ################################################################################
 # inner-python-package -- defines how the configuration, compilation
 # and installation of a Python package should be done, implements a
@@ -246,8 +236,6 @@ $(2)_PYTHON_INTERPRETER = $$(HOST_DIR)/bin/$$($(2)_NEEDS_HOST_PYTHON)
 endif
 endif
 
-$(2)_PRE_CONFIGURE_HOOKS += PKG_PYTHON_FIXUP_SYSCONFIGDATA
-
 #
 # Build step. Only define it if not already defined by the package .mk
 # file.
-- 
2.31.1

[Buildroot] [PATCH 10/15] package/pkg-generic.mk: Move python fixup to generic package infrastructure

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> Fixing _sysconfigdata*.{py,pyc} was previously done by python package
> infrastructure. Some packages use python stuff without using python
> package infrastructure.
> These packages perform overwrites and need the specific python fixup
> to fix them.
> 
> In order to be sure to fix all of these packages, the python fixup
> is moved to the generic package infrastructure and applied to all
> packages.
> This follows the same principle as for the .la libtool files fixup.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 17 +++++++++++++++++
>  package/pkg-python.mk  | 12 ------------
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 4069d2cf3c..918e2381af 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -103,6 +103,21 @@ define fixup-libtool-files
>  endef
>  endif
>  
> +# Make sure python _sysconfigdata*.py files only reference the current
> +# per-package directory.
> +
> +# $1: package name (lower case)
> +# $2: staging or host directory of the package
> +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> +define fixup-python-files
> +	$(Q)find $(2) \( -path '$(2)/lib/python*' -o -path '$(2)/usr/lib/python*' \) \
> +		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> +		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$(1)/:g" ;\
> +	find $(2) \( -path '$(2)/lib/python*' -o -path '$(2)/usr/lib/python*' \) \
> +		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f
> +endef
> +endif
> +
>  # Functions to detect overwritten files
>  
>  ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> @@ -257,6 +272,8 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
>  	$(call fixup-libtool-files,$(NAME),$(HOST_DIR))
>  	$(call fixup-libtool-files,$(NAME),$(STAGING_DIR))
> +	$(call fixup-python-files,$(NAME),$(HOST_DIR))
> +	$(call fixup-python-files,$(NAME),$(STAGING_DIR))
>  	$(foreach hook,$($(PKG)_PER_PACKAGE_TWEAK_HOOKS),$(call $(hook))$(sep))

So, earlier in the series, you introduced _PER_PACKAGE_TWEAK_HOOKS (or
whatever the name it will eventually bear), stating that infras could
set it, but here  you are not taking the chance to demonstrate this,
and instead you are inserting the python fixups in an ad-hoc way.

Which also illustrates that the libtool fixups should be converted to
the _PER_PACKAGE_TWEAK_HOOKS, probably...

Otherwise, this is a good change.

Regards,
Yann E. MORIN.

>  	@$(call pkg_detect_overwrite_before,$(TARGET_DIR))
>  	@$(call pkg_detect_overwrite_before,$(HOST_DIR),-host)
> diff --git a/package/pkg-python.mk b/package/pkg-python.mk
> index b3fde77da5..e6b81bdfd3 100644
> --- a/package/pkg-python.mk
> +++ b/package/pkg-python.mk
> @@ -92,16 +92,6 @@ HOST_PKG_PYTHON_SETUPTOOLS_INSTALL_OPTS = \
>  	--root=/ \
>  	--single-version-externally-managed
>  
> -ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> -define PKG_PYTHON_FIXUP_SYSCONFIGDATA
> -	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> -		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> -		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/$($(PKG)_NAME)/:g" ;\
> -	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> -		-name "_sysconfigdata*.pyc" -print0 | xargs -0 -r rm -f
> -endef
> -endif
> -
>  ################################################################################
>  # inner-python-package -- defines how the configuration, compilation
>  # and installation of a Python package should be done, implements a
> @@ -246,8 +236,6 @@ $(2)_PYTHON_INTERPRETER = $$(HOST_DIR)/bin/$$($(2)_NEEDS_HOST_PYTHON)
>  endif
>  endif
>  
> -$(2)_PRE_CONFIGURE_HOOKS += PKG_PYTHON_FIXUP_SYSCONFIGDATA
> -
>  #
>  # Build step. Only define it if not already defined by the package .mk
>  # file.
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 10/15] package/pkg-generic.mk: Move python fixup to generic package infrastructure

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 23:01:03 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> So, earlier in the series, you introduced _PER_PACKAGE_TWEAK_HOOKS (or
> whatever the name it will eventually bear), stating that infras could
> set it, but here  you are not taking the chance to demonstrate this,
> and instead you are inserting the python fixups in an ad-hoc way.
> 
> Which also illustrates that the libtool fixups should be converted to
> the _PER_PACKAGE_TWEAK_HOOKS, probably...
> 

Yes that's perfectly true.

But do you think we need it right now in this patch series.
I really would prefer to keep it as it is for this patch series.

The series is already quite complex and I think that converting
libtool and python fixups to PER_PACKAGE_TWEAK_HOOKS (or other name)
can be done afterward.

Herve

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 10/15] package/pkg-generic.mk: Move python fixup to generic package infrastructure

[Yann E. MORIN]

Herv?, All,

On 2021-06-25 10:22 +0200, Herve Codina spake thusly:
> On Tue, 22 Jun 2021 23:01:03 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > So, earlier in the series, you introduced _PER_PACKAGE_TWEAK_HOOKS (or
> > whatever the name it will eventually bear), stating that infras could
> > set it, but here  you are not taking the chance to demonstrate this,
> > and instead you are inserting the python fixups in an ad-hoc way.
> > 
> > Which also illustrates that the libtool fixups should be converted to
> > the _PER_PACKAGE_TWEAK_HOOKS, probably...
> Yes that's perfectly true.
> 
> But do you think we need it right now in this patch series.
> I really would prefer to keep it as it is for this patch series.
> 
> The series is already quite complex and I think that converting
> libtool and python fixups to PER_PACKAGE_TWEAK_HOOKS (or other name)
> can be done afterward.

I am always a bit wary about the "cleanup can be done in a later pass"
excuse, because that later pass almost never manifests.

But still, the issue is real, and we better have a solution rather than
nothing, and we better have it earlier rather than later. So yes, keep
things as they are.

Regards,
Yann E. MORIN, first-shoted in the morning!

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 11/15] package/owfs: Remove Python sysconfigdata fixup

[Herve Codina]

This fixup is done at pkg-generic level for all packages.
So, it is no more needed in owfs package.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/owfs/owfs.mk | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/package/owfs/owfs.mk b/package/owfs/owfs.mk
index ffc0b3098d..10543d3698 100644
--- a/package/owfs/owfs.mk
+++ b/package/owfs/owfs.mk
@@ -88,15 +88,6 @@ OWFS_DEPENDENCIES += python host-swig
 # override the PYSITEDIR variable in make.
 OWFS_EXTRA_MAKE_OPTS += PYSITEDIR=/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages
 
-ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
-define OWFS_FIXUP_PYTHON_SYSCONFIGDATA
-	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
-		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
-		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/owfs/:g"
-endef
-OWFS_PRE_CONFIGURE_HOOKS += OWFS_FIXUP_PYTHON_SYSCONFIGDATA
-endif
-
 else
 OWFS_CONF_OPTS += --disable-owpython --without-python
 endif
-- 
2.31.1

[Buildroot] [PATCH 11/15] package/owfs: Remove Python sysconfigdata fixup

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> This fixup is done at pkg-generic level for all packages.
> So, it is no more needed in owfs package.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

When you respin the series, you can carry my:

    Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> ---
>  package/owfs/owfs.mk | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/package/owfs/owfs.mk b/package/owfs/owfs.mk
> index ffc0b3098d..10543d3698 100644
> --- a/package/owfs/owfs.mk
> +++ b/package/owfs/owfs.mk
> @@ -88,15 +88,6 @@ OWFS_DEPENDENCIES += python host-swig
>  # override the PYSITEDIR variable in make.
>  OWFS_EXTRA_MAKE_OPTS += PYSITEDIR=/usr/lib/python$(PYTHON_VERSION_MAJOR)/site-packages
>  
> -ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> -define OWFS_FIXUP_PYTHON_SYSCONFIGDATA
> -	find $(HOST_DIR)/lib/python* $(STAGING_DIR)/usr/lib/python* \
> -		-name "_sysconfigdata*.py" | xargs --no-run-if-empty \
> -		$(SED) "s:$(PER_PACKAGE_DIR)/[^/]\+/:$(PER_PACKAGE_DIR)/owfs/:g"
> -endef
> -OWFS_PRE_CONFIGURE_HOOKS += OWFS_FIXUP_PYTHON_SYSCONFIGDATA
> -endif
> -
>  else
>  OWFS_CONF_OPTS += --disable-owpython --without-python
>  endif
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 11/15] package/owfs: Remove Python sysconfigdata fixup

[Herve Codina]

On Tue, 22 Jun 2021 23:02:26 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Herv?, All,
> 
> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > This fixup is done at pkg-generic level for all packages.
> > So, it is no more needed in owfs package.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>  
> 
> When you respin the series, you can carry my:
> 
>     Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>
> 

This will be done.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 12/15] package/pkg-generic.mk: Generate final rsync exclude file list

[Herve Codina]

The final rsync performed at {host,target}-finalize steps
need to be done by rsyncing files generated by each packages
without looking at files generated by other packages the current
package depends on. This is needed to avoid overwrites in final
{HOST,TARGET}_DIR.

In order to prepare the final rsync, an exclusion list is generated.
This list lists files that are not generated by the current package and
so files that need to be excluded from the final rsync.

Note also that the files list was not based on .files-list.{before,after}.
During .file-list.{before,after} built for host directory, staging
sub-directory (ie <toolchain>/sysroot) is filtered out.
The final rsync exclusion list needs to take into account the full
{host,target} directory to avoid final overwrites.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 918e2381af..546370af7a 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -162,6 +162,30 @@ define pkg_size_after
 	rm -f $($(PKG)_DIR)/.files-list$(2).after
 endef
 
+# Functions to collect final rsync exclusion files
+
+# $(1): base directory to search in
+# $(2): suffix of file (optional)
+define pkg_final_rsync_before
+	cd $(1); \
+	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
+		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).before
+endef
+
+# $(1): base directory to search in
+# $(2): suffix of file (optional)
+define pkg_final_rsync_after
+	cd $(1); \
+	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
+		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).after
+	LC_ALL=C comm -2 \
+		$($(PKG)_DIR)/.files-final-rsync$(2).before \
+		$($(PKG)_DIR)/.files-final-rsync$(2).after \
+		| sed -r -e 's/^[^,]+,./- /' \
+		> $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync
+	rm -f $($(PKG)_DIR)/.files-final-rsync$(2).after
+endef
+
 define check_bin_arch
 	support/scripts/check-bin-arch -p $($(PKG)_NAME) \
 		-l $($(PKG)_DIR)/.files-list.txt \
@@ -267,6 +291,8 @@ $(BUILD_DIR)/%/.stamp_configured:
 	@$(call MESSAGE,"Configuring")
 	$(Q)mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR) $(BINARIES_DIR)
 	$(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES))
+	@$(call pkg_final_rsync_before,$(TARGET_DIR))
+	@$(call pkg_final_rsync_before,$(HOST_DIR),-host)
 	@$(call pkg_size_before,$(TARGET_DIR))
 	@$(call pkg_size_before,$(STAGING_DIR),-staging)
 	@$(call pkg_size_before,$(HOST_DIR),-host)
@@ -404,6 +430,8 @@ $(BUILD_DIR)/%/.stamp_installed:
 	@$(call pkg_size_after,$(STAGING_DIR),-staging)
 	@$(call pkg_size_after,$(HOST_DIR),-host)
 	@$(call check_bin_arch)
+	@$(call pkg_final_rsync_after,$(TARGET_DIR))
+	@$(call pkg_final_rsync_after,$(HOST_DIR),-host)
 	@$(call pkg_detect_overwrite_after,$(TARGET_DIR))
 	@$(call pkg_detect_overwrite_after,$(HOST_DIR),-host)
 	$(Q)touch $@
-- 
2.31.1

[Buildroot] [PATCH 12/15] package/pkg-generic.mk: Generate final rsync exclude file list

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> The final rsync performed at {host,target}-finalize steps
> need to be done by rsyncing files generated by each packages
> without looking at files generated by other packages the current
> package depends on. This is needed to avoid overwrites in final
> {HOST,TARGET}_DIR.
> 
> In order to prepare the final rsync, an exclusion list is generated.
> This list lists files that are not generated by the current package and
> so files that need to be excluded from the final rsync.
> 
> Note also that the files list was not based on .files-list.{before,after}.
> During .file-list.{before,after} built for host directory, staging
> sub-directory (ie <toolchain>/sysroot) is filtered out.
> The final rsync exclusion list needs to take into account the full
> {host,target} directory to avoid final overwrites.

Ideally, we would in fact redirect the package installation to an emoty
directory, so that we can actually find what it installs.

However, as has been discussed in the past, this is fraught with
unworkable issues. For example, some paths may be hard-coded at
configure time and/or build time, and thus the package would still
install in the original stagin we presented it with (for target/, this
is not an isue, because target/ is never looked at during configure or
build, only at install time). Or a package may try to modify an existing
file (bad, but still). Or a file just assumes that tdirectory structure
exists (bad, but eh...)

So, yes, the rsync exclusion list is a good workaround.

I see you have provided detailed comit logs, that is great. Still, for
such core stuff, I think they still miss the bigger picture, like I
explained above for example, and which should have been part of the
commit log to explain why we resort to an exclusion list rather than the
more obvious and simple empty-DESTDIR.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 918e2381af..546370af7a 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -162,6 +162,30 @@ define pkg_size_after
>  	rm -f $($(PKG)_DIR)/.files-list$(2).after
>  endef
>  
> +# Functions to collect final rsync exclusion files
> +
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_final_rsync_before
> +	cd $(1); \
> +	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> +		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).before
> +endef
> +
> +# $(1): base directory to search in
> +# $(2): suffix of file (optional)
> +define pkg_final_rsync_after
> +	cd $(1); \
> +	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> +		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).after
> +	LC_ALL=C comm -2 \
> +		$($(PKG)_DIR)/.files-final-rsync$(2).before \
> +		$($(PKG)_DIR)/.files-final-rsync$(2).after \
> +		| sed -r -e 's/^[^,]+,./- /' \
> +		> $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync
> +	rm -f $($(PKG)_DIR)/.files-final-rsync$(2).after

You forgot to remove .files-final-rsync$(2).before

> +endef
> +
>  define check_bin_arch
>  	support/scripts/check-bin-arch -p $($(PKG)_NAME) \
>  		-l $($(PKG)_DIR)/.files-list.txt \
> @@ -267,6 +291,8 @@ $(BUILD_DIR)/%/.stamp_configured:
>  	@$(call MESSAGE,"Configuring")
>  	$(Q)mkdir -p $(HOST_DIR) $(TARGET_DIR) $(STAGING_DIR) $(BINARIES_DIR)
>  	$(call prepare-per-package-directory,$($(PKG)_FINAL_DEPENDENCIES))
> +	@$(call pkg_final_rsync_before,$(TARGET_DIR))
> +	@$(call pkg_final_rsync_before,$(HOST_DIR),-host)

The more I look at this, the more I believe we should resurrect the idea
of a 'prepare' step that goes in-between 'patch' and 'configure'...

Regards,
Yann E. MORIN.

>  	@$(call pkg_size_before,$(TARGET_DIR))
>  	@$(call pkg_size_before,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_before,$(HOST_DIR),-host)
> @@ -404,6 +430,8 @@ $(BUILD_DIR)/%/.stamp_installed:
>  	@$(call pkg_size_after,$(STAGING_DIR),-staging)
>  	@$(call pkg_size_after,$(HOST_DIR),-host)
>  	@$(call check_bin_arch)
> +	@$(call pkg_final_rsync_after,$(TARGET_DIR))
> +	@$(call pkg_final_rsync_after,$(HOST_DIR),-host)
>  	@$(call pkg_detect_overwrite_after,$(TARGET_DIR))
>  	@$(call pkg_detect_overwrite_after,$(HOST_DIR),-host)
>  	$(Q)touch $@
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 12/15] package/pkg-generic.mk: Generate final rsync exclude file list

[Herve Codina]

Hi,

On Tue, 22 Jun 2021 23:15:10 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Ideally, we would in fact redirect the package installation to an emoty
> directory, so that we can actually find what it installs.
> 
> However, as has been discussed in the past, this is fraught with
> unworkable issues. For example, some paths may be hard-coded at
> configure time and/or build time, and thus the package would still
> install in the original stagin we presented it with (for target/, this
> is not an isue, because target/ is never looked at during configure or
> build, only at install time). Or a package may try to modify an existing
> file (bad, but still). Or a file just assumes that tdirectory structure
> exists (bad, but eh...)

Modifying existing files is no more allowed -> Overwrites detection.

> 
> So, yes, the rsync exclusion list is a good workaround.
> 
> I see you have provided detailed comit logs, that is great. Still, for
> such core stuff, I think they still miss the bigger picture, like I
> explained above for example, and which should have been part of the
> commit log to explain why we resort to an exclusion list rather than the
> more obvious and simple empty-DESTDIR.
> 

I can add at the end of commit log :
--- 8< ---
Using an empty directory for per-package installation directory would be the
simplest way to find what a package installs.
However, as it has been discussed in the past, this is fraught with unworkable
issues. For example, some paths may be hard-coded at configure time and/or build
time, and thus the package would still install in the original stagin we presented
it with (for target/, this is not an isue, because target/ is never looked at
during configure or build, only@install time). Or a package installation
process just assumes that the directory structure exists (bad, but eh...).
--- 8< ---

Is that ok for you ?


> > +define pkg_final_rsync_after
> > +	cd $(1); \
> > +	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> > +		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).after
> > +	LC_ALL=C comm -2 \
> > +		$($(PKG)_DIR)/.files-final-rsync$(2).before \
> > +		$($(PKG)_DIR)/.files-final-rsync$(2).after \
> > +		| sed -r -e 's/^[^,]+,./- /' \
> > +		> $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync
> > +	rm -f $($(PKG)_DIR)/.files-final-rsync$(2).after  
> 
> You forgot to remove .files-final-rsync$(2).before

I cannot remove it in this patch.

.files-final-rsync$(2).before is generated by .stamp_configured rule calling
pkg_final_rsync_before call and if we run 'make && make foo-rebuild' the file
generated by first make invocation will be no more present for second make
invocation.

We can remove .files-final-rsync$(2).before only after patch
"package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure,rebuild,reinstall}"
Indeed, in this patch, pkg_final_rsync_after is called after the per-package
host and target dir are recreated from scratch.

I will remove .files-final-rsync$(2).before in a dedicated patch after
"package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure,rebuild,reinstall}"

Is that ok for you ?

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 12/15] package/pkg-generic.mk: Generate final rsync exclude file list

[Yann E. MORIN]

Herv?, All,

On 2021-06-25 11:05 +0200, Herve Codina spake thusly:
> On Tue, 22 Jun 2021 23:15:10 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > Ideally, we would in fact redirect the package installation to an emoty
> > directory, so that we can actually find what it installs.
> > 
> > However, as has been discussed in the past, this is fraught with
> > unworkable issues. For example, some paths may be hard-coded at
> > configure time and/or build time, and thus the package would still
> > install in the original stagin we presented it with (for target/, this
> > is not an isue, because target/ is never looked at during configure or
> > build, only at install time). Or a package may try to modify an existing
> > file (bad, but still). Or a file just assumes that tdirectory structure
> > exists (bad, but eh...)
> Modifying existing files is no more allowed -> Overwrites detection.

Exactly. But what I mean is that, for packages that really need to
modify an existing file (e.g. to register themselves against another
package's registry), providing an empty target/ is not viable. Yes, that
will trigger the overwrite-detection, and we will have to find a proper
solution for those. Just, installing in an empty target would not work.

So yes, the list of installed files is still the best solution.

> I can add at the end of commit log :
> --- 8< ---
> Using an empty directory for per-package installation directory would be the
> simplest way to find what a package installs.
> However, as it has been discussed in the past, this is fraught with unworkable
> issues. For example, some paths may be hard-coded at configure time and/or build
> time, and thus the package would still install in the original stagin we presented
> it with (for target/, this is not an isue, because target/ is never looked at
> during configure or build, only at install time). Or a package installation
> process just assumes that the directory structure exists (bad, but eh...).
> --- 8< ---
> 
> Is that ok for you ?

Yes. The registration thing is only just a corner case, so needs not be
in the commit log.

> > > +define pkg_final_rsync_after
> > > +	cd $(1); \
> > > +	LC_ALL=C find . \( -type f -o -type l \) -printf '%T@:%i:%#m:%y:%s,%p\n' \
> > > +		| LC_ALL=C sort > $($(PKG)_DIR)/.files-final-rsync$(2).after
> > > +	LC_ALL=C comm -2 \
> > > +		$($(PKG)_DIR)/.files-final-rsync$(2).before \
> > > +		$($(PKG)_DIR)/.files-final-rsync$(2).after \
> > > +		| sed -r -e 's/^[^,]+,./- /' \
> > > +		> $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync
> > > +	rm -f $($(PKG)_DIR)/.files-final-rsync$(2).after  
> > 
> > You forgot to remove .files-final-rsync$(2).before
> 
> I cannot remove it in this patch.
> 
> .files-final-rsync$(2).before is generated by .stamp_configured rule calling
> pkg_final_rsync_before call and if we run 'make && make foo-rebuild' the file
> generated by first make invocation will be no more present for second make
> invocation.

Ah, yes, I see now. Thanks.

> We can remove .files-final-rsync$(2).before only after patch
> "package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure,rebuild,reinstall}"
> Indeed, in this patch, pkg_final_rsync_after is called after the per-package
> host and target dir are recreated from scratch.
> 
> I will remove .files-final-rsync$(2).before in a dedicated patch after
> "package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure,rebuild,reinstall}"
> 
> Is that ok for you ?

Yes, it is. ?

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] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Herve Codina]

On a per-package build, rsync final {TARGET,HOST}_DIR using
exclusion file list computed for each packages.
As we rsync only files generated by each packages, we need to
to do this rsync recursively on each dependencies to collect
all needed files. This is done based on existing
<PKG>_FINAL_RECURSIVE_DEPENDENCIES

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 Makefile | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index 472af5a318..50bc6b4503 100644
--- a/Makefile
+++ b/Makefile
@@ -740,10 +740,28 @@ TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt))
 HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt))
 STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt))
 
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+# rsync the contents of per-package directories
+# $1: space-separated list of packages to rsync from
+# $2: 'host' or 'target'
+# $3: destination directory
+# $4: exclude file list to use
+define per-package-rsync-delta
+	mkdir -p $(3)
+	$(foreach pkg,$(1),\
+		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
+		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
+		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
+		$(3)$(sep))
+endef
+endif
+
 .PHONY: host-finalize
 host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
 	@$(call MESSAGE,"Finalizing host directory")
-	$(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR))
+	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
+		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
+		host,$(HOST_DIR),.files-final-rsync-host.exclude_rsync)
 
 .PHONY: staging-finalize
 staging-finalize: $(STAGING_DIR_SYMLINK)
@@ -751,7 +769,9 @@ staging-finalize: $(STAGING_DIR_SYMLINK)
 .PHONY: target-finalize
 target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize
 	@$(call MESSAGE,"Finalizing target directory")
-	$(call per-package-rsync,$(sort $(PACKAGES)),target,$(TARGET_DIR))
+	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
+		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
+		target,$(TARGET_DIR),.files-final-rsync.exclude_rsync)
 	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
 	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
 		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
-- 
2.31.1

[Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Yann E. MORIN]

Herv?, All,

Nitpick, applicable to most of your patches in this series: there should
not be an uppercase in the title, after the colon sign, and the tense
should be imperative, e.g:

    Makefile: rsync global {TARGET,HOST}_DIR using exclusion file list
    Makefile: break hardlinks in global {TARGET,HOST}_DIR on per-package build
    package/pkg-generic.mk: fix per-package <pkg>-{reconfigure, rebuild, reinstall} 

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> On a per-package build, rsync final {TARGET,HOST}_DIR using
> exclusion file list computed for each packages.
> As we rsync only files generated by each packages, we need to
> to do this rsync recursively on each dependencies to collect
> all needed files. This is done based on existing
> <PKG>_FINAL_RECURSIVE_DEPENDENCIES

I am not sure I understand why this is needed...

One thing that comes to mind is speedup. Indeed, now that we can ensure
that packages only install new files and don;t change existing files, we
can speedup the final aggregation by just handling the new files.

But since we are using hardlinks, the aggregation is rather fast...

Can you expand on the rationale in the commit log when you respin,
please?

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  Makefile | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 472af5a318..50bc6b4503 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -740,10 +740,28 @@ TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt))
>  HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt))
>  STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt))
>  
> +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> +# rsync the contents of per-package directories
> +# $1: space-separated list of packages to rsync from
> +# $2: 'host' or 'target'
> +# $3: destination directory
> +# $4: exclude file list to use
> +define per-package-rsync-delta
> +	mkdir -p $(3)

    $(Q)mkdir -p $(3)

> +	$(foreach pkg,$(1),\
> +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> +		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
> +		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> +		$(3)$(sep))

I would read it more easily if the lines after rsync are indented
furhter, to represent the fact that they are stilll rsync arguments.
Also, proably it would be nice to have this quiet by default. Indeed,
with rather-deep dependency chains, there can be quite a lot of rsync
lines...

    $(foreach pkg,$(1),\
        $(Q)rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
            --filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
            $(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
            $(3)$(sep))

Furthermore, this rsync filter is non-obvious. It would be nice to
explain the commit log that the list is already a proper filer, not just
a list.

Additionally, in the commit that generates that list, the commit log
should also mention that a list of filters is created, not a list of
filenames.

> +endef
> +endif
> +
>  .PHONY: host-finalize
>  host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
>  	@$(call MESSAGE,"Finalizing host directory")
> -	$(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR))
> +	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
> +		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
> +		host,$(HOST_DIR),.files-final-rsync-host.exclude_rsync)

This is unparseable by a human... :-(

    $(call per-package-rsync-delta, \
        $(sort $(foreach pkg, $(PACKAGES), \
                    $(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES)) \
        ), \
        host, \
        $(HOST_DIR), \
        .files-final-rsync-host.exclude_rsync \
    )

>  .PHONY: staging-finalize
>  staging-finalize: $(STAGING_DIR_SYMLINK)
> @@ -751,7 +769,9 @@ staging-finalize: $(STAGING_DIR_SYMLINK)
>  .PHONY: target-finalize
>  target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize
>  	@$(call MESSAGE,"Finalizing target directory")
> -	$(call per-package-rsync,$(sort $(PACKAGES)),target,$(TARGET_DIR))
> +	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
> +		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
> +		target,$(TARGET_DIR),.files-final-rsync.exclude_rsync)

Ditto.

But now that I review this patch, I notice that there is still a hole in
our file-overwrite detection. It only handles files modified by a
package, that comes from it dependencies.

However, if two packages in two different depednency chains, install the
same file, then we're still toast. For example:

    A --> B --> C
      `-> D

If C and D install the same file, then we don't detect the conflict, and
we can't easil  predict which the final file will come from...

Of course, such a detection will have to be *in addition* to what this
series already does, so this new issue does not invalidate this series.

Regards,
Yann E. MORIN.

>  	$(foreach hook,$(TARGET_FINALIZE_HOOKS),$($(hook))$(sep))
>  	rm -rf $(TARGET_DIR)/usr/include $(TARGET_DIR)/usr/share/aclocal \
>  		$(TARGET_DIR)/usr/lib/pkgconfig $(TARGET_DIR)/usr/share/pkgconfig \
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Yann E. MORIN]

Herv?, All,

Additional revview I forgot previously...

On 2021-06-24 22:20 +0200, Yann E. MORIN spake thusly:
> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > On a per-package build, rsync final {TARGET,HOST}_DIR using
> > exclusion file list computed for each packages.
> > As we rsync only files generated by each packages, we need to
> > to do this rsync recursively on each dependencies to collect
> > all needed files. This is done based on existing
> > <PKG>_FINAL_RECURSIVE_DEPENDENCIES
> I am not sure I understand why this is needed...
> 
> One thing that comes to mind is speedup. Indeed, now that we can ensure
> that packages only install new files and don;t change existing files, we
> can speedup the final aggregation by just handling the new files.
> 
> But since we are using hardlinks, the aggregation is rather fast...
> 
> Can you expand on the rationale in the commit log when you respin,
> please?

Now I have seen the next patch, it makes sense. Yet, it should be
explained here nonetheless. ;-)

[--SNIP--]
> > +	$(foreach pkg,$(1),\
> > +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > +		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
> > +		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > +		$(3)$(sep))
[--SNIP--]
> Furthermore, this rsync filter is non-obvious. It would be nice to
> explain the commit log that the list is already a proper filer, not just
> a list.
> Additionally, in the commit that generates that list, the commit log
> should also mention that a list of filters is created, not a list of
> filenames.

And to begin with, why are we generating an _exclude_ list, rather than
an _include_ list?

I.e. rather than rsync everything except what we don't want, why not
just rsync just what we want, and generate a list of filters about
exactly just the files installed by a package?

I.e. in previous patch, crea create '+' filters rather than '-' filters.
Also, I think using the full-length filter 'include' is nicer than the
shorter '+'.

Probably something like:

    LC_ALL=C comm -1 \
        $($(PKG)_DIR)/.files-final-rsync$(2).before \
        $($(PKG)_DIR)/.files-final-rsync$(2).after \
        | sed -r -e 's/^[^,]+,./include /' \
        > $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync

(Totally untested, slippery code, excersise caution.)

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] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Herve Codina]

Hi,

On Thu, 24 Jun 2021 22:34:10 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Herv?, All,
> 
> Additional revview I forgot previously...
> 
> On 2021-06-24 22:20 +0200, Yann E. MORIN spake thusly:
> > On 2021-06-21 16:11 +0200, Herve Codina spake thusly:  
> > > On a per-package build, rsync final {TARGET,HOST}_DIR using
> > > exclusion file list computed for each packages.
> > > As we rsync only files generated by each packages, we need to
> > > to do this rsync recursively on each dependencies to collect
> > > all needed files. This is done based on existing
> > > <PKG>_FINAL_RECURSIVE_DEPENDENCIES  
> > I am not sure I understand why this is needed...
> > 
> > One thing that comes to mind is speedup. Indeed, now that we can ensure
> > that packages only install new files and don;t change existing files, we
> > can speedup the final aggregation by just handling the new files.
> > 
> > But since we are using hardlinks, the aggregation is rather fast...
> > 
> > Can you expand on the rationale in the commit log when you respin,
> > please?  
> 
> Now I have seen the next patch, it makes sense. Yet, it should be
> explained here nonetheless. ;-)

The next patch just breaks the hardlinks had nothing to do with
the use of <PKG>_FINAL_RECURSIVE_DEPENDENCIES here.

Previously files were collected using a simple rsync and iterating on
$(PACKAGES). The rsync collected all files from current package host or target
so it collected files generated by the current package and the packages it
depends on.

Now, we collect only files generated by the current package and we discard
files generated by the packages it depends on.
Iterating on $(PACKAGES) is no longer enough.

Indeed $(PACKAGES) is the list of packages that are enabled in .config file (ie
selected during 'make menuconfig' or defconfig file)
But some packages (sure for host packages) can have some "hidden" dependencies
by hidden I means not seen or selected by config files.
This "hidden" dependencies are created at Makefile level.

HOST_FOO_DEPENDENCIES = host-bar

We need to collect files from this kind on dependency to generate the final
HOST_DIR.

<PKG>_FINAL_RECURSIVE_DEPENDENCIES contains all makefile level dependencies
a given package depends on.

Finally, we do:
for pkg selected in .config ($(PACKAGES))
        # Add current pkg and packages it depends on at makefile level
        FULL_PACKAGES_AND_DEPENDS += pkg + $(<PKG>_FINAL_RECURSIVE_DEPENDENCIES)
And then we call sort $(FULL_PACKAGES_AND_DEPENDS) to remove duplicates


I can add in the commit log message:
---- 8< ----
Using only packages present in $(PACKAGES) is no longer enough.
Indeed, we collect only files generated for a given package
and $(PACKAGES) contains the list of packages selected in .config file
Some dependencies exists at Makefile level that are no present@.config
file level (at least for host packages).
HOST_FOO_DEPENDENCIES += host-bar
To take into account these dependencies, <PKG>_FINAL_RECURSIVE_DEPENDENCIES
is used in addition to $(PACKAGES).
---- 8< ----

Is that ok for you ?

> 
> [--SNIP--]
> > > +	$(foreach pkg,$(1),\
> > > +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > > +		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
> > > +		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > > +		$(3)$(sep))  
> [--SNIP--]
> > Furthermore, this rsync filter is non-obvious. It would be nice to
> > explain the commit log that the list is already a proper filer, not just
> > a list.
> > Additionally, in the commit that generates that list, the commit log
> > should also mention that a list of filters is created, not a list of
> > filenames.  
> 
> And to begin with, why are we generating an _exclude_ list, rather than
> an _include_ list?
> 
> I.e. rather than rsync everything except what we don't want, why not
> just rsync just what we want, and generate a list of filters about
> exactly just the files installed by a package?
> 
> I.e. in previous patch, crea create '+' filters rather than '-' filters.
> Also, I think using the full-length filter 'include' is nicer than the
> shorter '+'.
> 
> Probably something like:
> 
>     LC_ALL=C comm -1 \
>         $($(PKG)_DIR)/.files-final-rsync$(2).before \
>         $($(PKG)_DIR)/.files-final-rsync$(2).after \
>         | sed -r -e 's/^[^,]+,./include /' \
>         > $($(PKG)_DIR)/.files-final-rsync$(2).exclude_rsync  
> 
> (Totally untested, slippery code, excersise caution.)
> 

Inclusion filter is not so simple ...

In rsync man, we can read:
---- 8< ----
Note  that,  when  using the --recursive (-r) option (which is implied by -a),
every subdir component of every path is visited left to right, with each
directory having a chance for exclusion before its content.  In this way
include/exclude patterns are applied recursively to the pathname of each node in
the filesystem's tree (those  inside  the  transfer).
The exclude patterns short-circuit the directory traversal
stage as rsync finds the files to send.

For  instance,  to include "/foo/bar/baz", the directories "/foo" and
"/foo/bar" must not be excluded.  Excluding one of those parent directories
prevents the examination of its content, cutting off rsync's recursion into
those paths and rendering the include for "/foo/bar/baz" ineffectual (since rsync
can't match something it never sees in the cut-off section of the directory
hierarchy).

The concept path exclusion is particularly important when using a trailing '*' rule.
For instance, this won't work:

           + /some/path/this-file-will-not-be-found
           + /file-is-included
           - *

This  fails because the parent directory "some" is excluded by the '*' rule, so
rsync never visits any of the files in the "some" or "some/path" directories.
One solution is to ask for all directories in the hierarchy to be included by
using a single rule: "+ */" (put it somewhere before the "- *" rule), and perhaps
use the  --prune-empty-dirs  option.
Another solution is to add specific include rules for all the parent dirs that
need to be visited.  For instance, this set of rules works fine:

           + /some/
           + /some/path/
           + /some/path/this-file-is-found
           + /file-also-included
           - *

---- 8< ----


I feel tricky and error prone to use include filter and really simpler to take the
problem on the other side.
Instead of including what we want, exclude what we don't want.

The filter file passed to rsync is as simple as:
  - /foo/bar/not_thisone
  - /foo/not_thisone_too
  - /not_thisone

With this filter file, rsync will include all files that are not mentioned without
any more needs.


Regards,
Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Yann E. MORIN]

Herv?, All,

On 2021-06-25 13:59 +0200, Herve Codina spake thusly:
> On Thu, 24 Jun 2021 22:34:10 +0200
> "Yann E. MORIN" <yann.morin.1998@free.fr> wrote:
> > On 2021-06-24 22:20 +0200, Yann E. MORIN spake thusly:
> > > On 2021-06-21 16:11 +0200, Herve Codina spake thusly:  
> > > > On a per-package build, rsync final {TARGET,HOST}_DIR using
> > > > exclusion file list computed for each packages.
> > > > As we rsync only files generated by each packages, we need to
> > > > to do this rsync recursively on each dependencies to collect
> > > > all needed files. This is done based on existing
> > > > <PKG>_FINAL_RECURSIVE_DEPENDENCIES  
> > > I am not sure I understand why this is needed...
> > > 
> > > One thing that comes to mind is speedup. Indeed, now that we can ensure
> > > that packages only install new files and don;t change existing files, we
> > > can speedup the final aggregation by just handling the new files.
> > > 
> > > But since we are using hardlinks, the aggregation is rather fast...
> > > 
> > > Can you expand on the rationale in the commit log when you respin,
> > > please?  
> > 
> > Now I have seen the next patch, it makes sense. Yet, it should be
> > explained here nonetheless. ;-)
> 
> The next patch just breaks the hardlinks had nothing to do with
> the use of <PKG>_FINAL_RECURSIVE_DEPENDENCIES here.

Oh, I was not commenting about the _FINAL_RECURSIVE_DEPENDENCIES, but
about the need for an exclusion list to begin with. I totally agree
witht the rationale for using _FINAL_RECURSIVE_DEPENDENCIES.

[--SNIP--]
> > And to begin with, why are we generating an _exclude_ list, rather than
> > an _include_ list?
[--SNIP--]
> > (Totally untested, slippery code, excersise caution.)
> Inclusion filter is not so simple ...
> 
> In rsync man, we can read:
[--SNIP--]

Indeed, I see now. The rsync filter is not about listing files; it is
deciding what should be or should not be transferred. Anything that is
not filtered-out is transferred.

So OK, using an exclude list toally makes sense.

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] [PATCH 13/15] Makefile: Rsync global {TARGET, HOST}_DIR using exclusion file list

[Herve Codina]

On Thu, 24 Jun 2021 22:20:10 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Herv?, All,
> 
> Nitpick, applicable to most of your patches in this series: there should
> not be an uppercase in the title, after the colon sign, and the tense
> should be imperative, e.g:
> 

Ok, thanks.

> 
> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > On a per-package build, rsync final {TARGET,HOST}_DIR using
> > exclusion file list computed for each packages.
> > As we rsync only files generated by each packages, we need to
> > to do this rsync recursively on each dependencies to collect
> > all needed files. This is done based on existing
> > <PKG>_FINAL_RECURSIVE_DEPENDENCIES  
> 
> I am not sure I understand why this is needed...
> 
> One thing that comes to mind is speedup. Indeed, now that we can ensure
> that packages only install new files and don;t change existing files, we
> can speedup the final aggregation by just handling the new files.
> 
> But since we are using hardlinks, the aggregation is rather fast...
> 
> Can you expand on the rationale in the commit log when you respin,
> please?
> 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  Makefile | 24 ++++++++++++++++++++++--
> >  1 file changed, 22 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index 472af5a318..50bc6b4503 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -740,10 +740,28 @@ TARGET_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list.txt))
> >  HOST_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-host.txt))
> >  STAGING_DIR_FILES_LISTS = $(sort $(wildcard $(BUILD_DIR)/*/.files-list-staging.txt))
> >  
> > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> > +# rsync the contents of per-package directories
> > +# $1: space-separated list of packages to rsync from
> > +# $2: 'host' or 'target'
> > +# $3: destination directory
> > +# $4: exclude file list to use
> > +define per-package-rsync-delta
> > +	mkdir -p $(3)  
> 
>     $(Q)mkdir -p $(3)

Changed.

> 
> > +	$(foreach pkg,$(1),\
> > +		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > +		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
> > +		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> > +		$(3)$(sep))  
> 
> I would read it more easily if the lines after rsync are indented
> furhter, to represent the fact that they are stilll rsync arguments.
> Also, proably it would be nice to have this quiet by default. Indeed,
> with rather-deep dependency chains, there can be quite a lot of rsync
> lines...
> 
>     $(foreach pkg,$(1),\
>         $(Q)rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
>             --filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
>             $(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
>             $(3)$(sep))

Changed

> 
> Furthermore, this rsync filter is non-obvious. It would be nice to
> explain the commit log that the list is already a proper filer, not just
> a list.
> 
> Additionally, in the commit that generates that list, the commit log
> should also mention that a list of filters is created, not a list of
> filenames.
> 
> > +endef
> > +endif
> > +
> >  .PHONY: host-finalize
> >  host-finalize: $(PACKAGES) $(HOST_DIR) $(HOST_DIR_SYMLINK)
> >  	@$(call MESSAGE,"Finalizing host directory")
> > -	$(call per-package-rsync,$(sort $(PACKAGES)),host,$(HOST_DIR))
> > +	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
> > +		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
> > +		host,$(HOST_DIR),.files-final-rsync-host.exclude_rsync)  
> 
> This is unparseable by a human... :-(
> 
>     $(call per-package-rsync-delta, \
>         $(sort $(foreach pkg, $(PACKAGES), \
>                     $(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES)) \
>         ), \
>         host, \
>         $(HOST_DIR), \
>         .files-final-rsync-host.exclude_rsync \
>     )

Changed.

> 
> >  .PHONY: staging-finalize
> >  staging-finalize: $(STAGING_DIR_SYMLINK)
> > @@ -751,7 +769,9 @@ staging-finalize: $(STAGING_DIR_SYMLINK)
> >  .PHONY: target-finalize
> >  target-finalize: $(PACKAGES) $(TARGET_DIR) host-finalize
> >  	@$(call MESSAGE,"Finalizing target directory")
> > -	$(call per-package-rsync,$(sort $(PACKAGES)),target,$(TARGET_DIR))
> > +	$(call per-package-rsync-delta, $(sort $(foreach pkg, $(PACKAGES), \
> > +		$(pkg) $($(call UPPERCASE,$(pkg))_FINAL_RECURSIVE_DEPENDENCIES))), \
> > +		target,$(TARGET_DIR),.files-final-rsync.exclude_rsync)  
> 
> Ditto.

Changed

> 
> But now that I review this patch, I notice that there is still a hole in
> our file-overwrite detection. It only handles files modified by a
> package, that comes from it dependencies.
> 
> However, if two packages in two different depednency chains, install the
> same file, then we're still toast. For example:
> 
>     A --> B --> C
>       `-> D
> 
> If C and D install the same file, then we don't detect the conflict, and
> we can't easil  predict which the final file will come from...
> 
> Of course, such a detection will have to be *in addition* to what this
> series already does, so this new issue does not invalidate this series.
> 

I missed this use case.

You you think it can help to keep in mind if
  (1) I add it in my v2 my cover letter
or
  (2) I add it in this patch commit log


Herv?


-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 14/15] Makefile: Breaks hardlinks in global {TARGET, HOST}_DIR on per-package build

[Herve Codina]

Without this patch, a make <pkg>_rebuild detects overwrites. Indeed, in
target_finalize steps some modifications are done on installed files (ie
strip or TARGET_FINALIZE_HOOKS for instance).

In order to avoid these modifications seen from per-package {TARGET,HOST}_DIR
and so been analyzed as some overwrites, global {TARGET,HOST}_DIR is built
using a full copy of the involved per-package files instead of hardlinks.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 50bc6b4503..fe6e3f95af 100644
--- a/Makefile
+++ b/Makefile
@@ -749,7 +749,7 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
 define per-package-rsync-delta
 	mkdir -p $(3)
 	$(foreach pkg,$(1),\
-		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
+		rsync -a \
 		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
 		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
 		$(3)$(sep))
-- 
2.31.1

[Buildroot] [PATCH 14/15] Makefile: Breaks hardlinks in global {TARGET, HOST}_DIR on per-package build

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> Without this patch, a make <pkg>_rebuild detects overwrites. Indeed, in
> target_finalize steps some modifications are done on installed files (ie
> strip or TARGET_FINALIZE_HOOKS for instance).
> 
> In order to avoid these modifications seen from per-package {TARGET,HOST}_DIR
> and so been analyzed as some overwrites, global {TARGET,HOST}_DIR is built
> using a full copy of the involved per-package files instead of hardlinks.

OK, now it makes sense to do a partial rsync with just the needed files.

When you respin:

    Reviewed-by: Yann E. MORIN <yann.morin.1998@free.fr>

Regards,
Yann E. MORIN.

> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  Makefile | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 50bc6b4503..fe6e3f95af 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -749,7 +749,7 @@ ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
>  define per-package-rsync-delta
>  	mkdir -p $(3)
>  	$(foreach pkg,$(1),\
> -		rsync -a --link-dest=$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
> +		rsync -a \
>  		--filter='merge $($(call UPPERCASE,$(pkg))_DIR)/$(strip $(4))' \
>  		$(PER_PACKAGE_DIR)/$(pkg)/$(strip $(2))/ \
>  		$(3)$(sep))
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 15/15] package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure, rebuild, reinstall}

[Herve Codina]

Many overwrites are detected on <pkg>-{reconfigure,rebuild,reinstall}.
Indeed, files previously installed by a package were detected as
overwritten on next reconfigure, rebuild or reinstall.

To avoid this, we recreate per-package host and target dir from
scratch as it was done during the first configure step.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 package/pkg-generic.mk | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 546370af7a..0aafd865ad 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -470,6 +470,13 @@ define pkg-graph-depends
 		$$(GRAPHS_DIR)/$$(@).dot
 endef
 
+ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
+define empty-per-package-directory
+	rm -rf $(HOST_DIR) $(TARGET_DIR)
+	mkdir -p $(HOST_DIR) $(TARGET_DIR)
+endef
+endif
+
 ################################################################################
 # inner-generic-package -- generates the make targets needed to build a
 # generic package
@@ -1072,6 +1079,25 @@ endif
 			rm -f $$($(2)_TARGET_INSTALL_TARGET)
 			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
 			rm -f $$($(2)_TARGET_INSTALL_HOST)
+			$$(call empty-per-package-directory)
+			$$(call prepare-per-package-directory,$$($(2)_FINAL_DOWNLOAD_DEPENDENCIES))
+			$$(call prepare-per-package-directory,$$($(2)_FINAL_EXTRACT_DEPENDENCIES))
+			$$(call prepare-per-package-directory,$$($(2)_FINAL_DEPENDENCIES))
+			@$$(call pkg_final_rsync_before,$$(TARGET_DIR))
+			@$$(call pkg_final_rsync_before,$$(HOST_DIR),-host)
+			@$$(call pkg_size_before,$$(TARGET_DIR))
+			@$$(call pkg_size_before,$$(STAGING_DIR),-staging)
+			@$$(call pkg_size_before,$$(HOST_DIR),-host)
+			$$(call fixup-libtool-files,$$(NAME),$$(HOST_DIR))
+			$$(call fixup-libtool-files,$$(NAME),$$(STAGING_DIR))
+			$$(call fixup-python-files,$$(NAME),$$(HOST_DIR))
+			$$(call fixup-python-files,$$(NAME),$$(STAGING_DIR))
+			$$(foreach hook,$$($(2)_PER_PACKAGE_TWEAK_HOOKS),$$(call $$(hook))$$(sep))
+			@$$(call pkg_detect_overwrite_before,$$(TARGET_DIR))
+			@$$(call pkg_detect_overwrite_before,$$(HOST_DIR),-host)
+
+$(1)-clean-for-reinstall: PKG=$(2)
+$(1)-clean-for-reinstall: NAME=$(1)
 
 $(1)-reinstall:		$(1)-clean-for-reinstall $(1)
 
-- 
2.31.1

[Buildroot] [PATCH 15/15] package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure, rebuild, reinstall}

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> Many overwrites are detected on <pkg>-{reconfigure,rebuild,reinstall}.
> Indeed, files previously installed by a package were detected as
> overwritten on next reconfigure, rebuild or reinstall.
> 
> To avoid this, we recreate per-package host and target dir from
> scratch as it was done during the first configure step.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  package/pkg-generic.mk | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 546370af7a..0aafd865ad 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -470,6 +470,13 @@ define pkg-graph-depends
>  		$$(GRAPHS_DIR)/$$(@).dot
>  endef
>  
> +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> +define empty-per-package-directory
> +	rm -rf $(HOST_DIR) $(TARGET_DIR)
> +	mkdir -p $(HOST_DIR) $(TARGET_DIR)
> +endef
> +endif
> +
>  ################################################################################
>  # inner-generic-package -- generates the make targets needed to build a
>  # generic package
> @@ -1072,6 +1079,25 @@ endif
>  			rm -f $$($(2)_TARGET_INSTALL_TARGET)
>  			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
>  			rm -f $$($(2)_TARGET_INSTALL_HOST)
> +			$$(call empty-per-package-directory)
> +			$$(call prepare-per-package-directory,$$($(2)_FINAL_DOWNLOAD_DEPENDENCIES))
> +			$$(call prepare-per-package-directory,$$($(2)_FINAL_EXTRACT_DEPENDENCIES))
> +			$$(call prepare-per-package-directory,$$($(2)_FINAL_DEPENDENCIES))
> +			@$$(call pkg_final_rsync_before,$$(TARGET_DIR))
> +			@$$(call pkg_final_rsync_before,$$(HOST_DIR),-host)
> +			@$$(call pkg_size_before,$$(TARGET_DIR))
> +			@$$(call pkg_size_before,$$(STAGING_DIR),-staging)
> +			@$$(call pkg_size_before,$$(HOST_DIR),-host)
> +			$$(call fixup-libtool-files,$$(NAME),$$(HOST_DIR))
> +			$$(call fixup-libtool-files,$$(NAME),$$(STAGING_DIR))
> +			$$(call fixup-python-files,$$(NAME),$$(HOST_DIR))
> +			$$(call fixup-python-files,$$(NAME),$$(STAGING_DIR))
> +			$$(foreach hook,$$($(2)_PER_PACKAGE_TWEAK_HOOKS),$$(call $$(hook))$$(sep))
> +			@$$(call pkg_detect_overwrite_before,$$(TARGET_DIR))
> +			@$$(call pkg_detect_overwrite_before,$$(HOST_DIR),-host)

I don;t like this duplication, because it will inevitably diverge. And
already, it is different: rather than rely on $(PKG)_FINAL_DEPENDENCIES)
and a single call to prepare-per-package-directory, there are now three
calls to prepare-per-package-directory with three types of depenencies.

Why can't we comonalise this into a big macro that gets expanded twise,
once in the original $(BUILD_DIR)/%/.stamp_configured rule, and once
here?

This would also termendously help into factorising this away and into
its own $(1)-prepare step, eventually.

> +$(1)-clean-for-reinstall: PKG=$(2)
> +$(1)-clean-for-reinstall: NAME=$(1)

Please move those two right before the actual rule (i.e. just before
line 991 in mastr as of today.

Regards,
Yann E. MORIN.

>  $(1)-reinstall:		$(1)-clean-for-reinstall $(1)
>  
> -- 
> 2.31.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  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] [PATCH 15/15] package/pkg-generic.mk: Fix per-package <pkg>-{reconfigure, rebuild, reinstall}

[Herve Codina]

On Thu, 24 Jun 2021 22:44:42 +0200
"Yann E. MORIN" <yann.morin.1998@free.fr> wrote:

> Herv?, All,
> 
> On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> > Many overwrites are detected on <pkg>-{reconfigure,rebuild,reinstall}.
> > Indeed, files previously installed by a package were detected as
> > overwritten on next reconfigure, rebuild or reinstall.
> > 
> > To avoid this, we recreate per-package host and target dir from
> > scratch as it was done during the first configure step.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  package/pkg-generic.mk | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> > index 546370af7a..0aafd865ad 100644
> > --- a/package/pkg-generic.mk
> > +++ b/package/pkg-generic.mk
> > @@ -470,6 +470,13 @@ define pkg-graph-depends
> >  		$$(GRAPHS_DIR)/$$(@).dot
> >  endef
> >  
> > +ifeq ($(BR2_PER_PACKAGE_DIRECTORIES),y)
> > +define empty-per-package-directory
> > +	rm -rf $(HOST_DIR) $(TARGET_DIR)
> > +	mkdir -p $(HOST_DIR) $(TARGET_DIR)
> > +endef
> > +endif
> > +
> >  ################################################################################
> >  # inner-generic-package -- generates the make targets needed to build a
> >  # generic package
> > @@ -1072,6 +1079,25 @@ endif
> >  			rm -f $$($(2)_TARGET_INSTALL_TARGET)
> >  			rm -f $$($(2)_TARGET_INSTALL_IMAGES)
> >  			rm -f $$($(2)_TARGET_INSTALL_HOST)
> > +			$$(call empty-per-package-directory)
> > +			$$(call prepare-per-package-directory,$$($(2)_FINAL_DOWNLOAD_DEPENDENCIES))
> > +			$$(call prepare-per-package-directory,$$($(2)_FINAL_EXTRACT_DEPENDENCIES))
> > +			$$(call prepare-per-package-directory,$$($(2)_FINAL_DEPENDENCIES))
> > +			@$$(call pkg_final_rsync_before,$$(TARGET_DIR))
> > +			@$$(call pkg_final_rsync_before,$$(HOST_DIR),-host)
> > +			@$$(call pkg_size_before,$$(TARGET_DIR))
> > +			@$$(call pkg_size_before,$$(STAGING_DIR),-staging)
> > +			@$$(call pkg_size_before,$$(HOST_DIR),-host)
> > +			$$(call fixup-libtool-files,$$(NAME),$$(HOST_DIR))
> > +			$$(call fixup-libtool-files,$$(NAME),$$(STAGING_DIR))
> > +			$$(call fixup-python-files,$$(NAME),$$(HOST_DIR))
> > +			$$(call fixup-python-files,$$(NAME),$$(STAGING_DIR))
> > +			$$(foreach hook,$$($(2)_PER_PACKAGE_TWEAK_HOOKS),$$(call $$(hook))$$(sep))
> > +			@$$(call pkg_detect_overwrite_before,$$(TARGET_DIR))
> > +			@$$(call pkg_detect_overwrite_before,$$(HOST_DIR),-host)  
> 
> I don;t like this duplication, because it will inevitably diverge. And
> already, it is different: rather than rely on $(PKG)_FINAL_DEPENDENCIES)
> and a single call to prepare-per-package-directory, there are now three
> calls to prepare-per-package-directory with three types of depenencies.

The basic sequence (out of <pkg>_{reconfigure, rebuild, reinstall} is:
.stamp_downloaded
  -> call prepare-per-package-directory with <PKG>_FINAL_DOWNLOAD_DEPENDENCIES
.stamp_extracted
  -> call prepare-per-package-directory with <PKG>_FINAL_EXTRACT_DEPENDENCIES
.stamp_configured
  -> call prepare-per-package-directory with <PKG>_FINAL_FINAL_DEPENDENCIES

Here, I wanted to recreate host and target dir with all dependencies as they
were at previous .stamp_configure and so did the 3 calls.


> 
> Why can't we comonalise this into a big macro that gets expanded twise,
> once in the original $(BUILD_DIR)/%/.stamp_configured rule, and once
> here?

Excluding call to prepare-per-package-directory as they are not the same.

What name for this macro ? PREPARE_PRE_CONFIGURE ?

> 
> This would also termendously help into factorising this away and into
> its own $(1)-prepare step, eventually.
> 
> > +$(1)-clean-for-reinstall: PKG=$(2)
> > +$(1)-clean-for-reinstall: NAME=$(1)  
> 
> Please move those two right before the actual rule (i.e. just before
> line 991 in mastr as of today.

Done

> 
> Regards,
> Yann E. MORIN.
> 
> >  $(1)-reinstall:		$(1)-clean-for-reinstall $(1)
> >  
> > -- 
> > 2.31.1
> > 
> > _______________________________________________
> > buildroot mailing list
> > buildroot at busybox.net
> > http://lists.busybox.net/mailman/listinfo/buildroot  
> 



-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Arnout Vandecappelle]

 Hi Herve?,

 Great that someone is picking thuis up!

On 21/06/2021 16:11, Herve Codina wrote:
> Hi,
> 
> This series fixes some issues in the TLP build feature.
> The starting point was the work done by Thomas Petazzoni on overwrites
> detection (PATCH 1).

 I've marked [1] as Changes Requested.

 However, it seems the tests [2][3][4] are missing from this series. Could you
include them next time you post it?

> Based on this work, this series fixes some overwrites, introduces and uses
> <PKG>_PER_PACKAGE_TWEAK_HOOKS. This variable is used to collect specific
> per-package path tweaks needed by per-package build.
> With this tweaks collected in this variable, they can be re-applied
> when they are needed (<pkg>_{reconfigure,rebuild,reinstall}).

 That's excellent!


> It also reworks the final {HOST,TARGET}_DIR rsync.
> For each packages only files generated by the package are rsynced (PATCH 13).
> This is done using an exclusion list computed in (PATCH 12).
> In order to avoid modifications performed in target-finalize step to be seen
> as overwrites on next build, the final {HOST,TARGET}_DIR do not contain any
> hardlink anymore and are a full copy of expected files and directories (PATCH 14).

 Hm, that will have quite the impact on disk size, no? In non-PPS builds,
HOST_DIR is typically about 1/3 of the total size (the other 2/3 is BUILD_DIR,
the rest is negligible). By doing a non-hardlinked copy, the size of HOST_DIR is
basically going to double, right?

 I still think it's acceptable, but it's something to take into account...


> The last patch (PATCH 15) fixes <pkg>_{reconfigure,rebuild,reinstall} recreating
> per-package {HOST,TARGET}_DIR from scratch.
> Indeed, for a given package, re-generating the same file is detected as an overwrite.
> The simplest way to avoid this is to fully discard previous per-package {HOST,TARGET}_DIR
> directory and recreate them.

 +1 to that!


> To have a TLP build fully fonctionnal, several things are still missing:
>   - Be sure that all buildroot packages do not perform any overwrites.

 I guess that's easy: increase the chance of PER_PACKAGE enabled in
genrandconfig, and see what happens to the autobuilders. Or is there anything
else missing?

>   - Undo tweak stuff that have be done for per-package build (fixup-libtool-files,
>     fixup-python-files, <PKG>_PER_PACKAGE_TWEAK_HOOKS) after collecting files
>     in final {HOST,STAGING}_DIR.
>     This mainly means reverting .../per-package/xxxxx/... path to point to
>     the correct ones in final {HOST,STAGING}_DIR.
> 
> I hope this series will help in TLP build feature.


 Regards,
 Arnout

[1]
https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-9-thomas.petazzoni at bootlin.com/
[2]
https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-10-thomas.petazzoni at bootlin.com/
[3]
https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-11-thomas.petazzoni at bootlin.com/
[4]
https://patchwork.ozlabs.org/project/buildroot/patch/20200430095249.782597-12-thomas.petazzoni at bootlin.com/



> 
> Best regards,
> Herv? Codina
> 
> Herve Codina (14):
>   package/e2fsprogs: fix fsck overwrite in HOST_DIR
>   package/pkg-generic.mk: Remove Info documents dir entry
>   package/pkg-generic.mk: Fix .la files overwrite detection
>   package/pkg-generic.mk: Perform .la files fixup in per-package
>     HOST_DIR
>   package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS
>   package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS
>   package/apache: Move APACHE_FIXUP_APR_LIBTOOL to
>     <PKG>_PER_PACKAGE_TWEAK_HOOKS
>   package/pkg-python: Remove _sysconfigdata*.pyc files when
>     _sysconfigdata*.py are changed
>   package/pkg-generic.mk: Move python fixup to generic package
>     infrastructure
>   package/owfs: Remove Python sysconfigdata fixup
>   package/pkg-generic.mk: Generate final rsync exclude file list
>   Makefile: Rsync global {TARGET,HOST}_DIR using exclusion file list
>   Makefile: Breaks hardlinks in global {TARGET,HOST}_DIR on per-package
>     build
>   package/pkg-generic.mk: Fix per-package
>     <pkg>-{reconfigure,rebuild,reinstall}
> 
> Thomas Petazzoni (1):
>   package/pkg-generic.mk: detect files overwritten in TARGET_DIR and
>     HOST_DIR
> 
>  Makefile                       |  24 +++++++-
>  package/apache/apache.mk       |   2 +-
>  package/apr-util/apr-util.mk   |  12 ++--
>  package/e2fsprogs/e2fsprogs.mk |   1 +
>  package/owfs/owfs.mk           |   9 ---
>  package/pkg-generic.mk         | 103 ++++++++++++++++++++++++++++++++-
>  package/pkg-python.mk          |  10 ----
>  7 files changed, 134 insertions(+), 27 deletions(-)
> 

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Herve Codina]

Hi,

On Mon, 21 Jun 2021 22:42:18 +0200
Arnout Vandecappelle <arnout@mind.be> wrote:

>  However, it seems the tests [2][3][4] are missing from this series. Could you
> include them next time you post it?

Added in v2 series.


-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Yann E. MORIN]

Herv?, All,

On 2021-06-21 16:11 +0200, Herve Codina spake thusly:
> This series fixes some issues in the TLP build feature.
> The starting point was the work done by Thomas Petazzoni on overwrites
> detection (PATCH 1).
[--SNIP--]

Thanks for working on this complex topic. :-)

I'm now done with my review; I may have missed details of lesser
importance, so we'll deal with those later; Thomas also provided some
feedback. I see that you have already taken initial reviews into
account, and that you seem nicely on track for a v2.

As such, I've marked this entire series as "changes requested" in
patchwork.

Can you add me in Cc of the full series when you later respin, please?

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] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Andreas Naumann]

Hi Herve, all,

On 21.06.21 16:11, Herve Codina wrote:
> Hi,
> 
> This series fixes some issues in the TLP build feature.
> The starting point was the work done by Thomas Petazzoni on overwrites
> detection (PATCH 1).

after a long time I have recently picked up playing with TLP build 
again. As part of that I found a problem in qt5 where I sent a patch two 
days ago "qt5: Fix sporadic build failure during top-level parallel 
build" which is caused by manipulation of the hard-linked qt.conf in 
HOST_DIR from different qt5 packages.

Now I see your promising set, but the matter is quite complex and so I 
wonder if the "overwritten file detection" would a) uncover the qt.conf 
problem and b)if your patch series somehow fixes it in a generic way?


regards,
Andreas



> 
> Based on this work, this series fixes some overwrites, introduces and uses
> <PKG>_PER_PACKAGE_TWEAK_HOOKS. This variable is used to collect specific
> per-package path tweaks needed by per-package build.
> With this tweaks collected in this variable, they can be re-applied
> when they are needed (<pkg>_{reconfigure,rebuild,reinstall}).
> 
> It also reworks the final {HOST,TARGET}_DIR rsync.
> For each packages only files generated by the package are rsynced (PATCH 13).
> This is done using an exclusion list computed in (PATCH 12).
> In order to avoid modifications performed in target-finalize step to be seen
> as overwrites on next build, the final {HOST,TARGET}_DIR do not contain any
> hardlink anymore and are a full copy of expected files and directories (PATCH 14).
> 
> The last patch (PATCH 15) fixes <pkg>_{reconfigure,rebuild,reinstall} recreating
> per-package {HOST,TARGET}_DIR from scratch.
> Indeed, for a given package, re-generating the same file is detected as an overwrite.
> The simplest way to avoid this is to fully discard previous per-package {HOST,TARGET}_DIR
> directory and recreate them.
> 
> To have a TLP build fully fonctionnal, several things are still missing:
>    - Be sure that all buildroot packages do not perform any overwrites.
>    - Undo tweak stuff that have be done for per-package build (fixup-libtool-files,
>      fixup-python-files, <PKG>_PER_PACKAGE_TWEAK_HOOKS) after collecting files
>      in final {HOST,STAGING}_DIR.
>      This mainly means reverting .../per-package/xxxxx/... path to point to
>      the correct ones in final {HOST,STAGING}_DIR.
> 
> I hope this series will help in TLP build feature.
> 
> Best regards,
> Herv? Codina
> 
> Herve Codina (14):
>    package/e2fsprogs: fix fsck overwrite in HOST_DIR
>    package/pkg-generic.mk: Remove Info documents dir entry
>    package/pkg-generic.mk: Fix .la files overwrite detection
>    package/pkg-generic.mk: Perform .la files fixup in per-package
>      HOST_DIR
>    package/pkg-generic.mk: Introduce <PKG>_PER_PACKAGE_TWEAK_HOOKS
>    package/apr-util: Use <PKG>_PER_PACKAGE_TWEAK_HOOKS
>    package/apache: Move APACHE_FIXUP_APR_LIBTOOL to
>      <PKG>_PER_PACKAGE_TWEAK_HOOKS
>    package/pkg-python: Remove _sysconfigdata*.pyc files when
>      _sysconfigdata*.py are changed
>    package/pkg-generic.mk: Move python fixup to generic package
>      infrastructure
>    package/owfs: Remove Python sysconfigdata fixup
>    package/pkg-generic.mk: Generate final rsync exclude file list
>    Makefile: Rsync global {TARGET,HOST}_DIR using exclusion file list
>    Makefile: Breaks hardlinks in global {TARGET,HOST}_DIR on per-package
>      build
>    package/pkg-generic.mk: Fix per-package
>      <pkg>-{reconfigure,rebuild,reinstall}
> 
> Thomas Petazzoni (1):
>    package/pkg-generic.mk: detect files overwritten in TARGET_DIR and
>      HOST_DIR
> 
>   Makefile                       |  24 +++++++-
>   package/apache/apache.mk       |   2 +-
>   package/apr-util/apr-util.mk   |  12 ++--
>   package/e2fsprogs/e2fsprogs.mk |   1 +
>   package/owfs/owfs.mk           |   9 ---
>   package/pkg-generic.mk         | 103 ++++++++++++++++++++++++++++++++-
>   package/pkg-python.mk          |  10 ----
>   7 files changed, 134 insertions(+), 27 deletions(-)
> 

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Herve Codina]

Hi,

On Fri, 25 Jun 2021 11:08:43 +0200
Andreas Naumann <dev@andin.de> wrote:

> after a long time I have recently picked up playing with TLP build 
> again. As part of that I found a problem in qt5 where I sent a patch two 
> days ago "qt5: Fix sporadic build failure during top-level parallel 
> build" which is caused by manipulation of the hard-linked qt.conf in 
> HOST_DIR from different qt5 packages.
> 
> Now I see your promising set, but the matter is quite complex and so I 
> wonder if the "overwritten file detection" would a) uncover the qt.conf 
> problem and b)if your patch series somehow fixes it in a generic way?
> 
> 

I quickly looked at your patch and it looks like an overwrite issue.

Your patch:
---- 8< ----
 # compiled into the Qt library. We need it to make "qmake" relocatable and
 # tweak the per-package install pathes
 define QT5_INSTALL_QT_CONF
+	rm -f $(HOST_DIR)/bin/qt.conf
 	sed -e "s|@@HOST_DIR@@|$(HOST_DIR)|" -e "s|@@STAGING_DIR@@|$(STAGING_DIR)|" \
 		$(QT5BASE_PKGDIR)/qt.conf.in > $(HOST_DIR)/bin/qt.conf
 endef
---- 8< ----

This overwrite is done by sed whose output is redirected $(HOST_DIR)/bin/qt.conf

With TLP this overwite leads to package A seeing modification done by package B

Without your patch and with overwrite detection present in my series
the overwrite should be detected and the build stopped.

My series do not contains any fixes for this specific Qt5 overwrite.
This is a concrete example of what I said still missing ...

Your 'rm' breaks the hardlink and so the following sed does not perform
overwrite anymore.
That's the correct fix.

To go further with interactions with my current series still ongoing and not ready
to be merged, the entire macro (QT5_INSTALL_QT_CONF) should be moved to
<PKG>_PER_PACKAGE_TWEAK_HOOKS and QT5_QT_CONF_FIXUP is no more needed.

Herv?

-- 
Herv? Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[Buildroot] [PATCH 00/15] Overwritten file detection and fixes, one more step to TLP build

[Andreas Naumann]

Hi Herve,

On 25.06.21 15:13, Herve Codina wrote:
> Hi,
> 
> On Fri, 25 Jun 2021 11:08:43 +0200
> Andreas Naumann <dev@andin.de> wrote:
> 
>> after a long time I have recently picked up playing with TLP build
>> again. As part of that I found a problem in qt5 where I sent a patch two
>> days ago "qt5: Fix sporadic build failure during top-level parallel
>> build" which is caused by manipulation of the hard-linked qt.conf in
>> HOST_DIR from different qt5 packages.
>>
>> Now I see your promising set, but the matter is quite complex and so I
>> wonder if the "overwritten file detection" would a) uncover the qt.conf
>> problem and b)if your patch series somehow fixes it in a generic way?
>>
>>
> 
> I quickly looked at your patch and it looks like an overwrite issue.
> 
> Your patch:
> ---- 8< ----
>   # compiled into the Qt library. We need it to make "qmake" relocatable and
>   # tweak the per-package install pathes
>   define QT5_INSTALL_QT_CONF
> +	rm -f $(HOST_DIR)/bin/qt.conf
>   	sed -e "s|@@HOST_DIR@@|$(HOST_DIR)|" -e "s|@@STAGING_DIR@@|$(STAGING_DIR)|" \
>   		$(QT5BASE_PKGDIR)/qt.conf.in > $(HOST_DIR)/bin/qt.conf
>   endef
> ---- 8< ----
> 
> This overwrite is done by sed whose output is redirected $(HOST_DIR)/bin/qt.conf
> 
> With TLP this overwite leads to package A seeing modification done by package B
> 
> Without your patch and with overwrite detection present in my series
> the overwrite should be detected and the build stopped.
> 
> My series do not contains any fixes for this specific Qt5 overwrite.
> This is a concrete example of what I said still missing ...

Thank you for the confirmation.

> 
> Your 'rm' breaks the hardlink and so the following sed does not perform
> overwrite anymore.
> That's the correct fix.
> 
> To go further with interactions with my current series still ongoing and not ready
> to be merged, the entire macro (QT5_INSTALL_QT_CONF) should be moved to
> <PKG>_PER_PACKAGE_TWEAK_HOOKS and QT5_QT_CONF_FIXUP is no more needed.

Ok, I'll try to catch the point when your set is accepted and the repost 
an updated version.


have a nice weekend,
Andreas


> 
> Herv?
>