On Thu, Jul 29, 2021 at 01:50:08PM +0200, Marc Espie wrote:
> 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.
It does make sense to me to chance the default naming of our patches
instead of trying to adjust every upstream file going for the same and
obvious ".orig" ending.
`PATCHORIG ?= .orig.port' seems like a simple solution (until some
upstream uses that exact suffix).
Porters with personal scripts need to adjust from time to time, so I
wouldn't count that as an argument against it -- perhaps go the extra
mile and grab PATCHORIG's value instead of assuming ".orig" in the first
place?
