All of lore.kernel.org
 help / color / mirror / Atom feed
* [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
@ 2022-05-11  6:23 yann.morin
  2022-05-11 17:15 ` Arnout Vandecappelle
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: yann.morin @ 2022-05-11  6:23 UTC (permalink / raw)
  To: buildroot; +Cc: Yann E. MORIN

From: "Yann E. MORIN" <yann.morin@orange.com>

Recent commit f0c7cb01a960 (package/pkg-download: do not try to vendor
_EXTRA_DOWNLOADS) got last-minute changes when applied, which changed
the expected behaviour for packages that do not have a main download.

Before f0c7cb01a960, the dl-wrapper would not even be called for those
packages, and the original patch that was sent also avoided downloading
such packages, but f0c7cb01a960 now causes the dl-wrapper to be called.

It is however an accident that the dl-wrapper does not fail. Indeed, it
is expected to fail if no download was successful; we pass no URI, so
the dl-wrapper should have failed, as it basically does:

    download_and_check=0
    for uri in "${uris[@]}"; do
        ...
    done
    if [ "${download_and_check}" -eq 0 ]; then
        exit 1
    fi

However, it does not even go that far...

Even though there is no output file, we still pass the path to the
package output directory as the output path. So, to avoid downloading
files already present, the wrapper checks if the output file exists,
and checks its hash:

    if [ -e "${output}" ]; then
        if support/download/check-hash ${quiet} "${hfile}" "${output}" ...
            exit 0
        ...
    fi

The output path does exist now, because we explicitly create it just
before calling the wrapper, because that's where we also locate the
lockfile.

So it ends up trying to validate the hash of a directory, but it fails
to, as there is indeed no hash file for that package. And a missing hash
file is just a warning, not an error, which makes the download actually
a success...

So, this is currently working, and this is by pure luck.

However, there is a potential issue: if a target package is a virtual
package, but the host package is a real package, e.g. the same foo.mk
does (or the other way around):

    HOST_FOO_VERSION = 1.2.3
    HOST_FOO_SITE = http://example.net/
    $(eval $(virtual-package))
    $(eval $(host-generic-package))

If there is a hash file to validate the host download, then the current
situation will cause a failure, because there would be a hash file, but
no hash for the output path of the target variant, which would then be
a hard-error.

So, revert to the behaviour from before f0c7cb01a960, where no download
is attempted for a package without a source (really, without a main
download, now).

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
---
 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 38219ce088..b233b07548 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -192,7 +192,7 @@ $(BUILD_DIR)/%/.stamp_downloaded:
 			break ; \
 		fi ; \
 	done
-	$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS)))
+	$(if $($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS))))
 	$(foreach p,$($(PKG)_ADDITIONAL_DOWNLOADS),$(call DOWNLOAD,$(p),$(PKG))$(sep))
 	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
 	$(Q)mkdir -p $(@D)
-- 
2.25.1


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-11  6:23 [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source yann.morin
@ 2022-05-11 17:15 ` Arnout Vandecappelle
  2022-05-11 20:04   ` yann.morin
  2022-05-14  9:47 ` Arnout Vandecappelle
  2022-05-28 19:34 ` Peter Korsgaard
  2 siblings, 1 reply; 7+ messages in thread
From: Arnout Vandecappelle @ 2022-05-11 17:15 UTC (permalink / raw)
  To: yann.morin, buildroot



On 11/05/2022 08:23, yann.morin@orange.com wrote:
> From: "Yann E. MORIN" <yann.morin@orange.com>
> 
> Recent commit f0c7cb01a960 (package/pkg-download: do not try to vendor
> _EXTRA_DOWNLOADS) got last-minute changes when applied, which changed
> the expected behaviour for packages that do not have a main download.
> 
> Before f0c7cb01a960, the dl-wrapper would not even be called for those
> packages, and the original patch that was sent also avoided downloading
> such packages, but f0c7cb01a960 now causes the dl-wrapper to be called.
> 
> It is however an accident that the dl-wrapper does not fail. Indeed, it
> is expected to fail if no download was successful; we pass no URI, so
> the dl-wrapper should have failed, as it basically does:
> 
>      download_and_check=0
>      for uri in "${uris[@]}"; do
>          ...
>      done
>      if [ "${download_and_check}" -eq 0 ]; then
>          exit 1
>      fi
> 
> However, it does not even go that far...
> 
> Even though there is no output file, we still pass the path to the
> package output directory as the output path. So, to avoid downloading
> files already present, the wrapper checks if the output file exists,
> and checks its hash:
> 
>      if [ -e "${output}" ]; then
>          if support/download/check-hash ${quiet} "${hfile}" "${output}" ...
>              exit 0
>          ...
>      fi
> 
> The output path does exist now, because we explicitly create it just
> before calling the wrapper, because that's where we also locate the
> lockfile.
> 
> So it ends up trying to validate the hash of a directory, but it fails
> to, as there is indeed no hash file for that package. And a missing hash
> file is just a warning, not an error, which makes the download actually
> a success...
> 
> So, this is currently working, and this is by pure luck.
> 
> However, there is a potential issue: if a target package is a virtual
> package, but the host package is a real package, e.g. the same foo.mk
> does (or the other way around):
> 
>      HOST_FOO_VERSION = 1.2.3
>      HOST_FOO_SITE = http://example.net/
>      $(eval $(virtual-package))
>      $(eval $(host-generic-package))
> 
> If there is a hash file to validate the host download, then the current
> situation will cause a failure, because there would be a hash file, but
> no hash for the output path of the target variant, which would then be
> a hard-error.
> 
> So, revert to the behaviour from before f0c7cb01a960, where no download
> is attempted for a package without a source (really, without a main
> download, now).
> 
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> ---
>   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 38219ce088..b233b07548 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -192,7 +192,7 @@ $(BUILD_DIR)/%/.stamp_downloaded:
>   			break ; \
>   		fi ; \
>   	done
> -	$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS)))
> +	$(if $($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS))))

  Perfection is the enemy of the good and all that, but a few lines below, we 
actually have the same issue for ACTUAL_SOURCE.

  So maybe instead the condition should be moved inside the DOWNLOAD macro, and 
it should check $(notdir $(1)).

  Regards,
  Arnout

>   	$(foreach p,$($(PKG)_ADDITIONAL_DOWNLOADS),$(call DOWNLOAD,$(p),$(PKG))$(sep))
>   	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
>   	$(Q)mkdir -p $(@D)
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-11 17:15 ` Arnout Vandecappelle
@ 2022-05-11 20:04   ` yann.morin
  2022-05-12 21:54     ` Arnout Vandecappelle
  0 siblings, 1 reply; 7+ messages in thread
From: yann.morin @ 2022-05-11 20:04 UTC (permalink / raw)
  To: Arnout Vandecappelle; +Cc: buildroot

Arnout, All,

On 2022-05-11 19:15 +0200, Arnout Vandecappelle spake thusly:
> On 11/05/2022 08:23, yann.morin@orange.com wrote:
> >From: "Yann E. MORIN" <yann.morin@orange.com>
[--SNIP--]
> >So, revert to the behaviour from before f0c7cb01a960, where no download
> >is attempted for a package without a source (really, without a main
> >download, now).
> >
> >Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> >Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
> >---
> >  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 38219ce088..b233b07548 100644
> >--- a/package/pkg-generic.mk
> >+++ b/package/pkg-generic.mk
> >@@ -192,7 +192,7 @@ $(BUILD_DIR)/%/.stamp_downloaded:
> >  			break ; \
> >  		fi ; \
> >  	done
> >-	$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS)))
> >+	$(if $($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS))))
> 
>  Perfection is the enemy of the good and all that, but a few lines below, we
> actually have the same issue for ACTUAL_SOURCE.

