[v2,2/2] pkg-generic: Cleanup some redundant checks

With the last change, packages will depend
only on host-{xz,lzip} if the source archives
have filenames requesting the corresponding compressor.

This allows using a single guard.

Signed-off-by: Norbert Lange <nolange79@gmail.com>
---
 package/pkg-generic.mk | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

Norbert, All,

On 2019-03-17 22:20 +0100, Norbert Lange spake thusly:
> With the last change, packages will depend
> only on host-{xz,lzip} if the source archives
> have filenames requesting the corresponding compressor.
> 
> This allows using a single guard.

This commit log is an improvement agaisnt the previous one, yet I still
had to think a bit too hard to understand the reason that works.

And I think there is an issue with that. Not today, but that opens up a
case where we can introduce a subtil bug in the future.

If, say, xz changes its distribution archive from .bz2 to .lz, iand lzip
changes theirs from .gz to .xz, then we'd introduce a circular
dependency at the make level, and make silently and arbitrarily drops
one of the dependencies.

However, now that I think about it, that is of not big consequence: in
either case, the build woulld break on the first one we try to extract,
and we would notice quite early and quite easily.

Still, I'd like we think a bit harder about those special cases.

Regards,
Yann E. MORIN.

> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/pkg-generic.mk | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 89da43d5e5..11f16cab18 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -604,18 +604,15 @@ endif
>  
>  ifeq ($$(filter host-tar host-skeleton host-fakedate,$(1)),)
>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_TAR_HOST_DEPENDENCY)
> -endif
>  
> -ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate,$(1)),)
>  ifneq ($$(filter .xz .lzma,$$(suffix $$($(2)_SOURCE))),)
>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_XZCAT_HOST_DEPENDENCY)
>  endif
> -endif
>  
> -ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate,$(1)),)
>  ifeq ($$(suffix $$($(2)_SOURCE)),.lz)
>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_LZIP_HOST_DEPENDENCY)
>  endif
> +
>  endif
>  
>  ifeq ($$(BR2_CCACHE),y)
> -- 
> 2.20.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

Am Di., 19. März 2019 um 23:03 Uhr schrieb Yann E. MORIN
<yann.morin.1998@free.fr>:
>
> Norbert, All,
>
> On 2019-03-17 22:20 +0100, Norbert Lange spake thusly:
> > With the last change, packages will depend
> > only on host-{xz,lzip} if the source archives
> > have filenames requesting the corresponding compressor.
> >
> > This allows using a single guard.
>
> This commit log is an improvement agaisnt the previous one, yet I still
> had to think a bit too hard to understand the reason that works.
>
> And I think there is an issue with that. Not today, but that opens up a
> case where we can introduce a subtil bug in the future.
>
> If, say, xz changes its distribution archive from .bz2 to .lz, iand lzip
> changes theirs from .gz to .xz, then we'd introduce a circular
> dependency at the make level, and make silently and arbitrarily drops
> one of the dependencies.

What would you do in that case anyway?
(besides I dont think lzip is getting any traction, or that a sane compressor
package should use anything but gzip for "boostrapping").

> However, now that I think about it, that is of not big consequence: in
> either case, the build woulld break on the first one we try to extract,
> and we would notice quite early and quite easily.
>
> Still, I'd like we think a bit harder about those special cases.

I really don't think the patch makes things worse.

Norbert

Hello Norbert,

On Sun, 17 Mar 2019 22:20:14 +0100
Norbert Lange <nolange79@gmail.com> wrote:

> With the last change, packages will depend
> only on host-{xz,lzip} if the source archives
> have filenames requesting the corresponding compressor.
> 
> This allows using a single guard.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>
> ---
>  package/pkg-generic.mk | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)

I hesitated a bit on this. Originally, I was quite in favor of the
change and found Yann's opinion a bit too strict. However, after
applying the patch, and looking at the code, I ended up feeling a bit
like Yann: even though there are less lines of code, the conditions
become a bit tricky, while currently, they are perfectly clear.

So I'll mark this patch as Rejected. Of course, if other BR maintainers
object, we can revisit, my choice is not super strong.

Best regards,

Thomas

On 17/03/2019 22:20, Norbert Lange wrote:
> With the last change, packages will depend
> only on host-{xz,lzip} if the source archives
> have filenames requesting the corresponding compressor.
> 
> This allows using a single guard.
> 
> Signed-off-by: Norbert Lange <nolange79@gmail.com>

 I disagree with Yann and Thomas that this makes it less clear. Except, I don't
think you should have even the single guard...

 Regarding the potential circular dependency that Yann mentioned: yes, that's
true. But conversely, by adding the filter like we have now, you remove a
potentially required dependency. So in the end it's the same thing. And for
circular dependencies we have graph-depends that detects them.

> ---
>  package/pkg-generic.mk | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 89da43d5e5..11f16cab18 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -604,18 +604,15 @@ endif
>  
>  ifeq ($$(filter host-tar host-skeleton host-fakedate,$(1)),)

 This condition is needed because skeleton and fakedate are dependencies of tar,
but host-skeleton and host-fakedate don't actually need tar. However, since
there's nothing that detects that host-skeleton and host-fakedate don't have a
tarball to extract, we have to remove those dependencies by hand.

>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_TAR_HOST_DEPENDENCY)
> -endif
>  
> -ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate,$(1)),)
>  ifneq ($$(filter .xz .lzma,$$(suffix $$($(2)_SOURCE))),)

 Here, however, there is an explicit dependency. So the condition on tar,
skeleton, fakedate is not needed.

 Bottom line: I don't think the endif above should be removed.


 Regards,
 Arnout


>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_XZCAT_HOST_DEPENDENCY)
>  endif
> -endif
>  
> -ifeq ($$(filter host-tar host-skeleton host-xz host-lzip host-fakedate,$(1)),)
>  ifeq ($$(suffix $$($(2)_SOURCE)),.lz)
>  $(2)_EXTRACT_DEPENDENCIES += $$(BR2_LZIP_HOST_DEPENDENCY)
>  endif
> +
>  endif
>  
>  ifeq ($$(BR2_CCACHE),y)
>