RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 3:34 PM, Ævar Arnfjörð Bjarmason
> On Sat, Jan 20 2018, Randall S. Becker jotted:
> 
> > I will reissue this particular patch shortly.
> 
> It makes sense to base that submission on the next branch instead of master.
> I have a patch queued up there which adds two new tar invocations.
> 
> Also re your commit message see the formatting guide in
> Documentation/SubmittingPatches, in particular: instead of:
> 
>  - Add a brief subject line
> 
>  - Just make the body be a normal paragraph instead of a bullet-point
>list with one item.

Got it. I've been swapping between contribution styles so got a little 
confused. I'm going to reissue all of the patches required for the NonStop port 
later tonight or tomorrow, once the test suite run is finished. I'll take 
everyone's comments into account for that. Thanks for your (and everyone 
else's) guidance.

Cheers,
Randall

-- Brief whoami:
  NonStop developer since approximately NonStop(2112884442)
  UNIX developer since approximately 421664400
-- In my real life, I talk too much.

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Ævar Arnfjörð Bjarmason]

On Sat, Jan 20 2018, Randall S. Becker jotted:

> I will reissue this particular patch shortly.

It makes sense to base that submission on the next branch instead of
master. I have a patch queued up there which adds two new tar
invocations.

Also re your commit message see the formatting guide in
Documentation/SubmittingPatches, in particular: instead of:

 - Add a brief subject line

 - Just make the body be a normal paragraph instead of a bullet-point
   list with one item.

RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 9:25 AM, René Scharfe wrote:
> To: Randall S. Becker <rsbec...@nexbridge.com>; git@vger.kernel.org
> Subject: Re: [PATCH v2 2/6] Add tar extract install options override in
> installation processing.
> 
> Am 20.01.2018 um 14:44 schrieb Randall S. Becker:
> > On January 20, 2018 7:31 AM, René Scharfe wrote:
> >> Am 19.01.2018 um 18:34 schrieb randall.s.bec...@rogers.com:
> >>> From: "Randall S. Becker" <rsbec...@nexbridge.com>
> >>>
> >>> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
> >>> specified if needed. The default is xof.
> >>>
> >>> Signed-off-by: Randall S. Becker <rsbec...@nexbridge.com> ---
> >>> Makefile | 6 +- 1 file changed, 5 insertions(+), 1
> >>> deletion(-)
> >>>
> >>> diff --git a/Makefile b/Makefile index 1a9b23b67..040e9eacd
> >>> 100644 --- a/Makefile +++ b/Makefile @@ -429,6 +429,9 @@ all:: #
> >>> running the test scripts (e.g., bash has better support for "set -x"
> >>> # tracing). # +# Define TAR_EXTRACT_OPTIONS if you want to change
> >>> the default +behaviour # from xvf to something else during
> >>> installation.
> >>
> >> "xof" instead of "xvf"?
> >
> > When I look at the parent commit, it says xof, so I wanted to preserve
> > existing behaviour by default. Our install process wants to see the
> > actual set of files, so we wanted to use xvof but that hardly seemed
> > of general interest. I was hoping an option to control it would be
> > agreeable.
> 
> Preserving the default is good. I meant that the default is "xof", but the
> added line implies it was "xvf" instead.
> 
> Seeing your actual use case confirms that my suggestion below would work
> for you.
> 
> >
> >>> +# # When cross-compiling, define HOST_CPU as the canonical name
> >>> of the
> >> CPU on
> >>> # which the built Git will run (for instance "x86_64").
> >>>
> >>> @@ -452,6 +455,7 @@ LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> >>> ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +TAR_EXTRACT_OPTIONS = xof
> >>>
> >>> # Create as necessary, replace existing, make ranlib unneeded.
> >>> ARFLAGS = rcs @@ -2569,7 +2573,7 @@ install: all ifndef NO_GETTEXT
> >>> $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> >>> (cd po/build/locale && $(TAR) cf - .) | \ -   (cd
> >>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -) + (cd
> >>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR)
> >>> +$(TAR_EXTRACT_OPTIONS) -)
> >>
> >> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file)
> >> at the end to go together with the following dash, meaning to extract
> >> from stdin. And x (or -x, or --extract) is probably needed in all
> >> cases as well.  So wouldn't it make more sense to only put the o (or
> >> -o, or --no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and
> f?
> >
> > This is a good suggestion, and I'd love to do that, if I could
> > guarantee a modern tar, which I can't. The platform comes with a
> > really old-school tar from some old (seemingly BSD4.3) epoch that only
> > takes one option set. There is a more modern tar that can be
> > optionally installed if the sysadmin decides to that takes a slightly
> > more modern set, which could support your request, and my team also
> > has a gnu port that is very modern. I can't control what customers are
> > choosing to have installed, unfortunately. Your point is well made and
> > I am completely on board with it, but it introduces a configuration
> > requirement.
> 
> Long options would be nice to nice to have, but are not my main point; I
> included them mainly to spare readers from firing up "man tar" to look up
> the meaning of the short ones.
> 
> I just meant to say that something like this here would make more sense
> because you always need x and f- anyway:
> 
>   TAR_EXTRACT_OPTIONS = o
> 
>   ... ${TAR} x${TAR_EXTRACT_OPTIONS}f -
> 
> > As with the broadening setbuf (patch 2/6) change, I would like to
> > consider this for the future, having a slightly different more complex
> > idea. I could introduce something like this:
> >
> > 1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables
> > this capability all together 2. HAS_ANCIENT_TAR=AreYouKiddingMe
> > (joke) then set up TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond
> the
> > default, so --file, --no-same-owner would always be in effect for that
> > operation.
> >
> > The micro-project would also, logically, need to apply to other tar
> > occurrences throughout the code and potentially need a test case
> > written for it (not entirely sure what that would test, yet).
> > Is that a reasonable approach?
> 
> As long as old-school dash-less flags suffice for our purposes (including
> yours) we can just keep using that style everywhere and avoid adding more
> settings.  It would be a different matter if we needed features that have no
> short flag, or are only offered by certain tar implementations.

