* [PATCH] documentation: Makefile accounts for SHELL_PATH setting
@ 2009-03-22 13:20 Ben Walton
2009-03-23 6:57 ` Jeff King
2009-04-10 0:34 ` Nanako Shiraishi
0 siblings, 2 replies; 14+ messages in thread
From: Ben Walton @ 2009-03-22 13:20 UTC (permalink / raw)
To: git; +Cc: Ben Walton
Ensure that the Makefile that generates and installs the Documentation
is aware of any SHELL_PATH setting. Use this value if found or the
current setting for SHELL if not. This is an accommodation for systems
where sh is not bash.
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
Documentation/Makefile | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 144ec32..0353690 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -67,6 +67,10 @@ ASCIIDOC_EXTRA += -a docbook-xsl-172
MANPAGE_XSL = manpage-1.72.xsl
endif
+SHELL_PATH ?= $(SHELL)
+# Shell quote;
+SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))
+
#
# Please note that there is a minor bug in asciidoc.
# The version after 6.0.3 _will_ include the patch found here:
@@ -116,7 +120,7 @@ install-pdf: pdf
$(INSTALL) -m 644 user-manual.pdf $(DESTDIR)$(pdfdir)
install-html: html
- sh ./install-webdoc.sh $(DESTDIR)$(htmldir)
+ '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(DESTDIR)$(htmldir)
../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
$(MAKE) -C ../ GIT-VERSION-FILE
@@ -178,7 +182,7 @@ user-manual.xml: user-manual.txt user-manual.conf
technical/api-index.txt: technical/api-index-skel.txt \
technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))
- cd technical && sh ./api-index.sh
+ cd technical && '$(SHELL_PATH_SQ)' ./api-index.sh
$(patsubst %,%.html,$(API_DOCS) technical/api-index): %.html : %.txt
$(ASCIIDOC) -b xhtml11 -f asciidoc.conf \
@@ -220,7 +224,7 @@ $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
howto-index.txt: howto-index.sh $(wildcard howto/*.txt)
$(RM) $@+ $@
- sh ./howto-index.sh $(wildcard howto/*.txt) >$@+
+ '$(SHELL_PATH_SQ)' ./howto-index.sh $(wildcard howto/*.txt) >$@+
mv $@+ $@
$(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
@@ -234,14 +238,14 @@ $(patsubst %.txt,%.html,$(wildcard howto/*.txt)): %.html : %.txt
mv $@+ $@
install-webdoc : html
- sh ./install-webdoc.sh $(WEBDOC_DEST)
+ '$(SHELL_PATH_SQ)' ./install-webdoc.sh $(WEBDOC_DEST)
quick-install: quick-install-man
quick-install-man:
- sh ./install-doc-quick.sh $(DOC_REF) $(DESTDIR)$(mandir)
+ '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(DOC_REF) $(DESTDIR)$(mandir)
quick-install-html:
- sh ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
+ '$(SHELL_PATH_SQ)' ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
.PHONY: .FORCE-GIT-VERSION-FILE
--
1.6.0.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-22 13:20 [PATCH] documentation: Makefile accounts for SHELL_PATH setting Ben Walton
@ 2009-03-23 6:57 ` Jeff King
2009-03-23 12:57 ` Ben Walton
2009-04-10 0:34 ` Nanako Shiraishi
1 sibling, 1 reply; 14+ messages in thread
From: Jeff King @ 2009-03-23 6:57 UTC (permalink / raw)
To: Ben Walton; +Cc: git
On Sun, Mar 22, 2009 at 09:20:44AM -0400, Ben Walton wrote:
> Ensure that the Makefile that generates and installs the Documentation
> is aware of any SHELL_PATH setting. Use this value if found or the
> current setting for SHELL if not. This is an accommodation for systems
> where sh is not bash.
Nit: I have lots of systems where sh is not bash, and they work fine. It
is really about "where sh is not horribly broken". There aren't (AFAIK)
and should not be any bash-isms in these scripts.
The patch itself looks fine; thanks for addressing my concerns.
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-23 6:57 ` Jeff King
@ 2009-03-23 12:57 ` Ben Walton
2009-03-23 14:03 ` Jeff King
0 siblings, 1 reply; 14+ messages in thread
From: Ben Walton @ 2009-03-23 12:57 UTC (permalink / raw)
To: Jeff King; +Cc: GIT List
[-- Attachment #1: Type: text/plain, Size: 852 bytes --]
Excerpts from Jeff King's message of Mon Mar 23 02:57:10 -0400 2009:
> Nit: I have lots of systems where sh is not bash, and they work fine. It
> is really about "where sh is not horribly broken". There aren't (AFAIK)
> and should not be any bash-isms in these scripts.
That's a fair point. I've been lugging this around for a bit in the
build tree I use, so I don't recall specifically what was breaking.
The sh in question is the one found in solaris 8. When I get a few
mintues, I'll build without the patch to see what the problem was...
> The patch itself looks fine; thanks for addressing my concerns.
Thanks for the helpful feedback! :)
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-23 12:57 ` Ben Walton
@ 2009-03-23 14:03 ` Jeff King
2009-03-23 14:12 ` Mike Hommey
0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2009-03-23 14:03 UTC (permalink / raw)
To: Ben Walton; +Cc: GIT List
On Mon, Mar 23, 2009 at 08:57:27AM -0400, Ben Walton wrote:
> That's a fair point. I've been lugging this around for a bit in the
> build tree I use, so I don't recall specifically what was breaking.
> The sh in question is the one found in solaris 8. When I get a few
> mintues, I'll build without the patch to see what the problem was...
I wouldn't worry about it. The /bin/sh on Solaris 8 is pretty hopelessly
broken for our use. At the very least it doesn't understand $()
subsititution.
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-23 14:03 ` Jeff King
@ 2009-03-23 14:12 ` Mike Hommey
0 siblings, 0 replies; 14+ messages in thread
From: Mike Hommey @ 2009-03-23 14:12 UTC (permalink / raw)
To: Jeff King; +Cc: Ben Walton, GIT List
On Mon, Mar 23, 2009 at 10:03:02AM -0400, Jeff King <peff@peff.net> wrote:
> On Mon, Mar 23, 2009 at 08:57:27AM -0400, Ben Walton wrote:
>
> > That's a fair point. I've been lugging this around for a bit in the
> > build tree I use, so I don't recall specifically what was breaking.
> > The sh in question is the one found in solaris 8. When I get a few
> > mintues, I'll build without the patch to see what the problem was...
>
> I wouldn't worry about it. The /bin/sh on Solaris 8 is pretty hopelessly
> broken for our use. At the very least it doesn't understand $()
> subsititution.
Neither does it on solaris 10. Fortunately, there is /usr/xpg4/bin/sh.
Mike
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-22 13:20 [PATCH] documentation: Makefile accounts for SHELL_PATH setting Ben Walton
2009-03-23 6:57 ` Jeff King
@ 2009-04-10 0:34 ` Nanako Shiraishi
2009-04-11 20:42 ` Junio C Hamano
1 sibling, 1 reply; 14+ messages in thread
From: Nanako Shiraishi @ 2009-04-10 0:34 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Ben Walton
Quoting Ben Walton:
> Ensure that the Makefile that generates and installs the Documentation
> is aware of any SHELL_PATH setting. Use this value if found or the
> current setting for SHELL if not. This is an accommodation for systems
> where sh is not bash.
>
> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
Junio, may I ask what happened to this patch?
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-04-10 0:34 ` Nanako Shiraishi
@ 2009-04-11 20:42 ` Junio C Hamano
2009-04-12 1:49 ` Ben Walton
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2009-04-11 20:42 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: git, Ben Walton
Nanako Shiraishi <nanako3@lavabit.com> writes:
> Quoting Ben Walton:
>
>> Ensure that the Makefile that generates and installs the Documentation
>> is aware of any SHELL_PATH setting. Use this value if found or the
>> current setting for SHELL if not. This is an accommodation for systems
>> where sh is not bash.
>>
>> Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
>
> Junio, may I ask what happened to this patch?
There was a discussion going that eventurally petered out without seeing
success (or breakage) reports from people with various platforms.
I think the patch text is Ok, with "where sh is not bash" in the log
message rephrased to "where sh is not POSIX", but I do not have an easy
access to a system where /bin/sh is broken (perhaps I should install
OpenSolaris in a vbox to try out), so it is still sitting in my inbox.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-04-11 20:42 ` Junio C Hamano
@ 2009-04-12 1:49 ` Ben Walton
2009-04-12 8:28 ` Junio C Hamano
0 siblings, 1 reply; 14+ messages in thread
From: Ben Walton @ 2009-04-12 1:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GIT List
[-- Attachment #1: Type: text/plain, Size: 846 bytes --]
Excerpts from Junio C Hamano's message of Sat Apr 11 16:42:39 -0400 2009:
> There was a discussion going that eventurally petered out without seeing
> success (or breakage) reports from people with various platforms.
I didn't see that part of the discussion. Were there breakage
reports? I'm willing to make any required corrections to the patch
such that it gets included (then I could drop it locally).
> I think the patch text is Ok, with "where sh is not bash" in the log
> message rephrased to "where sh is not POSIX", but I do not have an
> easy
I'll resubmit with the wording correction and any other requested
alterations.
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-04-12 1:49 ` Ben Walton
@ 2009-04-12 8:28 ` Junio C Hamano
2009-04-13 14:21 ` Ben Walton
0 siblings, 1 reply; 14+ messages in thread
From: Junio C Hamano @ 2009-04-12 8:28 UTC (permalink / raw)
To: Ben Walton; +Cc: GIT List
Ben Walton <bwalton@artsci.utoronto.ca> writes:
> Excerpts from Junio C Hamano's message of Sat Apr 11 16:42:39 -0400 2009:
>> There was a discussion going that eventurally petered out without seeing
>> success (or breakage) reports from people with various platforms.
>
> I didn't see that part of the discussion. Were there breakage
> reports? I'm willing to make any required corrections to the patch
> such that it gets included (then I could drop it locally).
I do not recall breakage reports nor success reports. At least it does
not seem to break things for me, but I do not do anything so...
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] documentation: Makefile accounts for SHELL_PATH setting
@ 2009-03-21 2:40 Ben Walton
2009-03-21 3:22 ` Jeff King
0 siblings, 1 reply; 14+ messages in thread
From: Ben Walton @ 2009-03-21 2:40 UTC (permalink / raw)
To: git; +Cc: Ben Walton
Take SHELL_PATH into account, if set, in Documentation/Makefile. This
allows the caller to provide a shell capable of running the install
scripts on systems where sh is not bash.
Signed-off-by: Ben Walton <bwalton@artsci.utoronto.ca>
---
Documentation/Makefile | 17 +++++++++++------
1 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/Documentation/Makefile b/Documentation/Makefile
index 144ec32..2eb5a72 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile
@@ -67,6 +67,11 @@ ASCIIDOC_EXTRA += -a docbook-xsl-172
MANPAGE_XSL = manpage-1.72.xsl
endif
+#retain original (but broken) behaviour if SHELL_PATH isn't overridden
+ifndef SHELL_PATH
+ SHELL_PATH = sh
+endif
+
#
# Please note that there is a minor bug in asciidoc.
# The version after 6.0.3 _will_ include the patch found here:
@@ -116,7 +121,7 @@ install-pdf: pdf
$(INSTALL) -m 644 user-manual.pdf $(DESTDIR)$(pdfdir)
install-html: html
- sh ./install-webdoc.sh $(DESTDIR)$(htmldir)
+ $(SHELL_PATH) ./install-webdoc.sh $(DESTDIR)$(htmldir)
../GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
$(MAKE) -C ../ GIT-VERSION-FILE
@@ -178,7 +183,7 @@ user-manual.xml: user-manual.txt user-manual.conf
technical/api-index.txt: technical/api-index-skel.txt \
technical/api-index.sh $(patsubst %,%.txt,$(API_DOCS))
- cd technical && sh ./api-index.sh
+ cd technical && $(SHELL_PATH) ./api-index.sh
$(patsubst %,%.html,$(API_DOCS) technical/api-index): %.html : %.txt
$(ASCIIDOC) -b xhtml11 -f asciidoc.conf \
@@ -220,7 +225,7 @@ $(patsubst %.txt,%.texi,$(MAN_TXT)): %.texi : %.xml
howto-index.txt: howto-index.sh $(wildcard howto/*.txt)
$(RM) $@+ $@
- sh ./howto-index.sh $(wildcard howto/*.txt) >$@+
+ $(SHELL_PATH) ./howto-index.sh $(wildcard howto/*.txt) >$@+
mv $@+ $@
$(patsubst %,%.html,$(ARTICLES)) : %.html : %.txt
@@ -234,14 +239,14 @@ $(patsubst %.txt,%.html,$(wildcard howto/*.txt)): %.html : %.txt
mv $@+ $@
install-webdoc : html
- sh ./install-webdoc.sh $(WEBDOC_DEST)
+ $(SHELL_PATH) ./install-webdoc.sh $(WEBDOC_DEST)
quick-install: quick-install-man
quick-install-man:
- sh ./install-doc-quick.sh $(DOC_REF) $(DESTDIR)$(mandir)
+ $(SHELL_PATH) ./install-doc-quick.sh $(DOC_REF) $(DESTDIR)$(mandir)
quick-install-html:
- sh ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
+ $(SHELL_PATH) ./install-doc-quick.sh $(HTML_REF) $(DESTDIR)$(htmldir)
.PHONY: .FORCE-GIT-VERSION-FILE
--
1.6.0.5
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-21 2:40 Ben Walton
@ 2009-03-21 3:22 ` Jeff King
2009-03-21 11:38 ` Ben Walton
0 siblings, 1 reply; 14+ messages in thread
From: Jeff King @ 2009-03-21 3:22 UTC (permalink / raw)
To: Ben Walton; +Cc: git
On Fri, Mar 20, 2009 at 10:40:20PM -0400, Ben Walton wrote:
> Take SHELL_PATH into account, if set, in Documentation/Makefile. This
> allows the caller to provide a shell capable of running the install
> scripts on systems where sh is not bash.
Makes sense.
> +#retain original (but broken) behaviour if SHELL_PATH isn't overridden
> +ifndef SHELL_PATH
> + SHELL_PATH = sh
> +endif
The Makefile in t/Makefile does:
SHELL_PATH ?= $(SHELL)
which I think makes more sense (and yes, yours actually keeps the
existing behavior, but it's probably better to inherit from SHELL in
case it is set to something more useful).
> install-html: html
> - sh ./install-webdoc.sh $(DESTDIR)$(htmldir)
> + $(SHELL_PATH) ./install-webdoc.sh $(DESTDIR)$(htmldir)
You need a SHELL_PATH_SQ to handle paths with spaces; see t/Makefile for
an example.
I wonder if both subdirs should simply be pulling from
GIT-BUILD-OPTIONS, though, which would allow this to use the specified
SHELL_PATH:
$ make SHELL_PATH=whatever
$ cd Documentation && make
but maybe it is not worth caring about (since it may complicate building
Documentation if you _haven't_ build the actual code).
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-21 3:22 ` Jeff King
@ 2009-03-21 11:38 ` Ben Walton
2009-03-22 6:19 ` Jeff King
0 siblings, 1 reply; 14+ messages in thread
From: Ben Walton @ 2009-03-21 11:38 UTC (permalink / raw)
To: Jeff King; +Cc: GIT List
[-- Attachment #1: Type: text/plain, Size: 2007 bytes --]
Excerpts from Jeff King's message of Fri Mar 20 23:22:40 -0400 2009:
> > +#retain original (but broken) behaviour if SHELL_PATH isn't overridden
> > +ifndef SHELL_PATH
> > + SHELL_PATH = sh
> > +endif
>
> The Makefile in t/Makefile does:
>
> SHELL_PATH ?= $(SHELL)
>
> which I think makes more sense (and yes, yours actually keeps the
> existing behavior, but it's probably better to inherit from SHELL in
> case it is set to something more useful).
I used the ifndef/endif setup becuase that's how the PERL_PATH was set
and also becuase I think it's slightly more explicit. I'm ok with ?=
though too. I had considered using $(SHELL), but discarded it because
it veered from the current behaviour. I agree that $(SHELL) is likely
better than sh though.
> > install-html: html
> > - sh ./install-webdoc.sh $(DESTDIR)$(htmldir)
> > + $(SHELL_PATH) ./install-webdoc.sh $(DESTDIR)$(htmldir)
>
> You need a SHELL_PATH_SQ to handle paths with spaces; see t/Makefile for
> an example.
Ok. I'll look at this and implement the _SQ bits too to make it safer
and more general.
> I wonder if both subdirs should simply be pulling from
> GIT-BUILD-OPTIONS, though, which would allow this to use the specified
> SHELL_PATH:
>
> $ make SHELL_PATH=whatever
> $ cd Documentation && make
>
> but maybe it is not worth caring about (since it may complicate building
> Documentation if you _haven't_ build the actual code).
In my case, I'm using the configure script and then running make,
which sees the Documentation/Makefile source in the ../config.mak
files, so there may be some variance between pure make and make +
autoconf in this respect. I hadn't looked at it in that light.
Should this be reconciled too?
I'm away today, but will try to correct this patch when I get home.
Thanks
-Ben
--
Ben Walton
Systems Programmer - CHASS
University of Toronto
C:416.407.5610 | W:416.978.4302
GPG Key Id: 8E89F6D2; Key Server: pgp.mit.edu
Contact me to arrange for a CAcert assurance meeting.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting
2009-03-21 11:38 ` Ben Walton
@ 2009-03-22 6:19 ` Jeff King
0 siblings, 0 replies; 14+ messages in thread
From: Jeff King @ 2009-03-22 6:19 UTC (permalink / raw)
To: Ben Walton; +Cc: GIT List
On Sat, Mar 21, 2009 at 07:38:35AM -0400, Ben Walton wrote:
> I used the ifndef/endif setup becuase that's how the PERL_PATH was set
> and also becuase I think it's slightly more explicit. I'm ok with ?=
I can't think of any reason why the two would not be equivalent
functionally. I would generally use ?= because it is more portable, but
we are inextricably bound to gmake at this point, so I don't think that
matters. So I don't have a strong preference.
> > but maybe it is not worth caring about (since it may complicate building
> > Documentation if you _haven't_ build the actual code).
>
> In my case, I'm using the configure script and then running make,
> which sees the Documentation/Makefile source in the ../config.mak
> files, so there may be some variance between pure make and make +
> autoconf in this respect. I hadn't looked at it in that light.
> Should this be reconciled too?
Oh, right, I forgot that it pulls in config.mak. So it is really a
non-issue if you are putting SHELL_PATH in your config.mak (or defining
it via autoconf). So nevermind my ramblings in that direction.
I think it should be fine to just resend your patch with:
1. default to $(SHELL)
2. quote $(SHELL_PATH) as appropriate
-Peff
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2009-04-13 14:22 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-03-22 13:20 [PATCH] documentation: Makefile accounts for SHELL_PATH setting Ben Walton
2009-03-23 6:57 ` Jeff King
2009-03-23 12:57 ` Ben Walton
2009-03-23 14:03 ` Jeff King
2009-03-23 14:12 ` Mike Hommey
2009-04-10 0:34 ` Nanako Shiraishi
2009-04-11 20:42 ` Junio C Hamano
2009-04-12 1:49 ` Ben Walton
2009-04-12 8:28 ` Junio C Hamano
2009-04-13 14:21 ` Ben Walton
-- strict thread matches above, loose matches on Subject: below --
2009-03-21 2:40 Ben Walton
2009-03-21 3:22 ` Jeff King
2009-03-21 11:38 ` Ben Walton
2009-03-22 6:19 ` Jeff King
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).