[PATCH] Makefile: Remove excess backslashes from sed

[PATCH] Makefile: Remove excess backslashes from sed

[Brian Gernhardt]

The sed script that was intended to add lines altering the sys.path
had extra backslashes in them.  Instead resulting in

  import sys;  import os;  sys.path.insert( ... )

It output

  import sys; \ import os; \ sys.path.insert( ... )

Unfortunately this caused python (2.6.1 on OS X 10.6.3) to error

  SyntaxError: unexpected character after line continuation character

Removing two of the backslashes solves this problem.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 I'm not sure if this is a result of OS X's python or sed, but t5800
 won't work for me without this change.

 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f0fe351..b9eb1ca 100644
--- a/Makefile
+++ b/Makefile
@@ -1632,8 +1632,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	sed -e '1{' \
 	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
+	    -e 's|^import sys.*|&; \
+	           import os; \
 	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
 	                                        "@@INSTLIBDIR@@"));|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
-- 
1.7.1.rc0.210.ge6da

Re: [PATCH] Makefile: Remove excess backslashes from sed

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> The sed script that was intended to add lines altering the sys.path
> had extra backslashes in them.  Instead resulting in
>
>   import sys;  import os;  sys.path.insert( ... )
>
> It output
>
>   import sys; \ import os; \ sys.path.insert( ... )
>
> Unfortunately this caused python (2.6.1 on OS X 10.6.3) to error
>
>   SyntaxError: unexpected character after line continuation character
>
> Removing two of the backslashes solves this problem.

Traditionally, multi-line sed statements written in the Makefile were
portability nightmare, as the line folding rules (especially how the
backslash is treated in the output) in make implementations were subtly
different, and implementations of sed also got multi-line statement often
wrong.  These days, things might have gotten much better, but in olden
days (back when BSD vs SysV war was still raging), the trick to write
things like this portably was to invoke a shell script that has multi-line
sed statement from the Makefile.  It was painful.

I wonder if we can make this a lot simpler to avoid multi-line sed script.
For example, we could write the source Python script to always begin with:

	#!/usr/bin/python
        import sys;
        import os;
        sys.path.insert(0, os.getenv("GITPYTHONLIB","."));

and then do something like this in the Makefile:

	INSTLIBDIR=... && \
	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
        -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"'"$$INSTLIBDIR"'")|' \
        <$@.py >$@+
	mv $@+ $@

Contributors can then run things in-place while developing the scripts,
perhaps setting GITPYTHONLIB to the source area.

Re: [PATCH] Makefile: Remove excess backslashes from sed

[David Aguilar]

On Thu, Apr 08, 2010 at 11:22:10PM -0400, Brian Gernhardt wrote:
> The sed script that was intended to add lines altering the sys.path
> had extra backslashes in them.  Instead resulting [snip]
>   import sys; \ import os; \ sys.path.insert( ... )

On linux sed I get (or I was dreaming when I tried it):

import sys; \
	import os; \
	sys.path.insert(0, os.getenv(foo,
	                             bar))

We should instead smash it all into one line instead to
avoid differences in sed behavior:

import sys; import os; sys.path.insert(0, os.getenv(foo,
                                                    bar))

If that's what it is, then your patch would produce this on
linux:

import sys;
	import os;
	sys.path.insert(foo,
	                bar)

...which is invalid syntax.

So let's smash it all onto one line instead.  That way I don't
have to think about or test whether there's any difference in
how sed handles newlines.  It'll be the Obviously Correct(tm)
sol'n.  Or maybe I was dreaming?

If you don't beat me to it by the tomorrow night I'll try and
throw together a patch.  Thanks for catching this.

> diff --git a/Makefile b/Makefile
> index f0fe351..b9eb1ca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1632,8 +1632,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
>  	sed -e '1{' \
>  	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
>  	    -e '}' \
> -	    -e 's|^import sys.*|&; \\\
> -	           import os; \\\
> +	    -e 's|^import sys.*|&; \
> +	           import os; \
>  	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
>  	                                        "@@INSTLIBDIR@@"));|' \
-- 

	David

Re: [PATCH] Makefile: Remove excess backslashes from sed

[David Aguilar]

