[Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[yann.morin]

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

For golang- or cargo-based packages, we apply a vendoring pass after the
package's "main" download is done. Whether to vendor or not is based on
the heuristic that a specific directory exists or not; for golang
packages, we look for '/vendor', while for cargo, we look for '/VENDOR'.

This is fine for the "main" (by lack of a better term) download, but
this falls flat on its face for extra downloads. Indeed, so packages may
need to download data sets, or assets, as _EXTRA_DOWNLOADS. Those are
usually just data blobs, and are not actual golang or cargo packages; as
such they do not need to be vendored, but worse, if we try to actually
vendor them, this fails because the required files for vendoring are
missing from the archives in such data sets.

We fix that by decoupling the download for the extra download, from the
download for the main archive. We pass the post-processing option only
to the main download.

This makes the hard assumption that extra downloads will never need to
be post-processed for vendoring, of course; we hope this will always be
correct in practice.

Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
---
 package/pkg-download.mk | 3 ++-
 package/pkg-generic.mk  | 8 ++++++--
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/package/pkg-download.mk b/package/pkg-download.mk
index a15e21e110..28b31ca30e 100644
--- a/package/pkg-download.mk
+++ b/package/pkg-download.mk
@@ -103,6 +103,7 @@ endif
 #
 # Argument 1 is the source location
 # Argument 2 is the upper-case package name
+# Argument 3 is a sapce-separated list of optional arguments
 #
 ################################################################################
 
@@ -118,10 +119,10 @@ define DOWNLOAD
 		-n '$($(2)_BASENAME_RAW)' \
 		-N '$($(2)_RAWNAME)' \
 		-o '$($(2)_DL_DIR)/$(notdir $(1))' \
-		$(if $($(2)_DOWNLOAD_POST_PROCESS),-p '$($(2)_DOWNLOAD_POST_PROCESS)') \
 		$(if $($(2)_GIT_SUBMODULES),-r) \
 		$(if $($(2)_GIT_LFS),-l) \
 		$(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
+		$(3) \
 		$(QUIET) \
 		-- \
 		$($(2)_DL_OPTS)
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
index 1222526ba1..3a3af44fc4 100644
--- a/package/pkg-generic.mk
+++ b/package/pkg-generic.mk
@@ -192,7 +192,8 @@ $(BUILD_DIR)/%/.stamp_downloaded:
 			break ; \
 		fi ; \
 	done
-	$(foreach p,$($(PKG)_ALL_DOWNLOADS),$(call DOWNLOAD,$(p),$(PKG))$(sep))
+	$(foreach p,$($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$(p),$(PKG),$(if $($(PKG)_DOWNLOAD_POST_PROCESS),-p '$($(PKG)_DOWNLOAD_POST_PROCESS)'))$(sep))
+	$(foreach p,$(filter-out $($(PKG)_MAIN_DOWNLOAD),$($(PKG)_ALL_DOWNLOADS)),$(call DOWNLOAD,$(p),$(PKG))$(sep))
 	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
 	$(Q)mkdir -p $(@D)
 	@$(call step_end,download)
@@ -601,8 +602,11 @@ ifndef $(2)_PATCH
  endif
 endif
 
-$(2)_ALL_DOWNLOADS = \
+$(2)_MAIN_DOWNLOAD = \
 	$$(if $$($(2)_SOURCE),$$($(2)_SITE_METHOD)+$$($(2)_SITE)/$$($(2)_SOURCE)) \
+
+$(2)_ALL_DOWNLOADS = \
+	$$($(2)_MAIN_DOWNLOAD) \
 	$$(foreach p,$$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS),\
 		$$(if $$(findstring ://,$$(p)),$$(p),\
 			$$($(2)_SITE_METHOD)+$$($(2)_SITE)/$$(p)))
-- 
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

Re: [Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[Arnout Vandecappelle]

On 05/05/2022 19:19, yann.morin@orange.com wrote:
> From: "Yann E. MORIN" <yann.morin@orange.com>
> 
> For golang- or cargo-based packages, we apply a vendoring pass after the
> package's "main" download is done. Whether to vendor or not is based on
> the heuristic that a specific directory exists or not; for golang
> packages, we look for '/vendor', while for cargo, we look for '/VENDOR'.
> 
> This is fine for the "main" (by lack of a better term) download, but
> this falls flat on its face for extra downloads. Indeed, so packages may
> need to download data sets, or assets, as _EXTRA_DOWNLOADS. Those are
> usually just data blobs, and are not actual golang or cargo packages; as
> such they do not need to be vendored, but worse, if we try to actually
> vendor them, this fails because the required files for vendoring are
> missing from the archives in such data sets.
> 
> We fix that by decoupling the download for the extra download, from the
> download for the main archive. We pass the post-processing option only
> to the main download.
> 
> This makes the hard assumption that extra downloads will never need to
> be post-processed for vendoring, of course; we hope this will always be
> correct in practice.
> 
> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>


  Applied to master, thanks, with a few changes:

      - no loop needed for MAIN_DOWNLOAD, it can have only one;
      - remove superfluous backslash in the definition of MAIN_DOWNLOAD;
      - introduce _ADDITIONAL_DOWNLOADS to avoid filter-out.


  Regards,
  Arnout

> ---
>   package/pkg-download.mk | 3 ++-
>   package/pkg-generic.mk  | 8 ++++++--
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/package/pkg-download.mk b/package/pkg-download.mk
> index a15e21e110..28b31ca30e 100644
> --- a/package/pkg-download.mk
> +++ b/package/pkg-download.mk
> @@ -103,6 +103,7 @@ endif
>   #
>   # Argument 1 is the source location
>   # Argument 2 is the upper-case package name
> +# Argument 3 is a sapce-separated list of optional arguments
>   #
>   ################################################################################
>   
> @@ -118,10 +119,10 @@ define DOWNLOAD
>   		-n '$($(2)_BASENAME_RAW)' \
>   		-N '$($(2)_RAWNAME)' \
>   		-o '$($(2)_DL_DIR)/$(notdir $(1))' \
> -		$(if $($(2)_DOWNLOAD_POST_PROCESS),-p '$($(2)_DOWNLOAD_POST_PROCESS)') \
>   		$(if $($(2)_GIT_SUBMODULES),-r) \
>   		$(if $($(2)_GIT_LFS),-l) \
>   		$(foreach uri,$(call DOWNLOAD_URIS,$(1),$(2)),-u $(uri)) \
> +		$(3) \
>   		$(QUIET) \
>   		-- \
>   		$($(2)_DL_OPTS)
> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
> index 1222526ba1..3a3af44fc4 100644
> --- a/package/pkg-generic.mk
> +++ b/package/pkg-generic.mk
> @@ -192,7 +192,8 @@ $(BUILD_DIR)/%/.stamp_downloaded:
>   			break ; \
>   		fi ; \
>   	done
> -	$(foreach p,$($(PKG)_ALL_DOWNLOADS),$(call DOWNLOAD,$(p),$(PKG))$(sep))
> +	$(foreach p,$($(PKG)_MAIN_DOWNLOAD),$(call DOWNLOAD,$(p),$(PKG),$(if $($(PKG)_DOWNLOAD_POST_PROCESS),-p '$($(PKG)_DOWNLOAD_POST_PROCESS)'))$(sep))
> +	$(foreach p,$(filter-out $($(PKG)_MAIN_DOWNLOAD),$($(PKG)_ALL_DOWNLOADS)),$(call DOWNLOAD,$(p),$(PKG))$(sep))
>   	$(foreach hook,$($(PKG)_POST_DOWNLOAD_HOOKS),$(call $(hook))$(sep))
>   	$(Q)mkdir -p $(@D)
>   	@$(call step_end,download)
> @@ -601,8 +602,11 @@ ifndef $(2)_PATCH
>    endif
>   endif
>   
> -$(2)_ALL_DOWNLOADS = \
> +$(2)_MAIN_DOWNLOAD = \
>   	$$(if $$($(2)_SOURCE),$$($(2)_SITE_METHOD)+$$($(2)_SITE)/$$($(2)_SOURCE)) \
> +
> +$(2)_ALL_DOWNLOADS = \
> +	$$($(2)_MAIN_DOWNLOAD) \
>   	$$(foreach p,$$($(2)_PATCH) $$($(2)_EXTRA_DOWNLOADS),\
>   		$$(if $$(findstring ://,$$(p)),$$(p),\
>   			$$($(2)_SITE_METHOD)+$$($(2)_SITE)/$$(p)))
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[yann.morin]

Arnout, All,

On 2022-05-05 22:37 +0200, Arnout Vandecappelle spake thusly:
> On 05/05/2022 19:19, yann.morin@orange.com wrote:
> >From: "Yann E. MORIN" <yann.morin@orange.com>
> >
> >For golang- or cargo-based packages, we apply a vendoring pass after the
> >package's "main" download is done. Whether to vendor or not is based on
> >the heuristic that a specific directory exists or not; for golang
> >packages, we look for '/vendor', while for cargo, we look for '/VENDOR'.
> >
> >This is fine for the "main" (by lack of a better term) download, but
> >this falls flat on its face for extra downloads. Indeed, so packages may
> >need to download data sets, or assets, as _EXTRA_DOWNLOADS. Those are
> >usually just data blobs, and are not actual golang or cargo packages; as
> >such they do not need to be vendored, but worse, if we try to actually
> >vendor them, this fails because the required files for vendoring are
> >missing from the archives in such data sets.
> >
> >We fix that by decoupling the download for the extra download, from the
> >download for the main archive. We pass the post-processing option only
> >to the main download.
> >
> >This makes the hard assumption that extra downloads will never need to
> >be post-processed for vendoring, of course; we hope this will always be
> >correct in practice.
> >
> >Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
> >Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
> 
>  Applied to master, thanks, with a few changes:
> 
>      - no loop needed for MAIN_DOWNLOAD, it can have only one;

In fact, that was on purpose: it can have no main download...
Indeed, a package can very well only declare extra downloads...
I forgot to explain that in the commit log, sorry.

So, either we do the loop, which is a simple way to expand to nothing
when there is no item but the reason is not obvious, or we add an
explicit $(if ...) test, which is going to be a bit more verbose...

What's your preference?

>      - remove superfluous backslash in the definition of MAIN_DOWNLOAD;

Ah, yes I forgot to drop it... Good catch.

>      - introduce _ADDITIONAL_DOWNLOADS to avoid filter-out.

I did that initially, but I thought that yet another variable could be
easily avoided with the filtering our. But OK, that's fine with me too.

Thanks!

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

Re: [Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[Arnout Vandecappelle]

On 06/05/2022 07:21, yann.morin@orange.com wrote:
> Arnout, All,
> 
> On 2022-05-05 22:37 +0200, Arnout Vandecappelle spake thusly:
>> On 05/05/2022 19:19, yann.morin@orange.com wrote:
>>> From: "Yann E. MORIN" <yann.morin@orange.com>
>>>
>>> For golang- or cargo-based packages, we apply a vendoring pass after the
>>> package's "main" download is done. Whether to vendor or not is based on
>>> the heuristic that a specific directory exists or not; for golang
>>> packages, we look for '/vendor', while for cargo, we look for '/VENDOR'.
>>>
>>> This is fine for the "main" (by lack of a better term) download, but
>>> this falls flat on its face for extra downloads. Indeed, so packages may
>>> need to download data sets, or assets, as _EXTRA_DOWNLOADS. Those are
>>> usually just data blobs, and are not actual golang or cargo packages; as
>>> such they do not need to be vendored, but worse, if we try to actually
>>> vendor them, this fails because the required files for vendoring are
>>> missing from the archives in such data sets.
>>>
>>> We fix that by decoupling the download for the extra download, from the
>>> download for the main archive. We pass the post-processing option only
>>> to the main download.
>>>
>>> This makes the hard assumption that extra downloads will never need to
>>> be post-processed for vendoring, of course; we hope this will always be
>>> correct in practice.
>>>
>>> Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>
>>
>>   Applied to master, thanks, with a few changes:
>>
>>       - no loop needed for MAIN_DOWNLOAD, it can have only one;
> 
> In fact, that was on purpose: it can have no main download...
> Indeed, a package can very well only declare extra downloads...
> I forgot to explain that in the commit log, sorry.

  OK. However, somehow this seems to work:

package/foo/foo.mk:
FOO_SOURCE =
FOO_VERSION = 1

FOO_INSTALL_TARGET_CMDS = echo '***foo!***'

$(eval $(generic-package))

bash 84467$ make foo
WARNING: no hash file for
 >>> foo 1 Extracting
 >>> foo 1 Patching
 >>> foo 1 Configuring
 >>> foo 1 Building
 >>> foo 1 Installing to target
echo '***foo!***'
***foo!***


  So I don't think it is needed after all? The dl-wrapper script apparently 
doesn't do anything if the filename is empty.

  Regards,
  Arnout

> 
> So, either we do the loop, which is a simple way to expand to nothing
> when there is no item but the reason is not obvious, or we add an
> explicit $(if ...) test, which is going to be a bit more verbose...
> 
> What's your preference?
> 
>>       - remove superfluous backslash in the definition of MAIN_DOWNLOAD;
> 
> Ah, yes I forgot to drop it... Good catch.
> 
>>       - introduce _ADDITIONAL_DOWNLOADS to avoid filter-out.
> 
> I did that initially, but I thought that yet another variable could be
> easily avoided with the filtering our. But OK, that's fine with me too.
> 
> Thanks!
> 
> Regards,
> Yann E. MORIN.
> 
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot

Re: [Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[yann.morin]

Arnout, All,

On 2022-05-10 21:24 +0200, Arnout Vandecappelle spake thusly:
> On 06/05/2022 07:21, yann.morin@orange.com wrote:
> >On 2022-05-05 22:37 +0200, Arnout Vandecappelle spake thusly:
[--SNIP--]
> >>  Applied to master, thanks, with a few changes:
> >>      - no loop needed for MAIN_DOWNLOAD, it can have only one;
> >In fact, that was on purpose: it can have no main download...
> >Indeed, a package can very well only declare extra downloads...
> >I forgot to explain that in the commit log, sorry.
>  OK. However, somehow this seems to work:

Yes, otherwise we could not even install host-skeleton to begin with....

>  So I don't think it is needed after all?

Still, I am not happy that we are in a situation that works by accident.
I'd much prefer we understand why exactly this does not break...

> The dl-wrapper script apparently
> doesn't do anything if the filename is empty.

It really works by accident:

 1. we pass no URI, so I thought the dlwrapper would fail, 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...

 2. even though there is no output file, we still pass the path to
    the package output directory as the output path. So, as part of
    validating the download, 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

 3. 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, this is all a bit fragile, and I am not feeling very happy about
that...

So, I'll cook up a fixup patch to revert to the previous behaviour of
not even attempting a download in that case.

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

Re: [Buildroot] [PATCH] package/pkg-download: do not try to vendor _EXTRA_DOWNLOADS

[Peter Korsgaard]

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

 > From: "Yann E. MORIN" <yann.morin@orange.com>
 > For golang- or cargo-based packages, we apply a vendoring pass after the
 > package's "main" download is done. Whether to vendor or not is based on
 > the heuristic that a specific directory exists or not; for golang
 > packages, we look for '/vendor', while for cargo, we look for '/VENDOR'.

 > This is fine for the "main" (by lack of a better term) download, but
 > this falls flat on its face for extra downloads. Indeed, so packages may
 > need to download data sets, or assets, as _EXTRA_DOWNLOADS. Those are
 > usually just data blobs, and are not actual golang or cargo packages; as
 > such they do not need to be vendored, but worse, if we try to actually
 > vendor them, this fails because the required files for vendoring are
 > missing from the archives in such data sets.

 > We fix that by decoupling the download for the extra download, from the
 > download for the main archive. We pass the post-processing option only
 > to the main download.

 > This makes the hard assumption that extra downloads will never need to
 > be post-processed for vendoring, of course; we hope this will always be
 > correct in practice.

 > Signed-off-by: Yann E. MORIN <yann.morin@orange.com>
 > Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com>

Committed to 2022.02.x, thanks.

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