Points taken. I will reissue this particular patch shortly.

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[René Scharfe]

Am 20.01.2018 um 14:44 schrieb Randall S. Becker:
> On January 20, 2018 7:31 AM, René Scharfe wrote:
>> Am 19.01.2018 um 18:34 schrieb randall.s.bec...@rogers.com:
>>> From: "Randall S. Becker" 
>>> 
>>> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to
>>> be specified if needed. The default is xof.
>>> 
>>> Signed-off-by: Randall S. Becker  --- 
>>> Makefile | 6 +- 1 file changed, 5 insertions(+), 1
>>> deletion(-)
>>> 
>>> diff --git a/Makefile b/Makefile index 1a9b23b67..040e9eacd
>>> 100644 --- a/Makefile +++ b/Makefile @@ -429,6 +429,9 @@ all:: #
>>> running the test scripts (e.g., bash has better support for "set
>>> -x" # tracing). # +# Define TAR_EXTRACT_OPTIONS if you want to
>>> change the default +behaviour # from xvf to something else during
>>> installation.
>> 
>> "xof" instead of "xvf"?
> 
> When I look at the parent commit, it says xof, so I wanted to
> preserve existing behaviour by default. Our install process wants to
> see the actual set of files, so we wanted to use xvof but that hardly
> seemed of general interest. I was hoping an option to control it
> would be agreeable.

Preserving the default is good. I meant that the default is "xof",
but the added line implies it was "xvf" instead.

Seeing your actual use case confirms that my suggestion below would
work for you.

> 
>>> +# # When cross-compiling, define HOST_CPU as the canonical name
>>> of the
>> CPU on
>>> # which the built Git will run (for instance "x86_64").
>>> 
>>> @@ -452,6 +455,7 @@ LDFLAGS = ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS) 
>>> ALL_LDFLAGS = $(LDFLAGS) STRIP ?= strip +TAR_EXTRACT_OPTIONS =
>>> xof
>>> 
>>> # Create as necessary, replace existing, make ranlib unneeded. 
>>> ARFLAGS = rcs @@ -2569,7 +2573,7 @@ install: all ifndef
>>> NO_GETTEXT $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)' 
>>> (cd po/build/locale && $(TAR) cf - .) | \ - (cd
>>> '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -) +
>>> (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) 
>>> +$(TAR_EXTRACT_OPTIONS) -)
>> 
>> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file)
>> at the end to go together with the following dash, meaning to
>> extract from stdin. And x (or -x, or --extract) is probably needed
>> in all cases as well.  So wouldn't it make more sense to only put
>> the o (or -o, or --no-same-owner) into TAR_EXTRACT_OPTIONS and
>> enforce x and f?
> 
> This is a good suggestion, and I'd love to do that, if I could
> guarantee a modern tar, which I can't. The platform comes with a
> really old-school tar from some old (seemingly BSD4.3) epoch that
> only takes one option set. There is a more modern tar that can be
> optionally installed if the sysadmin decides to that takes a slightly
> more modern set, which could support your request, and my team also
> has a gnu port that is very modern. I can't control what customers
> are choosing to have installed, unfortunately. Your point is well
> made and I am completely on board with it, but it introduces a
> configuration requirement.

