Re: [ptxdist] [PATCH] zstd: new package

[Michael Olbrich <m.olbrich@pengutronix.de>]

On Thu, Jan 02, 2020 at 02:35:43PM +0100, Roland Hieber wrote:
> Hi Florian,
> 
> unfortunately your mailer seems to apply word wrapping to the patch, so
> it didn't apply for me. The git-format-patch manpage [1] has some tips how to
> prevent that.
> 
> [1]: https://git-scm.com/docs/git-format-patch#_thunderbird
> 
> Nevertheless some feedback below:
> 
> On Fri, Dec 27, 2019 at 06:13:19PM +0100, Florian Faber wrote:
> > 
> > Add zstd-1.4.4 to the repository.
> > 
> > Signed-off-by: Florian Faber <faber@faberman.de>
> > ---
> > diff -upNr a/zstd.in rules/zstd.in
> > --- a/zstd.in   1970-01-01 01:00:00.000000000 +0100
> > +++ b/zstd.in   2019-12-18 16:56:31.000000000 +0100
> > @@ -0,0 +1,42 @@
> > +## SECTION=shell_and_console
> > +
> > +menuconfig ZSTD
> > +       bool
> > +       prompt "zstd"
> 
> We'd like all "-->" submenu markers to line up in the graphical menu, so
> please add an appropriate amount of whitespace to the end of the string.
> 
> Also please indent the lines with one tab here instead of spaces.
> 
> > +       select HOST_CMAKE
> > +       help
> > +         zstd is a fast lossless compression algorithm and data
> > +         compression tool, with command line syntax similar to
> > +         gzip and xz. It is based on the LZ77 family, with further
> > +         FSE & huff0 entropy stages. zstd offers highly configurable
> > +         compression speed, with fast modes at > 200 MB/s per code,
> > +         and strong modes nearing lzma compression ratios. It also
> > +         features a very fast decoder, with speeds > 500 MB/s per core.
> > +
> > +if ZSTD
> > +
> > +config ZSTD_LIBZSTD
> > +       tristate
> 
> Hmm, sub-options usually are not tristate, but I guess it doesn't hurt
> either.
> 
> > +       prompt "install zstd library"
> > +
> > +config ZSTD_ZSTD
> > +       tristate
> > +       select ZSTD_LIBZSTD
> > +       prompt "install zstd tool"
> > +
> > +config ZSTD_ZSTDCAT
> > +       tristate
> > +       select ZSTD_LIBZSTD
> > +       prompt "install zstdcat"
> 
> zstdcat seems to be a link to zstd, so you have to select
> ZSTD_ZSTD here too.

no need for separate options if it's just a link.

> > +
> > +config ZSTD_ZSTDGREP
> > +       tristate
> > +       select ZSTD_LIBZSTD
> > +       prompt "install zstdgrep"
> > +
> > +config ZSTD_ZSTDLESS
> > +       tristate
> > +       select ZSTD_LIBZSTD
> > +       prompt "install zstdless"
> > +
> > +endif
> > diff -upNr a/zstd.make rules/zstd.make
> > --- a/zstd.make 1970-01-01 01:00:00.000000000 +0100
> > +++ b/zstd.make 2019-12-18 16:52:12.000000000 +0100
> > @@ -0,0 +1,69 @@
> > +# -*-makefile-*-
> > +#
> > +# Copyright (C) 2019 by Florian Faber <faber@faberman.de>
> > +#
> > +# For further information about the PTXdist project and license conditions
> > +# see the README file.
> > +#
> > +
> > +#
> > +# We provide this package
> > +#
> > +PACKAGES-$(PTXCONF_ZSTD) += zstd
> > +
> > +#
> > +# Paths and names
> > +#
> > +ZSTD_VERSION   := 1.4.4
> > +ZSTD_MD5       := 532aa7b3a873e144bbbedd9c0ea87694
> > +ZSTD           := zstd-$(ZSTD_VERSION)
> > +ZSTD_SUFFIX    := tar.gz
> > +ZSTD_URL       :=
> > https://github.com/facebook/zstd/archive/v$(ZSTD_VERSION).$(ZSTD_SUFFIX)
> > +ZSTD_SOURCE    := $(SRCDIR)/$(ZSTD).$(ZSTD_SUFFIX)
> > +ZSTD_DIR       := $(BUILDDIR)/$(ZSTD)
> > +ZSTD_SUBDIR    := build/cmake
> > +ZSTD_LICENSE   := BSD
> > +
> > +#
> > ----------------------------------------------------------------------------
> > +# Prepare
> > +#
> > ----------------------------------------------------------------------------
> 
> Mangled line, keep these separators on one line with the comment marker.

Probably broken by your MUA. The patch does not apply like this.
And all tabs got replaced by tabs. Try using 'git send-email'.

Michael

> > +
> > +ZSTD_CONF_TOOL := cmake
> > +ZSTD_BUILD_DIR  := $(ZSTD_DIR)-build
> > +ZSTD_CONF_OPT  := \
> > +       $(CROSS_CMAKE_USR) \
> > +       -B$(ZSTD_BUILD_DIR)
> > +
> > +#
> > ----------------------------------------------------------------------------
> > +# Target-Install
> > +#
> > ----------------------------------------------------------------------------
> > +
> > +$(STATEDIR)/zstd.targetinstall:
> > +       @$(call targetinfo)
> 
> Make sure to indent with tabs here, not spaces, or make will error out
> because of syntax errors.
> 
> > +
> > +       @$(call install_init, zstd)
> > +       @$(call install_fixup, zstd, PRIORITY, optional)
> > +       @$(call install_fixup, zstd, SECTION, base)
> > +       @$(call install_fixup, zstd, AUTHOR, "Florian Faber
> > <faber@faberman.de>")
> 
> Mangled line.
> 
> > +       @$(call install_fixup, zstd, DESCRIPTION, missing)
> > +
> > +ifdef PTXCONF_ZSTD_LIBZSTD
> > +       $(call install_lib, zstd, 0, 0, 0644, libzstd)
> > +endif
> > +ifdef PTXCONF_ZSTD_ZSTD
> > +       $(call install_copy, zstd, 0, 0, 0755, -, /usr/bin/zstd)
> > +endif
> > +ifdef PTXCONF_ZSTD_ZSTDCAT
> > +       $(call install_copy, zstd, 0, 0, 0755, -, /usr/bin/zstdcat)
> 
> This generates an error:
> 
>   ptxdist: error: file 'distrokit-next/platform-v7a/packages/zstd-1.4.4/usr/bin/zstdcat' is a link
>    using 'distrokit-next/platform-v7a/packages/zstd-1.4.4/usr/bin/zstd' instead
> 
> Use $(call install_link, ...) instead.
> 
>   - Roland
> 
> > +endif
> > +ifdef PTXCONF_ZSTD_ZSTDGREP
> > +       $(call install_copy, zstd, 0, 0, 0755, -, /usr/bin/zstdgrep)
> > +endif
> > +ifdef PTXCONF_ZSTD_ZSTDLESS
> > +       $(call install_copy, zstd, 0, 0, 0755, -, /usr/bin/zstdless)
> > +endif
> > +
> > +       @$(call install_finish, zstd)
> > +
> > +       @$(call touch)
> > -- 
> > Machines can do the work, so people have time to think.
> > 
> > _______________________________________________
> > ptxdist mailing list
> > ptxdist@pengutronix.de
> > 
> 
> -- 
> Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
> Steuerwalder Str. 21                     | https://www.pengutronix.de/ |
> 31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> ptxdist mailing list
> ptxdist@pengutronix.de
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
ptxdist mailing list
ptxdist@pengutronix.de