Re: [PATCH 12/13] Makefile: teach scripts to include make variables
On Sat, Feb 08, 2014 at 10:47:16PM +0100, Thomas Rast wrote: > Jeff King writes: > > > The current scheme for getting build-time variables into a > > shell script is to munge the script with sed, and stick the > > munged variable into a special sentinel file so that "make" > > knows about the dependency. > > > > Instead, we can combine both functions by generating a shell > > snippet with our value, and then "building" shell scripts by > > concatenating their snippets. "make" then handles the > > dependency automatically, and it's easy to generate tighter > > dependencies. > > > > We demonstrate here by moving the "DIFF" substitution into > > its own snippet, which lets us rebuild only the single > > affected file when it changes. > > I can't look right now *why* this happens, but this breaks > ./t2300-cd-to-toplevel.sh --valgrind with messages like I think it's the bug that Junio already pointed out; git-sh-setup gets the DIFF=... snippet instead of the initial #!-line. I didn't look at the details, but that probably screws up valgrind's symlinking, since we no longer realize it's a shell script. Once that bug is fixed, I'll double-check that the problem goes away. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/13] Makefile: teach scripts to include make variables
Jeff King writes: > The current scheme for getting build-time variables into a > shell script is to munge the script with sed, and stick the > munged variable into a special sentinel file so that "make" > knows about the dependency. > > Instead, we can combine both functions by generating a shell > snippet with our value, and then "building" shell scripts by > concatenating their snippets. "make" then handles the > dependency automatically, and it's easy to generate tighter > dependencies. > > We demonstrate here by moving the "DIFF" substitution into > its own snippet, which lets us rebuild only the single > affected file when it changes. I can't look right now *why* this happens, but this breaks ./t2300-cd-to-toplevel.sh --valgrind with messages like expecting success: ( cd 'repo' && . "$(git --exec-path)"/git-sh-setup && cd_to_toplevel && [ "$(pwd -P)" = "$TOPLEVEL" ] ) ./test-lib.sh: line 414: /home/thomas/g/t/valgrind/bin/git-sh-setup: No such file or directory not ok 1 - at physical root # # ( # cd 'repo' && # . "$(git --exec-path)"/git-sh-setup && # cd_to_toplevel && # [ "$(pwd -P)" = "$TOPLEVEL" ] # ) # I don't know why it only affects this test, or why it doesn't break when within 'git bisect run' -- probably there's something funky going on in the environment, quite possibly in my own configs. -- Thomas Rast t...@thomasrast.ch -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/13] Makefile: teach scripts to include make variables
On Wed, Feb 05, 2014 at 11:26:58AM -0800, Junio C Hamano wrote:
> Jeff King 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.
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 12/13] Makefile: teach scripts to include make variables
Jeff King 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.
- if $includes does not exist, cat $includes will barf but that is
OK;
- if $includes is empty, there is no point running cat so that is
OK;
- if $includes is not empty, we want to cat.
And then after emitting that piece, we start processing the *.sh
source file, replacing she-bang line?
> diff --git a/git-sh-setup.sh b/git-sh-setup.sh
> index fffa3c7..627d289 100644
> --- a/git-sh-setup.sh
> +++ b/git-sh-setup.sh
> @@ -285,7 +285,7 @@ clear_local_git_env() {
> # remove lines from $1 that are not in $2, leaving only common 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?
>
> # If we do not have enough common material, it is not
> diff --git a/script/mksh b/script/mksh
> new file mode 100644
> index 000..d41e77a
> --- /dev/null
> +++ b/script/mksh
> @@ -0,0 +1,4 @@
> +#!/bin/sh
> +
> +name=$1; shift
> +printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 12/13] Makefile: teach scripts to include make variables
The current scheme for getting build-time variables into a
shell script is to munge the script with sed, and stick the
munged variable into a special sentinel file so that "make"
knows about the dependency.
Instead, we can combine both functions by generating a shell
snippet with our value, and then "building" shell scripts by
concatenating their snippets. "make" then handles the
dependency automatically, and it's easy to generate tighter
dependencies.
We demonstrate here by moving the "DIFF" substitution into
its own snippet, which lets us rebuild only the single
affected file when it changes.
Signed-off-by: Jeff King
---
The astute reader will notice that:
MAKE_DIFF='diff'
$MAKE_DIFF ...
that we end up with is not _quite_ the same as replacing "$MAKE_DIFF" in
the actual script text with "diff" at build-time. In particular, the way
that whitespace and quotes are treated is different. I'm not sure what
we would want to do here. Calling "eval" would work. Or we could have
the snippet produce a function, rather than a variable, like:
DIFF() {
diff "$@"
}
Makefile| 18 +++---
git-sh-setup.sh | 2 +-
script/mksh | 4
3 files changed, 20 insertions(+), 4 deletions(-)
create mode 100644 script/mksh
diff --git a/Makefile b/Makefile
index 203171d..ad3100d 100644
--- a/Makefile
+++ b/Makefile
@@ -1598,6 +1598,11 @@ MAKE/%-string.h: MAKE/% script/mkcstring
$(subst -,_,$*) <$< >$@+ && \
mv $@+ $@
+MAKE/%.sh: MAKE/% script/mksh
+ $(QUIET_GEN)$(SHELL_PATH) script/mksh \
+ $(subst -,_,$*) <$< >$@+ && \
+ mv $@+ $@
+
LIBS = $(GITLIBS) $(EXTLIBS)
BASIC_CFLAGS += -DSHA1_HEADER=$(call sq,$(SHA1_HEADER)) \
@@ -1734,7 +1739,6 @@ common-cmds.h: $(wildcard Documentation/git-*.txt)
$(eval $(call make-var,SCRIPT-DEFINES,script parameters,\
:$(SHELL_PATH)\
- :$(DIFF)\
:$(GIT_VERSION)\
:$(localedir)\
:$(NO_CURL)\
@@ -1743,18 +1747,24 @@ $(eval $(call make-var,SCRIPT-DEFINES,script
parameters,\
:$(gitwebdir)\
:$(PERL_PATH)\
))
+$(eval $(call make-var,DIFF,diff command,$(DIFF)))
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
$(patsubst %.sh,%,$(SCRIPT_SH)) : % : %.sh MAKE/SCRIPT-DEFINES
@@ -1766,6 +1776,8 @@ $(SCRIPT_LIB) : % : %.sh MAKE/SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
+git-sh-setup: MAKE/DIFF.sh
+
git.res: git.rc GIT-VERSION-FILE
$(QUIET_RC)$(RC) \
$(join -DMAJOR= -DMINOR=, $(wordlist 1,2,$(subst -, ,$(subst .,
,$(GIT_VERSION) \
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index fffa3c7..627d289 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -285,7 +285,7 @@ clear_local_git_env() {
# remove lines from $1 that are not in $2, leaving only common 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")
# If we do not have enough common material, it is not
diff --git a/script/mksh b/script/mksh
new file mode 100644
index 000..d41e77a
--- /dev/null
+++ b/script/mksh
@@ -0,0 +1,4 @@
+#!/bin/sh
+
+name=$1; shift
+printf "MAKE_%s='%s'\n" "$name" "$(sed "s/'/'\\''/g")"
--
1.8.5.2.500.g8060133
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