Long options would be nice to nice to have, but are not my main
point; I included them mainly to spare readers from firing up
"man tar" to look up the meaning of the short ones.

I just meant to say that something like this here would make more
sense because you always need x and f- anyway:

TAR_EXTRACT_OPTIONS = o

... ${TAR} x${TAR_EXTRACT_OPTIONS}f -

> As with the broadening setbuf (patch 2/6) change, I would like to
> consider this for the future, having a slightly different more
> complex idea. I could introduce something like this:
> 
> 1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables
> this capability all together 2. HAS_ANCIENT_TAR=AreYouKiddingMe
> (joke) then set up TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond
> the default, so --file, --no-same-owner would always be in effect for
> that operation.
> 
> The micro-project would also, logically, need to apply to other tar
> occurrences throughout the code and potentially need a test case
> written for it (not entirely sure what that would test, yet).
> Is that a reasonable approach?

As long as old-school dash-less flags suffice for our purposes
(including yours) we can just keep using that style everywhere and
avoid adding more settings.  It would be a different matter if we
needed features that have no short flag, or are only offered by
certain tar implementations.

René

RE: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[Randall S. Becker]

On January 20, 2018 7:31 AM, René Scharfe wrote:
> Am 19.01.2018 um 18:34 schrieb randall.s.bec...@rogers.com:
> > From: "Randall S. Becker" 
> >
> > * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
> >specified if needed. The default is xof.
> >
> > Signed-off-by: Randall S. Becker 
> > ---
> >   Makefile | 6 +-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/Makefile b/Makefile
> > index 1a9b23b67..040e9eacd 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -429,6 +429,9 @@ all::
> >   # running the test scripts (e.g., bash has better support for "set -x"
> >   # tracing).
> >   #
> > +# Define TAR_EXTRACT_OPTIONS if you want to change the default
> > +behaviour # from xvf to something else during installation.
> 
> "xof" instead of "xvf"?

When I look at the parent commit, it says xof, so I wanted to preserve existing 
behaviour by default. Our install process wants to see the actual set of files, 
so we wanted to use xvof but that hardly seemed of general interest. I was 
hoping an option to control it would be agreeable.

