threads / patch / 18443

patchdocumentation: Makefile accounts for SHELL_PATH setting

Subject: [PATCH] documentation: Makefile accounts for SHELL_PATH setting

## tl;dr

4 messages between Mar 21, 2009 and Mar 22, 2009. Diffs are folded; open one to read it.

replies: 3people: 2as markdown or json

Ben Walton· Mar 21, 2009, 02:40 UTC · lore

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(-)
Show changes to Documentation/Makefile +11 −6
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
Jeff King· Mar 21, 2009, 03:22 UTC · re: Ben Walton · lore

Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting

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
Ben Walton· Mar 21, 2009, 11:38 UTC · re: Jeff King · lore

Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting

Excerpts from Jeff King's message of Fri Mar 20 23:22:40 -0400 2009:
Show 12 quoted lines
> > +#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.

Show 6 quoted lines
> >  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.

Show 9 quoted lines
> 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.
Jeff King· Mar 22, 2009, 06:19 UTC · re: Ben Walton · lore

Re: [PATCH] documentation: Makefile accounts for SHELL_PATH setting

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.

Show 8 quoted lines
> > 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

← back to recent threads