Re: [PATCH 12/13] Makefile: teach scripts to include make variables

From: Jeff King <peff@peff.net>
Date: Feb 5, 2014, 19:50 UTC
Message-ID: <20140205195052.GA16634@sigill.intra.peff.net>
In-Reply-To: <xmqqa9e54ayl.fsf@gitster.dls.corp.google.com>

On Wed, Feb 05, 2014 at 11:26:58AM -0800, Junio C Hamano wrote:

Show 27 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> >  define cmd_munge_script
> >  $(RM) $@ $@+ && \
> > +{ \
> > +includes="$(filter MAKE/%.sh,$^)"; \
> > +if ! test -z "$$includes"; then \
> > +	cat $$includes; \
> > +fi && \
> >  sed -e '1s|#!.*/sh|#!$(call sqi,$(SHELL_PATH))|' \
> >      -e 's|@SHELL_PATH@|$(call sqi,$(SHELL_PATH))|' \
> > -    -e 's|@@DIFF@@|$(call sqi,$(DIFF))|' \
> >      -e 's|@@LOCALEDIR@@|$(call sqi,$(localedir))|g' \
> >      -e 's/@@NO_CURL@@/$(NO_CURL)/g' \
> >      -e 's/@@USE_GETTEXT_SCHEME@@/$(USE_GETTEXT_SCHEME)/g' \
> >      -e $(BROKEN_PATH_FIX) \
> >      -e 's|@@GITWEBDIR@@|$(call sqi,$(gitwebdir))|g' \
> >      -e 's|@@PERL@@|$(call sqi,$(PERL_PATH))|g' \
> > -    $@.sh >$@+
> > +    $@.sh; \
> > +} >$@+
> >  endef
> 
> Sorry, but I am not quite sure what is going on here.
> [...]
> And then after emitting that piece, we start processing the *.sh
> source file, replacing she-bang line?

Yes, there is a bug here. The intent was to end up with:

  #!/bin/sh
  [include snippets]
  [the actual script]

but of course I bungled that, because #!/bin/sh is part of the file already, and our snippets should not come before. If we take this technique to its logical conclusion, the sed replacement goes away entirely, and we end up with something like:

  %: %.sh MAKE/SHELL_SHEBANG.sh
          { cat $(filter MAKE/%.sh,$^) && sed 1d $<; } >$@+
          chmod +x $@+
          mv $@+ $@

  MAKE/SHELL_SHEBANG.sh: MAKE/SHELL_SHEBANG
          echo "#!$(head -1 $<)" >$@+ &&
          mv $@+ $@

I.e., the shebang line simply becomes the first snippet.

Show 13 quoted lines

> >  create_virtual_base() {
> >  	sz0=$(wc -c <"$1")
> > -	@@DIFF@@ -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> > +	$MAKE_DIFF -u -La/"$1" -Lb/"$1" "$1" "$2" | git apply --no-add
> >  	sz1=$(wc -c <"$1")
> 
> This would mean that after this mechanism is extensively employed
> throughout our codebase, any random environment variable the user
> has whose name happens to begin with "MAKE_" will interfere with us
> (rather, we will override such a variable while we run).  Having to
> carve out our own namespace in such a way is OK, but we would want
> to see that namespace somewhat related to the name of our project,
> not to the name of somebody else's like "make", no?

Yes, it probably makes sense to carve out our own namespace. I kind of dislike the use of environment variables at all, though, just because it really bleeds through into how you write the script (as opposed to just replacing like we do now, or in C, where a snippet defining a preprocessor macro is just fine).

An alternative is that we could do something like:

  %: %.sh
          script/mkshellscript $^ >$@+ &&
          chmod +x $@+ &&
          mv $@+ $@

and then have mkshellscript look like (and this could obviously be inline in the Makefile, but I think it's worth pulling it out to avoid the quoting hell):

  #!/bin/sh

  main=$1; shift

  sed_quote() {
          sed 's/\\/\\\\/g; s/|/\\|/g'
  }

  # generate a sed expression that will replace tokens; if we are given
  # the file MAKE/FOO.sh, the expression will replace  @@FOO@@ with the
  # contents of that file
  sed_replace() {
          for var in "$@"; do
                  key=${i#MAKE/}
                  key=${key%.sh}
                  printf 's|@@%s@@|%s|g' "$key" "$(sed_quote <$var)"
          done
  }

  sed "$(sed_replace "$@")" <"$main"

-Peff