On Thu, Apr 08, 2010 at 10:57:56PM -0700, Junio C Hamano wrote:
> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
> 
> > The sed script that was intended to add lines altering the sys.path
> > had extra backslashes in them.  Instead resulting in
> >
> >   import sys;  import os;  sys.path.insert( ... )
> >
> > It output
> >
> >   import sys; \ import os; \ sys.path.insert( ... )
> >
> > Unfortunately this caused python (2.6.1 on OS X 10.6.3) to error
> >
> >   SyntaxError: unexpected character after line continuation character
> >
> > Removing two of the backslashes solves this problem.
> 
> Traditionally, multi-line sed statements written in the Makefile were
> portability nightmare, as the line folding rules (especially how the
> backslash is treated in the output) in make implementations were subtly
> different, and implementations of sed also got multi-line statement often
> wrong.  These days, things might have gotten much better, but in olden
> days (back when BSD vs SysV war was still raging), the trick to write
> things like this portably was to invoke a shell script that has multi-line
> sed statement from the Makefile.  It was painful.
> 
> I wonder if we can make this a lot simpler to avoid multi-line sed script.
> For example, we could write the source Python script to always begin with:
> 
> 	#!/usr/bin/python
>         import sys;
>         import os;
>         sys.path.insert(0, os.getenv("GITPYTHONLIB","."));
> 
> and then do something like this in the Makefile:
> 
> 	INSTLIBDIR=... && \
> 	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
>         -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"'"$$INSTLIBDIR"'")|' \
>         <$@.py >$@+
> 	mv $@+ $@
> 
> Contributors can then run things in-place while developing the scripts,
> perhaps setting GITPYTHONLIB to the source area.

Here's what I cooked up so far...:

$(patsubst %.py,%,$(SCRIPT_PYTHON)): GIT-CFLAGS
$(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
	$(QUIET_GEN)$(RM) $@ $@+ && \
	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
		--no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
		instlibdir` && \
	sed -e '1{' \
	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)\n@@STARTUP@@\n|' \
	    -e '}' \
	    -e 's|@@STARTUP@@|import os, sys; sys.path.insert(0, @@ENV@@)|' \
	    -e 's|@@ENV@@|os.getenv("GITPYTHONLIB", "@@INSTLIBDIR@@")|' \
	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
	    $@.py >$@+ && \
	chmod +x $@+ && \
	mv $@+ $@

It avoids the multi-line portability concerns by substituting variables
along each step.

This is also nicer in that we don't need to start scripts with
"import sys" or os.getenv() anything...  all we require is a
shebang line.  We inject @@STARTUP@@ when we rewrite the
shebang.

That seems nicer then the old expression which searched for
"import sys.*" when doing the replacements.

Is the \n@@STARTUP\n thing portable?

-- 
		David

[PATCH] Makefile: Avoid multi-line sed when building python scripts

[David Aguilar]

Instead of requiring scripts to start with "import sys",
install the sys.path initialization code immediately
after each script's new shebang line.

This works around portability issues with multi-line sed and
simplifies the work for contributors.  There is no longer
the requirement that every script must "import sys" before
importing git-provided modules.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Makefile |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/Makefile b/Makefile
index 956e781..b763947 100644
--- a/Makefile
+++ b/Makefile
@@ -1608,12 +1608,10 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 		--no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
 		instlibdir` && \
 	sed -e '1{' \
-	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)\n@@STARTUP@@|' \
 	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
-	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
-	                                        "@@INSTLIBDIR@@"));|' \
+	    -e 's|@@STARTUP@@|import os, sys; sys.path.insert(0, @@ENV@@)|' \
+	    -e 's|@@ENV@@|os.getenv("GITPYTHONLIB", "@@INSTLIBDIR@@")|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    $@.py >$@+ && \
 	chmod +x $@+ && \
-- 
1.7.0.3.313.g87b3c

Re: [PATCH] Makefile: Remove excess backslashes from sed

[Brian Gernhardt]

On Apr 9, 2010, at 4:02 AM, David Aguilar wrote:

> Is the \n@@STARTUP\n thing portable?

No.  OS X's sed renders it as 

  #!/usr/bin/pythonnimport os, sys; sys.path.insert(0, os.getenv("GITPYTHONLIB", "/usr/local/lib/python2.6/site-packages"))

So NAK for your patch.

~~ Brian

[PATCH] Makefile: Remove excess backslashes from sed

[Brian Gernhardt]

The sed script that was intended to add lines altering the sys.path
had extra backslashes in them.  Instead resulting in

  import sys;  import os;  sys.path.insert( ... )

It output

  import sys; \ import os; \ sys.path.insert( ... )

Unfortunately this caused python (2.6.1 on OS X 10.6.3) to error

  SyntaxError: unexpected character after line continuation character

