| Message ID | 0f0805c9d18137c555b446d6b6e7dd2c68d6e770.1486930542.git.baruch@tkos.co.il |
|---|---|
| State | Accepted |
| Headers | show |
On Sun, Feb 12, 2017 at 9:15 PM, Baruch Siach <baruch@tkos.co.il> wrote: > Don't special case $(XZCAT) when constructing DL_TOOLS_DEPENDENCIES. The next > commit will introduce another extractor that automatically builds when not > installed. Introduce EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS that lists > archive extensions for which the extractor is already checked in > support/dependencies/check-host-foo.mk. Use this in the newly introduced > extractor-dependency to populate DL_TOOLS_DEPENDENCIES. > > Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > --- > v3: new patch; split from the next as preparatory cleanup (Thomas DS) > --- > package/pkg-generic.mk | 8 +------- > package/pkg-utils.mk | 7 +++++++ > support/dependencies/check-host-xzcat.mk | 1 + > 3 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk > index 3ca71b03b9df..e8a8021e3c37 100644 > --- a/package/pkg-generic.mk > +++ b/package/pkg-generic.mk > @@ -926,13 +926,7 @@ else ifeq ($$($(2)_SITE_METHOD),cvs) > DL_TOOLS_DEPENDENCIES += cvs > endif # SITE_METHOD > > -# $(firstword) is used here because the extractor can have arguments, like > -# ZCAT="gzip -d -c", and to check for the dependency we only want 'gzip'. > -# Do not add xzcat to the list of required dependencies, as it gets built > -# automatically if it isn't found. > -ifneq ($$(call suitable-extractor,$$($(2)_SOURCE)),$$(XZCAT)) > -DL_TOOLS_DEPENDENCIES += $$(firstword $$(call suitable-extractor,$$($(2)_SOURCE))) > -endif > +DL_TOOLS_DEPENDENCIES += $$(call extractor-dependency,$$($(2)_SOURCE)) > > # Ensure all virtual targets are PHONY. Listed alphabetically. > .PHONY: $(1) \ > diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk > index c5d4080c72f4..4821456da5b0 100644 > --- a/package/pkg-utils.mk > +++ b/package/pkg-utils.mk > @@ -45,6 +45,13 @@ INFLATE.tar = cat > # suitable-extractor(filename): returns extractor based on suffix > suitable-extractor = $(INFLATE$(suffix $(1))) > > +# extractor-dependency(filename): returns extractor for 'filename' if the > +# extractor is a dependency. If we build the extractor return nothing. > +# $(firstword) is used here because the extractor can have arguments, like > +# ZCAT="gzip -d -c", and to check for the dependency we only want 'gzip'. > +extractor-dependency = $(firstword$(INFLATE$(filter-out \ There needs to be a space after 'firstword', otherwise this gets expanded to $(firstword.gz) for example, which is empty. With that fixed: Reviewed-by: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com>
Hello, On Sun, 12 Feb 2017 22:15:38 +0200, Baruch Siach wrote: > Don't special case $(XZCAT) when constructing DL_TOOLS_DEPENDENCIES. The next > commit will introduce another extractor that automatically builds when not > installed. Introduce EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS that lists > archive extensions for which the extractor is already checked in > support/dependencies/check-host-foo.mk. Use this in the newly introduced > extractor-dependency to populate DL_TOOLS_DEPENDENCIES. > > Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> I thought this patch series would be material for the next branch, as to me it was just an improvement/cleanup. But in fact, it is actually a fix: all packages that depend on host-lzip today are broken. Take ddrescue for example (but ed and ocrad are doing the same): DDRESCUE_DEPENDENCIES = host-lzip define DDRESCUE_EXTRACT_CMDS $(HOST_DIR)/usr/bin/lzip -d -c $(DL_DIR)/$(DDRESCUE_SOURCE) | \ tar --strip-components=1 -C $(@D) $(TAR_OPTIONS) - endef So they put host-lzip in <pkg>_DEPENDENCIES, and use $(HOST_DIR)/usr/bin/lzip in the extract step. The problem is that the dependencies in <pkg>_DEPENDENCIES are only guaranteed to be available before the *configure* step of the package, not before the extract step. So if you run: $ make ddrescue-patch You currently get: /home/thomas/projets/buildroot/output/host/usr/bin/lzip -d -c /home/thomas/dl/ddrescue-1.22.tar.lz | tar --strip-components=1 -C /home/thomas/projets/buildroot/output/build/ddrescue-1.22 -xf - /bin/bash: /home/thomas/projets/buildroot/output/host/usr/bin/lzip: No such file or directory tar: This does not look like a tar archive tar: Exiting with failure status due to previous errors So I believe this patch series should be applied on master, because it adds host-lzip to DEPENDENCIES_HOST_PREREQ, which is the only solution to guarantee it will be available to extract packages. Best regards, Thomas
Hi Thomas, On Tue, Feb 14, 2017 at 10:39:38PM +0100, Thomas Petazzoni wrote: > On Sun, 12 Feb 2017 22:15:38 +0200, Baruch Siach wrote: > > Don't special case $(XZCAT) when constructing DL_TOOLS_DEPENDENCIES. The next > > commit will introduce another extractor that automatically builds when not > > installed. Introduce EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS that lists > > archive extensions for which the extractor is already checked in > > support/dependencies/check-host-foo.mk. Use this in the newly introduced > > extractor-dependency to populate DL_TOOLS_DEPENDENCIES. > > > > Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > > Signed-off-by: Baruch Siach <baruch@tkos.co.il> > > I thought this patch series would be material for the next branch, as > to me it was just an improvement/cleanup. > > But in fact, it is actually a fix: all packages that depend on > host-lzip today are broken. Take ddrescue for example (but ed and ocrad > are doing the same): > > DDRESCUE_DEPENDENCIES = host-lzip > > define DDRESCUE_EXTRACT_CMDS > $(HOST_DIR)/usr/bin/lzip -d -c $(DL_DIR)/$(DDRESCUE_SOURCE) | \ > tar --strip-components=1 -C $(@D) $(TAR_OPTIONS) - > endef > > So they put host-lzip in <pkg>_DEPENDENCIES, and use > $(HOST_DIR)/usr/bin/lzip in the extract step. > > The problem is that the dependencies in <pkg>_DEPENDENCIES are only > guaranteed to be available before the *configure* step of the package, > not before the extract step. So if you run: > > $ make ddrescue-patch > > You currently get: > > /home/thomas/projets/buildroot/output/host/usr/bin/lzip -d -c /home/thomas/dl/ddrescue-1.22.tar.lz | tar --strip-components=1 -C /home/thomas/projets/buildroot/output/build/ddrescue-1.22 -xf - > /bin/bash: /home/thomas/projets/buildroot/output/host/usr/bin/lzip: No such file or directory > tar: This does not look like a tar archive > tar: Exiting with failure status due to previous errors > > So I believe this patch series should be applied on master, because it > adds host-lzip to DEPENDENCIES_HOST_PREREQ, which is the only solution > to guarantee it will be available to extract packages. To be on the safe side we might replace <pkg>_DEPENDENCIES with DEPENDENCIES_HOST_PREREQ for master, and apply this series to next. Not sure it's worth it though. Should I resend the series to fix the issue that Thomas DS spotted? baruch
Hello, On Sun, 12 Feb 2017 22:15:38 +0200, Baruch Siach wrote: > Don't special case $(XZCAT) when constructing DL_TOOLS_DEPENDENCIES. The next > commit will introduce another extractor that automatically builds when not > installed. Introduce EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS that lists > archive extensions for which the extractor is already checked in > support/dependencies/check-host-foo.mk. Use this in the newly introduced > extractor-dependency to populate DL_TOOLS_DEPENDENCIES. > > Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> > Signed-off-by: Baruch Siach <baruch@tkos.co.il> I've applied the entire series to master, after fixing the $(firstword ...) function call, as noticed by Thomas DS. I'll make some comments on PATCH 2/5 though. Thomas
diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk index 3ca71b03b9df..e8a8021e3c37 100644 --- a/package/pkg-generic.mk +++ b/package/pkg-generic.mk @@ -926,13 +926,7 @@ else ifeq ($$($(2)_SITE_METHOD),cvs) DL_TOOLS_DEPENDENCIES += cvs endif # SITE_METHOD -# $(firstword) is used here because the extractor can have arguments, like -# ZCAT="gzip -d -c", and to check for the dependency we only want 'gzip'. -# Do not add xzcat to the list of required dependencies, as it gets built -# automatically if it isn't found. -ifneq ($$(call suitable-extractor,$$($(2)_SOURCE)),$$(XZCAT)) -DL_TOOLS_DEPENDENCIES += $$(firstword $$(call suitable-extractor,$$($(2)_SOURCE))) -endif +DL_TOOLS_DEPENDENCIES += $$(call extractor-dependency,$$($(2)_SOURCE)) # Ensure all virtual targets are PHONY. Listed alphabetically. .PHONY: $(1) \ diff --git a/package/pkg-utils.mk b/package/pkg-utils.mk index c5d4080c72f4..4821456da5b0 100644 --- a/package/pkg-utils.mk +++ b/package/pkg-utils.mk @@ -45,6 +45,13 @@ INFLATE.tar = cat # suitable-extractor(filename): returns extractor based on suffix suitable-extractor = $(INFLATE$(suffix $(1))) +# extractor-dependency(filename): returns extractor for 'filename' if the +# extractor is a dependency. If we build the extractor return nothing. +# $(firstword) is used here because the extractor can have arguments, like +# ZCAT="gzip -d -c", and to check for the dependency we only want 'gzip'. +extractor-dependency = $(firstword$(INFLATE$(filter-out \ + $(EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS),$(suffix $(1)))) + # check-deprecated-variable -- throw an error on deprecated variables # example: # $(eval $(call check-deprecated-variable,FOO_MAKE_OPT,FOO_MAKE_OPTS)) diff --git a/support/dependencies/check-host-xzcat.mk b/support/dependencies/check-host-xzcat.mk index 5e08b6e8866e..c6d9eebe4d2c 100644 --- a/support/dependencies/check-host-xzcat.mk +++ b/support/dependencies/check-host-xzcat.mk @@ -3,5 +3,6 @@ ifeq (,$(call suitable-host-package,xzcat,$(XZCAT))) DEPENDENCIES_HOST_PREREQ += host-xz +EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS += .xz .lzma XZCAT = $(HOST_DIR)/usr/bin/xzcat endif
Don't special case $(XZCAT) when constructing DL_TOOLS_DEPENDENCIES. The next commit will introduce another extractor that automatically builds when not installed. Introduce EXTRACTOR_DEPENDENCY_PRECHECKED_EXTENSIONS that lists archive extensions for which the extractor is already checked in support/dependencies/check-host-foo.mk. Use this in the newly introduced extractor-dependency to populate DL_TOOLS_DEPENDENCIES. Cc: Thomas De Schampheleire <thomas.de_schampheleire@nokia.com> Signed-off-by: Baruch Siach <baruch@tkos.co.il> --- v3: new patch; split from the next as preparatory cleanup (Thomas DS) --- package/pkg-generic.mk | 8 +------- package/pkg-utils.mk | 7 +++++++ support/dependencies/check-host-xzcat.mk | 1 + 3 files changed, 9 insertions(+), 7 deletions(-)