Re: git: e64780fbc394 - main - xinstall: do not use copy_file_range(2) when BOOTSTRAPPING

Thanks for the quick fix. I'm not sure if it matters for performance but it
is a bit too restrictive: modern Linux also has the function so it works
just fine there (GitHub CI was happy with this commit on Linux just not
MacOS). And as Jess pointed out we should probably use it on FreeBSD as
well.

Alex


On Sat, 8 Jul 2023, 17:18 Jessica Clarke, <jrtc27@freebsd.org> wrote:

> On 9 Jul 2023, at 00:48, Martin Matuska <mm@FreeBSD.org> wrote:
> >
> > The branch main has been updated by mm:
> >
> > URL:
> https://cgit.FreeBSD.org/src/commit/?id=e64780fbc394b10581e50a850cc06c1c12a8e4f9
> >
> > commit e64780fbc394b10581e50a850cc06c1c12a8e4f9
> > Author:     Martin Matuska <mm@FreeBSD.org>
> > AuthorDate: 2023-07-08 23:24:38 +0000
> > Commit:     Martin Matuska <mm@FreeBSD.org>
> > CommitDate: 2023-07-08 23:25:23 +0000
> >
> >    xinstall: do not use copy_file_range(2) when BOOTSTRAPPING
> >
> >    Reported by:    arichardson
> > ---
> > usr.bin/xinstall/Makefile   | 5 +++++
> > usr.bin/xinstall/xinstall.c | 4 ++++
> > 2 files changed, 9 insertions(+)
> >
> > diff --git a/usr.bin/xinstall/Makefile b/usr.bin/xinstall/Makefile
> > index 9969ef104e98..3b49cb39d27a 100644
> > --- a/usr.bin/xinstall/Makefile
> > +++ b/usr.bin/xinstall/Makefile
> > @@ -17,6 +17,11 @@ CFLAGS+= -I${SRCTOP}/lib/libnetbsd
> > LIBADD= md
> > CFLAGS+= -DWITH_MD5 -DWITH_RIPEMD160
> >
> > +.ifdef BOOTSTRAPPING
> > +# For the bootstrap we disable copy_file_range()
> > +CFLAGS+= -DBOOTSTRAP_XINSTALL
>
> If the bootstrap xinstall is allowed to use copy_file_range on FreeBSD
> (i.e. it’s guaranteed to exist) this is overly restrictive and should
> be:
>
> .if defined(BOOTSTRAPPING) && ${.MAKE.OS} != "FreeBSD"
>
> > +.endif
> > +
> > HAS_TESTS=
> > SUBDIR.${MK_TESTS}+= tests
> >
> > diff --git a/usr.bin/xinstall/xinstall.c b/usr.bin/xinstall/xinstall.c
> > index 8dace862ef1e..6c269bbb5d91 100644
> > --- a/usr.bin/xinstall/xinstall.c
> > +++ b/usr.bin/xinstall/xinstall.c
> > @@ -1300,7 +1300,9 @@ copy(int from_fd, const char *from_name, int
> to_fd, const char *to_name,
> > static size_t bufsize;
> > int nr, nw;
> > int serrno;
> > +#ifndef BOOTSTRAP_XINSTALL
> > ssize_t ret;
> > +#endif
> > char *p;
> > int done_copy;
> > DIGEST_CTX ctx;
> > @@ -1311,6 +1313,7 @@ copy(int from_fd, const char *from_name, int
> to_fd, const char *to_name,
> > if (lseek(to_fd, (off_t)0, SEEK_SET) == (off_t)-1)
> > err(EX_OSERR, "lseek: %s", to_name);
> >
> > +#ifndef BOOTSTRAP_XINSTALL
> > /* Try copy_file_range() if no digest is requested */
> > if (digesttype == DIGEST_NONE) {
> > ret = 1;
> > @@ -1331,6 +1334,7 @@ copy(int from_fd, const char *from_name, int
> to_fd, const char *to_name,
> > /* Fall back */
> > }
> >
> > +#endif
>
> This looks to be in the wrong place wrt whitespace?
>
> Jess
>
> > digest_init(&ctx);
> >
> > done_copy = 0;
>
>