Removing two of the backslashes solves this problem.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Makefile |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/Makefile b/Makefile
index f0fe351..b9eb1ca 100644
--- a/Makefile
+++ b/Makefile
@@ -1632,8 +1632,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	sed -e '1{' \
 	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
 	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
+	    -e 's|^import sys.*|&; \
+	           import os; \
 	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
 	                                        "@@INSTLIBDIR@@"));|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
-- 
1.7.1.rc0.243.g2ce66

Re: [PATCH] Makefile: Remove excess backslashes from sed

[Brian Gernhardt]

This is the same patch as before.  I forgot --dry-run while testing git-send-email.  My apologies.

On Apr 9, 2010, at 10:41 AM, Brian Gernhardt wrote:

> The sed script that was intended to add lines altering the sys.path
> had extra backslashes in them.  Instead resulting in
> 
>  import sys;  import os;  sys.path.insert( ... )
> 
> It output
> 
>  import sys; \ import os; \ sys.path.insert( ... )
> 
> Unfortunately this caused python (2.6.1 on OS X 10.6.3) to error
> 
>  SyntaxError: unexpected character after line continuation character
> 
> Removing two of the backslashes solves this problem.
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
> Makefile |    4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index f0fe351..b9eb1ca 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1632,8 +1632,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
> 	sed -e '1{' \
> 	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
> 	    -e '}' \
> -	    -e 's|^import sys.*|&; \\\
> -	           import os; \\\
> +	    -e 's|^import sys.*|&; \
> +	           import os; \
> 	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
> 	                                        "@@INSTLIBDIR@@"));|' \
> 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
> -- 
> 1.7.1.rc0.243.g2ce66
> 
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Makefile: Remove excess backslashes from sed

[Sverre Rabbelier]

Heya,

On Fri, Apr 9, 2010 at 07:57, Junio C Hamano <gitster@pobox.com> wrote:
> I wonder if we can make this a lot simpler to avoid multi-line sed script.
> For example, we could write the source Python script to always begin with:
>
>        #!/usr/bin/python
>        import sys;
>        import os;
>        sys.path.insert(0, os.getenv("GITPYTHONLIB","."));

Thank you, I like this a lot better than our current approach. I think
it's very ugly that git (needs to) mangle(s) the python files when
installing, since that means that line-numbers don't match in
stacktraces. I'd definitely prefer using this.

-- 
Cheers,

Sverre Rabbelier

[PATCH] Makefile: Simplify handling of python scripts

[Brian Gernhardt]

The sed script intended to add a standard opening to python scripts
was non-compatible and overly complex.  Simplifying it down to a set
of one-liners removes the compatibility issues of newlines.  Moving
the environment alterations from the Makefile to the python scripts
makes also makes the scripts easier to run in-place.

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Makefile              |    9 ++-------
 git-remote-testgit.py |    2 ++
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/Makefile b/Makefile
index f0fe351..87c90d6 100644
--- a/Makefile
+++ b/Makefile
@@ -1629,13 +1629,8 @@ $(patsubst %.py,%,$(SCRIPT_PYTHON)): % : %.py
 	INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C git_remote_helpers -s \
 		--no-print-directory prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' \
 		instlibdir` && \
-	sed -e '1{' \
-	    -e '	s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
-	    -e '}' \
-	    -e 's|^import sys.*|&; \\\
-	           import os; \\\
-	           sys.path.insert(0, os.getenv("GITPYTHONLIB",\
-	                                        "@@INSTLIBDIR@@"));|' \
+	sed -e '1s|#!.*python|#!$(PYTHON_PATH_SQ)|' \
+	    -e 's|\(os\.getenv("GITPYTHONLIB"\)[^)]*)|\1,"@@INSTLIBDIR@@")|' \
 	    -e 's|@@INSTLIBDIR@@|'"$$INSTLIBDIR"'|g' \
 	    $@.py >$@+ && \
 	chmod +x $@+ && \
diff --git a/git-remote-testgit.py b/git-remote-testgit.py
index f61624e..9253922 100644
--- a/git-remote-testgit.py
+++ b/git-remote-testgit.py
@@ -2,6 +2,8 @@
 
 import hashlib
 import sys
+import os
+sys.path.insert(0, os.getenv("GITPYTHONLIB","."))
 
 from git_remote_helpers.util import die, debug, warn
 from git_remote_helpers.git.repo import GitRepo
-- 
1.7.1.rc0.243.g2ce66