On Thu, Jul 29, 2021 at 12:38:09PM +0100, Stuart Henderson wrote:
> On 2021/07/29 07:10, Sebastien Marie wrote:
> > On Wed, Jul 28, 2021 at 06:39:32PM +0000, Klemens Nanni wrote:
> > > No idea why, but cargo's API from which cargo-module(5) fetches crates
> > > ships tarballs which seem to always have both Cargo.toml *and*
> > > Cargo.toml.orig.
>
> Ah I didn't realise these came directly from distfiles.
>
> > > By using `${PATCHORIG}' instead of hard-coding ".orig" I leave the files
> > > should porters user other PATCHORIG values and therefore avoid the
> > > conflict in the first place.
>
> These files are specifically named .orig though, so rm'ing ${PATCHORIG}
> files just seems wrong. You could maybe skip removing them if PATCHORIG
> is set, not sure if that makes sense though.
>
> > > I know this is raceu since `make PATCHORIG=.foo.orig extract &&
> > > make patch && make update-patches' will still trip over that, but our
> > > framework can't cope with everything and I simply find using
> > > `${PATCHORIG}' in cargo.port.mk both cleaner and more obvious.
> > >
> > >
> > > Feedback? Objections? OK?
> >
> > The current pratice for patched ports using lang/rust is to set
> > PATCHORIG in the Makefile.
> >
> > For quoting bsd.port.mk(5):
> > PATCHORIG
> > Suffix used by patch to rename original files, and update-patches
> > to re-generate ${PATCHDIR}/${PATCH_LIST} by looking for files
> > using this suffix. Defaults to .orig. For a port that already
> > contains .orig files in the ${DISTFILES}, set this to something
> > else, such as .pat.orig. See also distpatch, DISTORIG.
> >
> >
> > About removing upstream files because there are conflicting with your
> > tool, I am unsure. But I am not strongly opposed. But please note that
> > it might not remove all conflicting files, and setting PATCHORIG might
> > be still need.
> >
> > About your patch, it should be done differently:
> >
> > - don't use PATCHORIG in a module. it could be something else than .orig
> > - respect MODCARGO_VENDOR_DIR variable: the Cargo.toml files could be
> > somewhere else than ${WRKDIR}/*
> > - append `rm -f' to existing MODCARGO_post-extract definition (the line
> > before) instead of using +=
> >
> > (your diff doesn't remove Cargo.toml.orig files from crates using
> > MODCARGO_CRATES)
> >
> >
> > The rm line should be something like (untested):
> > rm -f -- ${MODCARGO_CARGOTOML}.orig
> > ${MODCARGO_VENDOR_DIR}/*/Cargo.toml.orig ;
>
> I think I am now leaning towards setting PATCHORIG rather than removing
> them. I see no other modules do this, but I don't think they have a
> widespread problem with .orig files in distfiles so it's not really
> needed elsewhere.
>
> So here's one option (but read on because there's another option worth
> thinking about).
>
> Index: cargo.port.mk
> ===================================================================
> RCS file: /cvs/ports/devel/cargo/cargo.port.mk,v
> retrieving revision 1.22
> diff -u -p -r1.22 cargo.port.mk
> --- cargo.port.mk 27 Apr 2021 06:51:10 -0000 1.22
> +++ cargo.port.mk 29 Jul 2021 11:22:04 -0000
> @@ -49,6 +49,10 @@ MASTER_SITES${MODCARGO_MASTER_SITESN} ?=
> # per crates options
> MODCARGO_CRATES_SQLITE3_BUNDLED ?= No
>
> +# crates often include Cargo.toml.orig files, override the PATCHORIG
> +# default so that update-patches doesn't find them
> +PATCHORIG ?= .orig.port
> +
> # Generated list of DISTFILES.
> .for _cratename _cratever in ${MODCARGO_CRATES}
> DISTFILES +=
> ${_MODCARGO_DIST_SUBDIR}${_cratename}-${_cratever}.tar.gz{${_cratename}/${_cratever}/download}:${MODCARGO_MASTER_SITESN}
>
>
> Index: man5/cargo-module.5
> ===================================================================
> RCS file: /cvs/src/share/man/man5/cargo-module.5,v
> retrieving revision 1.3
> diff -u -p -r1.3 cargo-module.5
> --- man5/cargo-module.5 26 Feb 2021 01:46:52 -0000 1.3
> +++ man5/cargo-module.5 29 Jul 2021 11:27:02 -0000
> @@ -84,6 +84,14 @@ By default
> .Ev MASTER_SITES9
> is used to download the crates.
> .Pp
> +Because crates often include Cargo.toml.orig files,
> +this module changes the default setting of
> +.Ev PATCHORIG
> +to prevent
> +.Xr bsd.port.mk 5 Ns 's
> +.Cm update-patches
> +target from picking them up.
> +.Pp
> This module defines:
> .Bl -tag -width MODCARGO_CRATES_UPDATE
> .It MODCARGO_CARGOTOML
>
>
> Problem though, it is a pain to have differing PATCHORIG settings
> between ports (I think it's fairly common for people working with ports
> to have an alias/wrapper to copy a file to .orig and edit the original,
> it's especially useful with PORTS_PRIVSEP because it can deal with
> setting uid too - so it's quite helpful if these don't differ between
> ports).
>
> Should we just change the overall default for ports instead?
>
> Untested but not likely to have a problem. There will also be some "find
> ... -name *.orig -delete" or similar to change in ports make targets,
> but they won't need to be changed all at once, only when plists are
> updated, and it will be obvious.
>
> Index: bsd.port.mk
> ===================================================================
> RCS file: /cvs/ports/infrastructure/mk/bsd.port.mk,v
> retrieving revision 1.1555
> diff -u -p -r1.1555 bsd.port.mk
> --- bsd.port.mk 3 May 2021 17:53:15 -0000 1.1555
> +++ bsd.port.mk 29 Jul 2021 11:33:54 -0000
> @@ -761,7 +761,7 @@ MAKE_ENV += PATH='${PORTPATH}' PREFIX='$
>
> DISTORIG ?= .bak.orig
> PATCH ?= /usr/bin/patch
> -PATCHORIG ?= .orig
> +PATCHORIG ?= .orig.port
> PATCH_STRIP ?= -p0
> PATCH_DIST_STRIP ?= -p0
Well, a lot of porters, myself included, have aliases and scripts to edit
files that make use of the .orig suffix, so there will be a lot of individual
churn in the second case.
I expect rust to be a Special Needs language, so I'm not sure the second
solution is worth it.