Nope, because this is only ever attempted if the actual source is not
the same as the main source and if ACTUAL_SOURCE is not empty:

   852 $(2)_TARGET_ACTUAL_SOURCE =»$$($(2)_DIR)/.stamp_actual_downloaded
   ...
   974 # Only download the actual source if it differs from the 'main' archive
   975 ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),)
   976 ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE))
   977 $(1)-legal-source:» $$($(2)_TARGET_ACTUAL_SOURCE)
   978 endif # actual sources != sources
   979 endif # actual sources != ""

So I believe that we do not have the issue with ACTUAL_SOURCE.

>  So maybe instead the condition should be moved inside the DOWNLOAD macro,
> and it should check $(notdir $(1)).

I don't think this is correct. If the dl-wrapper were to be changed,
then it should be changed to not accept to be called without any URI.

Afteral, if we call the dl-wrapper, that's exactly because we want to
download something, so it does not make sense to call it in a way that
will not trigger any download.

Regards,
Yann E. MORIN.

-- 
                                        ____________
.-----------------.--------------------:       _    :------------------.
|  Yann E. MORIN  | Real-Time Embedded |    __/ )   | /"\ ASCII RIBBON |
| +33 534.541.179 | Software  Designer |  _/ - /'   | \ / CAMPAIGN     |
| +33 638.411.245 '--------------------: (_    `--, |  X  AGAINST      |
|      yann.morin (at) orange.com      |_="    ,--' | / \ HTML MAIL    |
'--------------------------------------:______/_____:------------------'


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-11 20:04   ` yann.morin
@ 2022-05-12 21:54     ` Arnout Vandecappelle
  2022-05-13 14:18       ` yann.morin
  0 siblings, 1 reply; 7+ messages in thread
From: Arnout Vandecappelle @ 2022-05-12 21:54 UTC (permalink / raw)
  To: yann.morin; +Cc: buildroot



On 11/05/2022 22:04, yann.morin@orange.com wrote:
> Arnout, All,
> 
> On 2022-05-11 19:15 +0200, Arnout Vandecappelle spake thusly:
>> On 11/05/2022 08:23, yann.morin@orange.com wrote:
>>> From: "Yann E. MORIN" <yann.morin@orange.com>
> [--SNIP--]
>>> So, revert to the behaviour from before f0c7cb01a960, where no download
>>> is attempted for a package without a source (really, without a main
>>> download, now).
>>>
>>> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
>>> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>
>>> ---
>>>   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 38219ce088..b233b07548 100644
>>> --- a/package/pkg-generic.mk
>>> +++ b/package/pkg-generic.mk
>>> @@ -192,7 +192,7 @@ $(BUILD_DIR)/%/.stamp_downloaded:
>>>   			break ; \
>>>   		fi ; \
>>>   	done
>>> -	$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS)))
>>> +	$(if $($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS))))
>>
>>   Perfection is the enemy of the good and all that, but a few lines below, we
>> actually have the same issue for ACTUAL_SOURCE.
> 
> Nope, because this is only ever attempted if the actual source is not
> the same as the main source and if ACTUAL_SOURCE is not empty:
> 
>     852 $(2)_TARGET_ACTUAL_SOURCE =»$$($(2)_DIR)/.stamp_actual_downloaded
>     ...
>     974 # Only download the actual source if it differs from the 'main' archive
>     975 ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),)
>     976 ifneq ($$($(2)_ACTUAL_SOURCE_TARBALL),$$($(2)_SOURCE))
>     977 $(1)-legal-source:» $$($(2)_TARGET_ACTUAL_SOURCE)
>     978 endif # actual sources != sources
>     979 endif # actual sources != ""
> 
> So I believe that we do not have the issue with ACTUAL_SOURCE.

  True, but feels brittle.

>>   So maybe instead the condition should be moved inside the DOWNLOAD macro,
>> and it should check $(notdir $(1)).
> 
> I don't think this is correct. If the dl-wrapper were to be changed,
> then it should be changed to not accept to be called without any URI.

  I was talking about the DOWNLOAD macro, not the dl-wrapper. I agree that 
dl-wrapper should error out if the -f parameter is empty (or contains slashes).


> Afteral, if we call the dl-wrapper, that's exactly because we want to
> download something, so it does not make sense to call it in a way that
> will not trigger any download.

  I do admit that you could claim the same of the DOWNLOAD macro.


  Regards,
  Arnout

> 
> Regards,
> Yann E. MORIN.
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-12 21:54     ` Arnout Vandecappelle
@ 2022-05-13 14:18       ` yann.morin
  0 siblings, 0 replies; 7+ messages in thread
From: yann.morin @ 2022-05-13 14:18 UTC (permalink / raw)
  To: Arnout Vandecappelle; +Cc: buildroot

Arnout, All,

On 2022-05-12 23:54 +0200, Arnout Vandecappelle spake thusly:
> On 11/05/2022 22:04, yann.morin@orange.com wrote:
> >On 2022-05-11 19:15 +0200, Arnout Vandecappelle spake thusly:
[--SNIP--]
> >>  Perfection is the enemy of the good and all that, but a few lines below, we
> >>actually have the same issue for ACTUAL_SOURCE.
[--SNIP--]
> >So I believe that we do not have the issue with ACTUAL_SOURCE.
>  True, but feels brittle.

Agreed.

> >>  So maybe instead the condition should be moved inside the DOWNLOAD macro,
> >>and it should check $(notdir $(1)).
> >I don't think this is correct. If the dl-wrapper were to be changed,
> >then it should be changed to not accept to be called without any URI.
>  I was talking about the DOWNLOAD macro, not the dl-wrapper. I agree that
> dl-wrapper should error out if the -f parameter is empty (or contains
> slashes).
> 
> >Afteral, if we call the dl-wrapper, that's exactly because we want to
> >download something, so it does not make sense to call it in a way that
> >will not trigger any download.
>  I do admit that you could claim the same of the DOWNLOAD macro.

Indeed, there is no reason to call the DOWNLOAD macro if there is
nothing to download. ;-)

What the current patch does is fix a regression in a previous change.

Then, for _ACTUAL_SOURCE, I do agree that the situation is a bit
fragile, but we did not have an issue so far as it is correctly
protected, albeit in a way that makes it not very maintainable.
Enhancing the situation can be done in another patch.

So, I'm going to be a bit stubborn here, and stand by my proposal in
this patch, if you will.

Regards,
Yann E. MORIN.

-- 
                                        ____________
.-----------------.--------------------:       _    :------------------.
|  Yann E. MORIN  | Real-Time Embedded |    __/ )   | /"\ ASCII RIBBON |
| +33 534.541.179 | Software  Designer |  _/ - /'   | \ / CAMPAIGN     |
| +33 638.411.245 '--------------------: (_    `--, |  X  AGAINST      |
|      yann.morin (at) orange.com      |_="    ,--' | / \ HTML MAIL    |
'--------------------------------------:______/_____:------------------'


