| Message ID | 17102_1652250198_627B5656_17102_437_1_abf36ee4172876cd86eaa1ce6691ddca377f8b0b.1652250196.git.yann.morin@orange.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | package/pkg-generic: explicitly do not download package withtout source | expand |
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)
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.
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. >
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.
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)
>>>>> <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.
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)