Re: [PATCH] rsync add zstd,lz4,iconv options as default and -minimal flavor

On Tue, Jul 02, 2024 at 10:24:43AM -0700, Bryan Vyhmeister wrote:
> On Tue, Jul 02, 2024 at 09:39:57AM +0100, Stuart Henderson wrote:
> > On 2024/07/01 18:32, Bryan Vyhmeister wrote:
> > > I updated the title based on the discussion and what this patch actually
> > > does now. I have a particular project that needs zstd compression with
> > > rsync. I can modify the port in my own tree to build zstd into the port
> > > but I would prefer not to have to maintain a private package repository.
> > > Based on the information about zstd out there, it might be a benefit as
> > > the compression in a lot of scenarios where rsync is used. The
> > > discussion led to the suggestion of adding zstd, lz4, and iconv into the
> > > standard rsync build and then adding a -minimal flavor that keeps those
> > > options out. I have been running rsync this way and it works great. Any
> > > feedback on this?
> >
> > Running on my anoncvs/reposync server. It would be nice to have
> > something more cpu-efficient than the rather heavier gzip compression in
> > the package binaries. With the port setup this way, someone running
> > "pkg_add rsync" will get the choice between rsync-3.3.0p1-minimal and
> > rsync-3.3.0p1.
> >
> > Does that seem acceptable to people who complained last time zstd/lz4
> > were enabled in the rsync port? (the commit log doesn't mention names).

People were complaining that it pushed too many deps, both for the
package and for the port. The 'minimal' FLAVOR should cover that.

> @@ -35,7 +33,10 @@ CONFIGURE_ARGS =--disable-lz4 \
> --with-nobody-user=_rsync \
> --with-nobody-group=_rsync
> CONFIGURE_ENV +=CPPFLAGS="-I${LOCALBASE}/include -DXXH_INLINE_ALL=1" \
> - ac_cv_search_XXH64_createState=""
> + LDFLAGS='-L${LOCALBASE}/lib' ac_cv_search_XXH64_createState=""
> +LIB_DEPENDS += archivers/lz4 \
> + archivers/zstd \
> + converters/libiconv
>
> .include <bsd.port.arch.mk>
>
> @@ -43,10 +44,11 @@ CONFIGURE_ENV +=CPPFLAGS="-I${LOCALBASE}
> CONFIGURE_ARGS +=--enable-md5-asm
> .endif
>
> -.if ${FLAVOR:Miconv}
> -CONFIGURE_ENV +=LDFLAGS='-L${LOCALBASE}/lib'
> -LIB_DEPENDS += converters/libiconv
> -WANTLIB += iconv
> +.if ${FLAVOR:Mminimal}
> +CONFIGURE_ARGS +=--disable-lz4 \
> + --disable-zstd
> +WANTLIB = c crypto
> +LIB_DEPENDS =
> .endif

Not tested but I'm pretty sure that this results in iconv being picked
up in the !minimal case. You're adding -L/usr/local/lib to LDFLAGS
unconditionally, but don't prevent iconv from being picked up.
Preventing that is a bit ugly, see my earlier diff in this thread.

I suggest you don't reverse the existing logic for CONFIGURE_ENV,
WANTLIB and LIB_DEPENDS, just add stuff as needed for the !minimal
FLAVOR:

.if !${FLAVOR:Mminimal}
CONFIGURE_ENV += LDFLAGS='-L${LOCALBASE}/lib'
LIB_DEPENDS += archivers/lz4 etc
WANTLIB += blah blah
.endif

afk for two days.

--
jca