Date: Tue, 22 Sep 2020 16:55:16 -0500 From: Kyle Evans <kevans@freebsd.org> To: Ian Lepore <ian@freebsd.org> Cc: Warner Losh <imp@bsdimp.com>, Alan Somers <asomers@freebsd.org>, Mateusz Guzik <mjguzik@gmail.com>, src-committers <src-committers@freebsd.org>, svn-src-all <svn-src-all@freebsd.org>, svn-src-head <svn-src-head@freebsd.org> Subject: Re: svn commit: r365643 - head/bin/cp Message-ID: <CACNAnaF5XdL2vF%2BofHPXzEu=Pi9=pTaGLnv9evryfvV6y=sBSA@mail.gmail.com> In-Reply-To: <fa3840e0a94d239c3ca42bdb7f16f033b90fbd98.camel@freebsd.org> References: <202009112049.08BKnavL032212@repo.freebsd.org> <CACNAnaGXCt=PezNX-5KU7xTk%2By-um37p8sk1c1KUEc4ywDQ5uQ@mail.gmail.com> <CAOtMX2gtCmyRrhUVia6Arc_PCbgoasG61Xwt=cUaDd1Q1Gtw8g@mail.gmail.com> <CAGudoHEoF3T=hD7-tSNzz0--xKB3LwbjZQH1Xtxujq19hdm2vg@mail.gmail.com> <CAOtMX2i9v73ydzCNz-ZWEZvTot68%2BoT7c0UBE4soOMOcRjBiyA@mail.gmail.com> <CACNAnaGO2FJeKOPXJZ3-etyQf03hvk-kuqj75hKCpg3JQD54=g@mail.gmail.com> <CAOtMX2geGiny1rDMKE7v94R33gb0NcpcJ9k%2B3TrHzsdc5DC8hQ@mail.gmail.com> <CACNAnaGDULoCpp7Rh%2B9mSfC-=uSXiHWOPh9_STpy2yA%2BtSk85A@mail.gmail.com> <CAOtMX2iPqRh6P3JSX3V0erEAzaeN1g4jCAyhTz5O3__ffsBtkA@mail.gmail.com> <CANCZdfqx%2BOboQFZ4xzCeOdNahPkNP3X8yeMD6uVSwHvv2=cinA@mail.gmail.com> <CACNAnaH-L%2Bw=SJW1Cw1M63LQi19d%2Bf_8Sv%2BdYUTyE%2BjYQU6nbA@mail.gmail.com> <CANCZdfrfOrFAkcRWbhQSW9qgyJ6JFyMrgMqrN=YS7eKoT1FmEA@mail.gmail.com> <CAOtMX2iNhii4tCwvTSn7JGNAJ_afoyjwQYM=8MUo4fitt992Kg@mail.gmail.com> <CANCZdfqs8Hrmm-mHConJCW29PRHAPByTwP8J0aTR6naxNL7Ncg@mail.gmail.com> <fa3840e0a94d239c3ca42bdb7f16f033b90fbd98.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Sep 22, 2020 at 4:53 PM Ian Lepore <ian@freebsd.org> wrote:
>
> On Tue, 2020-09-22 at 15:50 -0600, Warner Losh wrote:
> > I think it's a great leap sideways, but I've done cp /dev/null foo to
> > clear
> > it out for 35 years now... It's why it feels like a workaround.
> >
> > Though it is a legit optimization, no matter the feelings. As for
> > clearer,
> > I'm less sure since then I have to remember what the : operator does.
> >
> > Warner
> >
>
> For me, :> is idiomatic (but ugly).
>
> On the other hand, the cp /dev/null had a nice dogfooding aspect to
> it... when we broke cp by accident, its use in the build system was the
> first alarm to go off.
>
> --Ian
>
To be honest, this is a case that really should be covered by
regression tests somewhere.
> > On Tue, Sep 22, 2020 at 3:48 PM Alan Somers <asomers@freebsd.org>
> > wrote:
> >
> > > It doesn't feel like a workaround to me. I think Kyle's version is
> > > clearer than the original.
> > >
> > > On Tue, Sep 22, 2020 at 3:45 PM Warner Losh <imp@bsdimp.com> wrote:
> > >
> > > >
> > > >
> > > > On Tue, Sep 22, 2020 at 3:42 PM Kyle Evans <kevans@freebsd.org>
> > > > wrote:
> > > >
> > > > > cp is already fixed, people are still feeling the fallout of
> > > > > being
> > > > > within those revisions and needing to bootstrap their own cp.
> > > > > We can
> > > > > reduce the number of components these invocations rely on
> > > > > trivially to
> > > > > shell built-in mechanics, why not do so?
> > > > >
> > > >
> > > > Fair point. I just bristle at putting workarounds in for valid
> > > > /bin/sh
> > > > syntax because we opposed for a few days. so long as it's an
> > > > unconditional
> > > > clearing of the file to be zero length, I'm OK with that.
> > > >
> > > > Warner
> > > >
> > > >
> > > > > On Tue, Sep 22, 2020 at 4:40 PM Warner Losh <imp@bsdimp.com>
> > > > > wrote:
> > > > > >
> > > > > > So why do we need a workaround at all? cp /dev/null has been
> > > > > > fixed,
> > > > >
> > > > > and that's way more important to get right.
> > > > > >
> > > > > > I don't want to paper-over issues with this at all, though if
> > > > > > we use
> > > > >
> > > > > the host's (now broken) cp, I suppose that might be OK in the
> > > > > short term.
> > > > > If that's the case, then maybe this is OK.
> > > > > >
> > > > > > Otherwise, I'd strongly prefer we fix cp.
> > > > > >
> > > > > > Warner
> > > > > >
> > > > > > On Tue, Sep 22, 2020 at 3:31 PM Alan Somers <
> > > > > > asomers@freebsd.org>
> > > > >
> > > > > wrote:
> > > > > > >
> > > > > > > +1.
> > > > > > >
> > > > > > > On Tue, Sep 22, 2020 at 3:27 PM Kyle Evans <
> > > > > > > kevans@freebsd.org>
> > > > >
> > > > > wrote:
> > > > > > > >
> > > > > > > > I'm running a build at the suggestion of mjg to confirm
> > > > > > > > there aren't
> > > > > > > > any others hiding that can be converted, and I will
> > > > > > > > commit after I've
> > > > > > > > verified that this is it.
> > > > > > > >
> > > > > > > > On Tue, Sep 22, 2020 at 4:02 PM Alan Somers <
> > > > > > > > asomers@freebsd.org>
> > > > >
> > > > > wrote:
> > > > > > > > >
> > > > > > > > > LGTM.
> > > > > > > > >
> > > > > > > > > On Tue, Sep 22, 2020 at 2:59 PM Kyle Evans <
> > > > > > > > > kevans@freebsd.org>
> > > > >
> > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > Perhaps:
> > > > > > > > > >
> > > > > > > > > > diff --git a/stand/i386/zfsboot/Makefile
> > > > >
> > > > > b/stand/i386/zfsboot/Makefile
> > > > > > > > > > index ff315abc0ef..7e362b43a39 100644
> > > > > > > > > > --- a/stand/i386/zfsboot/Makefile
> > > > > > > > > > +++ b/stand/i386/zfsboot/Makefile
> > > > > > > > > > @@ -81,7 +81,7 @@ zfsboot.ld: zfsboot.ldr zfsboot.bin
> > > > > > > > > > ${BTXKERN}
> > > > > > > > > > -o ${.TARGET} -P 1 zfsboot.bin
> > > > > > > > > >
> > > > > > > > > > zfsboot.ldr:
> > > > > > > > > > - cp /dev/null ${.TARGET}
> > > > > > > > > > + :> ${.TARGET}
> > > > > > > > > >
> > > > > > > > > > zfsboot.bin: zfsboot.out
> > > > > > > > > > ${OBJCOPY} -S -O binary zfsboot.out
> > > > > > > > > > ${.TARGET}
> > > > > > > > > > diff --git a/stand/libsa/Makefile
> > > > > > > > > > b/stand/libsa/Makefile
> > > > > > > > > > index effece9e01b..63cd46a9c54 100644
> > > > > > > > > > --- a/stand/libsa/Makefile
> > > > > > > > > > +++ b/stand/libsa/Makefile
> > > > > > > > > > @@ -122,7 +122,7 @@ beforedepend:
> > > > > > > > > > ln -sf ${SRCTOP}/include/arpa/inet.h
> > > > > > > > > > arpa/inet.h; \
> > > > > > > > > > ln -sf ${SRCTOP}/include/arpa/t
> > > > > > > > > > ftp.h arpa/tftp.h; \
> > > > > > > > > > for i in _time.h _strings.h _string.h; do \
> > > > > > > > > > - [ -f xlocale/$$i ] || cp /dev/null
> > > > > > > > > > xlocale/$$i; \
> > > > > > > > > > + [ -f xlocale/$$i ] || :> xlocale/$$i;
> > > > > > > > > > \
> > > > > > > > > > done; \
> > > > > > > > > > for i in ${STAND_H_INC}; do \
> > > > > > > > > > ln -sf ${SASRC}/stand.h $$i; \
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Tue, Sep 22, 2020 at 3:58 PM Alan Somers <
> > > > > > > > > > asomers@freebsd.org>
> > > > >
> > > > > wrote:
> > > > > > > > > > >
> > > > > > > > > > > Looks like two places in stand. Is there any
> > > > > > > > > > > reason why
> > > > >
> > > > > Mateusz's suggestion wouldn't work?
> > > > > > > > > > >
> > > > > > > > > > > > rg -g Makefile 'cp.*/dev/null'
> > > > > > > > > > >
> > > > > > > > > > > stand/libsa/Makefile
> > > > > > > > > > > 125: [ -f xlocale/$$i ] || cp /dev/null
> > > > > > > > > > > xlocale/$$i; \
> > > > > > > > > > >
> > > > > > > > > > > stand/i386/zfsboot/Makefile
> > > > > > > > > > > 82: cp /dev/null ${.TARGET}
> > > > > > > > > > >
> > > > > > > > > > > On Tue, Sep 22, 2020 at 2:54 PM Mateusz Guzik <
> > > > >
> > > > > mjguzik@gmail.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Can we instead add a workaround to the build
> > > > > > > > > > > > tree?
> > > > > > > > > > > >
> > > > > > > > > > > > Where is cp /dev/null coming from anyway? Perhaps
> > > > > > > > > > > > this can be
> > > > >
> > > > > patched
> > > > > > > > > > > > to touch the target file.
> > > > > > > > > > > >
> > > > > > > > > > > > On 9/22/20, Alan Somers <asomers@freebsd.org>
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Tue, Sep 22, 2020 at 2:48 PM Kyle Evans <
> > > > >
> > > > > kevans@freebsd.org> wrote:
> > > > > > > > > > > > >
> > > > > > > > > > > > > > On Fri, Sep 11, 2020 at 3:49 PM Alan Somers <
> > > > >
> > > > > asomers@freebsd.org> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Author: asomers
> > > > > > > > > > > > > > > Date: Fri Sep 11 20:49:36 2020
> > > > > > > > > > > > > > > New Revision: 365643
> > > > > > > > > > > > > > > URL:
> > > > > > > > > > > > > > > https://svnweb.freebsd.org/changeset/base/365643
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Log:
> > > > > > > > > > > > > > > cp: fall back to read/write if
> > > > > > > > > > > > > > > copy_file_range fails
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Even though copy_file_range has a file-
> > > > > > > > > > > > > > > system agnostic
> > > > >
> > > > > version, it
> > > > > > > > > > > > > > still
> > > > > > > > > > > > > > > fails on devfs (perhaps because the file
> > > > > > > > > > > > > > > descriptor is
> > > > >
> > > > > non-seekable?)
> > > > > > > > > > > > > > In
> > > > > > > > > > > > > > > that case, fallback to old-fashioned
> > > > > > > > > > > > > > > read/write. Fixes
> > > > > > > > > > > > > > > "cp /dev/null /tmp/null"
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Any objection to adding a quick UPDATING
> > > > > > > > > > > > > > entry for this?
> > > > >
> > > > > I'm seeing
> > > > > > > > > > > > > > occasional reports of this breakage as recent
> > > > > > > > > > > > > > as today on
> > > > >
> > > > > IRC from
> > > > > > > > > > > > > > folks that were a little bit thrown off by
> > > > > > > > > > > > > > this because it
> > > > >
> > > > > throws up
> > > > > > > > > > > > > > fairly far into the build and looks like a
> > > > > > > > > > > > > > stand build
> > > > >
> > > > > regression
> > > > > > > > > > > > > > instead of a cp regression.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Thanks,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Kyle Evans
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > No objection. Can you suggest the proper
> > > > > > > > > > > > > wording?
> > > > > > > > > > > > > _______________________________________________
> > > > > > > > > > > > > svn-src-all@freebsd.org mailing list
> > > > > > > > > > > > >
> https://lists.freebsd.org/mailman/listinfo/svn-src-all
> > > > > > > > > > > > > To unsubscribe, send any mail to "
> > > > >
> > > > > svn-src-all-unsubscribe@freebsd.org"
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > --
> > > > > > > > > > > > Mateusz Guzik <mjguzik gmail.com>
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CACNAnaF5XdL2vF%2BofHPXzEu=Pi9=pTaGLnv9evryfvV6y=sBSA>