> > +#
> >   # When cross-compiling, define HOST_CPU as the canonical name of the
> CPU on
> >   # which the built Git will run (for instance "x86_64").
> >
> > @@ -452,6 +455,7 @@ LDFLAGS =
> >   ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
> >   ALL_LDFLAGS = $(LDFLAGS)
> >   STRIP ?= strip
> > +TAR_EXTRACT_OPTIONS = xof
> >
> >   # Create as necessary, replace existing, make ranlib unneeded.
> >   ARFLAGS = rcs
> > @@ -2569,7 +2573,7 @@ install: all
> >   ifndef NO_GETTEXT
> > $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
> > (cd po/build/locale && $(TAR) cf - .) | \
> > -   (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
> > +   (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR)
> > +$(TAR_EXTRACT_OPTIONS) -)
> 
> Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file) at the
> end to go together with the following dash, meaning to extract from stdin.
> And x (or -x, or --extract) is probably needed in all cases as well.  So 
> wouldn't
> it make more sense to only put the o (or -o, or
> --no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and f?

This is a good suggestion, and I'd love to do that, if I could guarantee a 
modern tar, which I can't. The platform comes with a really old-school tar from 
some old (seemingly BSD4.3) epoch that only takes one option set. There is a 
more modern tar that can be optionally installed if the sysadmin decides to 
that takes a slightly more modern set, which could support your request, and my 
team also has a gnu port that is very modern. I can't control what customers 
are choosing to have installed, unfortunately. Your point is well made and I am 
completely on board with it, but it introduces a configuration requirement.

As with the broadening setbuf (patch 2/6) change, I would like to consider this 
for the future, having a slightly different more complex idea. I could 
introduce something like this:

1. HAS_ANCIENT_TAR=UnfortunatelyYes in config.mak.uname that disables this 
capability all together
2. HAS_ANCIENT_TAR=AreYouKiddingMe (joke) then set up 
TAR_EXTRACT_ADDITIONAL_OPTIONS above and beyond the default, so --file, 
--no-same-owner would always be in effect for that operation.

The micro-project would also, logically, need to apply to other tar occurrences 
throughout the code and potentially need a test case written for it (not 
entirely sure what that would test, yet).

Is that a reasonable approach?

> 
> >   endif
> >   ifndef NO_PERL
> > $(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)'
> > install
> >

Re: [PATCH v2 2/6] Add tar extract install options override in installation processing.

[René Scharfe]

Am 19.01.2018 um 18:34 schrieb randall.s.bec...@rogers.com:
> From: "Randall S. Becker" 
> 
> * Makefile: Add TAR_EXTRACT_OPTIONS to allow platform options to be
>specified if needed. The default is xof.
> 
> Signed-off-by: Randall S. Becker 
> ---
>   Makefile | 6 +-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 1a9b23b67..040e9eacd 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -429,6 +429,9 @@ all::
>   # running the test scripts (e.g., bash has better support for "set -x"
>   # tracing).
>   #
> +# Define TAR_EXTRACT_OPTIONS if you want to change the default behaviour
> +# from xvf to something else during installation.

"xof" instead of "xvf"?

> +#
>   # When cross-compiling, define HOST_CPU as the canonical name of the CPU on
>   # which the built Git will run (for instance "x86_64").
>   
> @@ -452,6 +455,7 @@ LDFLAGS =
>   ALL_CFLAGS = $(CPPFLAGS) $(CFLAGS)
>   ALL_LDFLAGS = $(LDFLAGS)
>   STRIP ?= strip
> +TAR_EXTRACT_OPTIONS = xof
>   
>   # Create as necessary, replace existing, make ranlib unneeded.
>   ARFLAGS = rcs
> @@ -2569,7 +2573,7 @@ install: all
>   ifndef NO_GETTEXT
>   $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)'
>   (cd po/build/locale && $(TAR) cf - .) | \
> - (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) xof -)
> + (cd '$(DESTDIR_SQ)$(localedir_SQ)' && umask 022 && $(TAR) 
> $(TAR_EXTRACT_OPTIONS) -)

Hmm.  TAR_EXTRACT_OPTIONS always needs to have f (or -f, or --file) at
the end to go together with the following dash, meaning to extract from
stdin.  And x (or -x, or --extract) is probably needed in all cases as
well.  So wouldn't it make more sense to only put the o (or -o, or
--no-same-owner) into TAR_EXTRACT_OPTIONS and enforce x and f?

>   endif
>   ifndef NO_PERL
>   $(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
>