_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-11  6:23 [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source yann.morin
  2022-05-11 17:15 ` Arnout Vandecappelle
@ 2022-05-14  9:47 ` Arnout Vandecappelle
  2022-05-28 19:34 ` Peter Korsgaard
  2 siblings, 0 replies; 7+ messages in thread
From: Arnout Vandecappelle @ 2022-05-14  9:47 UTC (permalink / raw)
  To: yann.morin, buildroot



On 11/05/2022 08:23, yann.morin@orange.com wrote:
> From: "Yann E. MORIN" <yann.morin@orange.com>
> 
> Recent commit f0c7cb01a960 (package/pkg-download: do not try to vendor
> _EXTRA_DOWNLOADS) got last-minute changes when applied, which changed
> the expected behaviour for packages that do not have a main download.
> 
> Before f0c7cb01a960, the dl-wrapper would not even be called for those
> packages, and the original patch that was sent also avoided downloading
> such packages, but f0c7cb01a960 now causes the dl-wrapper to be called.
> 
> It is however an accident that the dl-wrapper does not fail. Indeed, it
> is expected to fail if no download was successful; we pass no URI, so
> the dl-wrapper should have failed, as it basically does:
> 
>      download_and_check=0
>      for uri in "${uris[@]}"; do
>          ...
>      done
>      if [ "${download_and_check}" -eq 0 ]; then
>          exit 1
>      fi
> 
> However, it does not even go that far...
> 
> Even though there is no output file, we still pass the path to the
> package output directory as the output path. So, to avoid downloading
> files already present, the wrapper checks if the output file exists,
> and checks its hash:
> 
>      if [ -e "${output}" ]; then
>          if support/download/check-hash ${quiet} "${hfile}" "${output}" ...
>              exit 0
>          ...
>      fi
> 
> The output path does exist now, because we explicitly create it just
> before calling the wrapper, because that's where we also locate the
> lockfile.
> 
> So it ends up trying to validate the hash of a directory, but it fails
> to, as there is indeed no hash file for that package. And a missing hash
> file is just a warning, not an error, which makes the download actually
> a success...
> 
> So, this is currently working, and this is by pure luck.
> 
> However, there is a potential issue: if a target package is a virtual
> package, but the host package is a real package, e.g. the same foo.mk
> does (or the other way around):
> 
>      HOST_FOO_VERSION = 1.2.3
>      HOST_FOO_SITE = http://example.net/
>      $(eval $(virtual-package))
>      $(eval $(host-generic-package))
> 
> If there is a hash file to validate the host download, then the current
> situation will cause a failure, because there would be a hash file, but
> no hash for the output path of the target variant, which would then be
> a hard-error.
> 
> So, revert to the behaviour from before f0c7cb01a960, where no download
> is attempted for a package without a source (really, without a main
> download, now).
> 
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

  Applied to master, thanks.

  Regards,
  Arnout

> ---
>   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 38219ce088..b233b07548 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -192,7 +192,7 @@ $(BUILD_DIR)/%/.stamp_downloaded:
>   			break ; \
>   		fi ; \
>   	done
> -	$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS)))
> +	$(if $($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$($(PKG)_MAIN_DOWNLOAD),$(PKG),$(patsubst %,-p '%',$($(PKG)_DOWNLOAD_POST_PROCESS))))
>   	$(foreach p,$($(PKG)_ADDITIONAL_DOWNLOADS),$(call DOWNLOAD,$(p),$(PKG))$(sep))
>   	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
>   	$(Q)mkdir -p $(@D)
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source
  2022-05-11  6:23 [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source yann.morin
  2022-05-11 17:15 ` Arnout Vandecappelle
  2022-05-14  9:47 ` Arnout Vandecappelle
@ 2022-05-28 19:34 ` Peter Korsgaard
  2 siblings, 0 replies; 7+ messages in thread
From: Peter Korsgaard @ 2022-05-28 19:34 UTC (permalink / raw)
  To: yann.morin; +Cc: buildroot

>>>>>   <yann.morin@orange.com> writes:

 > From: "Yann E. MORIN" <yann.morin@orange.com>
 > Recent commit f0c7cb01a960 (package/pkg-download: do not try to vendor
 > _EXTRA_DOWNLOADS) got last-minute changes when applied, which changed
 > the expected behaviour for packages that do not have a main download.

 > Before f0c7cb01a960, the dl-wrapper would not even be called for those
 > packages, and the original patch that was sent also avoided downloading
 > such packages, but f0c7cb01a960 now causes the dl-wrapper to be called.

 > It is however an accident that the dl-wrapper does not fail. Indeed, it
 > is expected to fail if no download was successful; we pass no URI, so
 > the dl-wrapper should have failed, as it basically does:

 >     download_and_check=0
 >     for uri in "${uris[@]}"; do
 >         ...
 >     done
 >     if [ "${download_and_check}" -eq 0 ]; then
 >         exit 1
 >     fi

 > However, it does not even go that far...

 > Even though there is no output file, we still pass the path to the
 > package output directory as the output path. So, to avoid downloading
 > files already present, the wrapper checks if the output file exists,
 > and checks its hash:

 >     if [ -e "${output}" ]; then
 >         if support/download/check-hash ${quiet} "${hfile}" "${output}" ...
 >             exit 0
 >         ...
 >     fi

 > The output path does exist now, because we explicitly create it just
 > before calling the wrapper, because that's where we also locate the
 > lockfile.

 > So it ends up trying to validate the hash of a directory, but it fails
 > to, as there is indeed no hash file for that package. And a missing hash
 > file is just a warning, not an error, which makes the download actually
 > a success...

 > So, this is currently working, and this is by pure luck.

 > However, there is a potential issue: if a target package is a virtual
 > package, but the host package is a real package, e.g. the same foo.mk
 > does (or the other way around):

 >     HOST_FOO_VERSION = 1.2.3
 >     HOST_FOO_SITE = http://example.net/
 >     $(eval $(virtual-package))
 >     $(eval $(host-generic-package))

 > If there is a hash file to validate the host download, then the current
 > situation will cause a failure, because there would be a hash file, but
 > no hash for the output path of the target variant, which would then be
 > a hard-error.

 > So, revert to the behaviour from before f0c7cb01a960, where no download
 > is attempted for a package without a source (really, without a main
 > download, now).

 > Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
 > Cc: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

Committed to 2022.02.x, thanks.

-- 
Bye, Peter Korsgaard
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-05-28 19:34 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-11  6:23 [Buildroot] [PATCH] package/pkg-generic: explicitly do not download package withtout source yann.morin
2022-05-11 17:15 ` Arnout Vandecappelle
2022-05-11 20:04   ` yann.morin
2022-05-12 21:54     ` Arnout Vandecappelle
2022-05-13 14:18       ` yann.morin
2022-05-14  9:47 ` Arnout Vandecappelle
2022-05-28 19:34 ` Peter Korsgaard

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.