[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[roy]

This allows different versions of Python to be used rather than
forcing version specific encoding in each script.

Signed-off-by: Roy Marples <roy at marples.name>
---
 filter.c                       | 8 ++++++++
 filters/email-gravatar.py      | 3 ---
 filters/syntax-highlighting.py | 5 +----
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/filter.c b/filter.c
index 949c931..3c0f978 100644
--- a/filter.c
+++ b/filter.c
@@ -15,6 +15,8 @@
 #include <lauxlib.h>
 #endif
 
+static bool filter_env_set;
+
 static inline void reap_filter(struct cgit_filter *filter)
 {
 	if (filter && filter->cleanup)
@@ -44,6 +46,12 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
 	int i;
 
+	if (!filter_env_set) {
+		/* Always input/output utf-8 for a Python filter. */
+		setenv("PYTHONIOENCODING", "utf-8", 1);
+		filter_env_set = true;
+	}
+
 	for (i = 0; i < filter->base.argument_count; i++)
 		filter->argv[i + 1] = va_arg(ap, char *);
 
diff --git a/filters/email-gravatar.py b/filters/email-gravatar.py
index d70440e..8b98471 100755
--- a/filters/email-gravatar.py
+++ b/filters/email-gravatar.py
@@ -30,9 +30,6 @@ if email[-1] == '>':
 
 page = sys.argv[2]
 
-sys.stdin = codecs.getreader("utf-8")(sys.stdin.detach())
-sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())
-
 md5 = hashlib.md5(email.encode()).hexdigest()
 text = sys.stdin.read().strip()
 
diff --git a/filters/syntax-highlighting.py b/filters/syntax-highlighting.py
index 5888b50..936fdb7 100755
--- a/filters/syntax-highlighting.py
+++ b/filters/syntax-highlighting.py
@@ -29,9 +29,6 @@ from pygments.lexers import guess_lexer
 from pygments.lexers import guess_lexer_for_filename
 from pygments.formatters import HtmlFormatter
 
-
-sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8', errors='replace')
-sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8', errors='replace')
 data = sys.stdin.read()
 filename = sys.argv[1]
 formatter = HtmlFormatter(style='pastie')
@@ -52,4 +49,4 @@ except TypeError:
 sys.stdout.write('<style>')
 sys.stdout.write(formatter.get_style_defs('.highlight'))
 sys.stdout.write('</style>')
-sys.stdout.write(highlight(data, lexer, formatter, outfile=None))
+highlight(data, lexer, formatter, outfile=sys.stdout)
-- 
2.11.1

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[john]

On Thu, Feb 23, 2017 at 03:48:23PM +0000, Roy Marples wrote:
> This allows different versions of Python to be used rather than
> forcing version specific encoding in each script.
> 
> Signed-off-by: Roy Marples <roy at marples.name>
> ---
>  filter.c                       | 8 ++++++++
>  filters/email-gravatar.py      | 3 ---
>  filters/syntax-highlighting.py | 5 +----
>  3 files changed, 9 insertions(+), 7 deletions(-)

Neat!  This definitely makes writing it easier to get it right with
Python filters, but having filter_env_set seems unnecessary.

Is there a reason not to either:

1) set PYTHONIOENCODING unconditionally early in startup

or

2) set the environment in the child after forking

?

> diff --git a/filter.c b/filter.c
> index 949c931..3c0f978 100644
> --- a/filter.c
> +++ b/filter.c
> @@ -15,6 +15,8 @@
>  #include <lauxlib.h>
>  #endif
>  
> +static bool filter_env_set;
> +
>  static inline void reap_filter(struct cgit_filter *filter)
>  {
>  	if (filter && filter->cleanup)
> @@ -44,6 +46,12 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
>  	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
>  	int i;
>  
> +	if (!filter_env_set) {
> +		/* Always input/output utf-8 for a Python filter. */
> +		setenv("PYTHONIOENCODING", "utf-8", 1);
> +		filter_env_set = true;
> +	}
> +
>  	for (i = 0; i < filter->base.argument_count; i++)
>  		filter->argv[i + 1] = va_arg(ap, char *);
>  
> diff --git a/filters/email-gravatar.py b/filters/email-gravatar.py
> index d70440e..8b98471 100755
> --- a/filters/email-gravatar.py
> +++ b/filters/email-gravatar.py
> @@ -30,9 +30,6 @@ if email[-1] == '>':
>  
>  page = sys.argv[2]
>  
> -sys.stdin = codecs.getreader("utf-8")(sys.stdin.detach())
> -sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())
> -
>  md5 = hashlib.md5(email.encode()).hexdigest()
>  text = sys.stdin.read().strip()
>  
> diff --git a/filters/syntax-highlighting.py b/filters/syntax-highlighting.py
> index 5888b50..936fdb7 100755
> --- a/filters/syntax-highlighting.py
> +++ b/filters/syntax-highlighting.py
> @@ -29,9 +29,6 @@ from pygments.lexers import guess_lexer
>  from pygments.lexers import guess_lexer_for_filename
>  from pygments.formatters import HtmlFormatter
>  
> -
> -sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8', errors='replace')
> -sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8', errors='replace')
>  data = sys.stdin.read()
>  filename = sys.argv[1]
>  formatter = HtmlFormatter(style='pastie')
> @@ -52,4 +49,4 @@ except TypeError:
>  sys.stdout.write('<style>')
>  sys.stdout.write(formatter.get_style_defs('.highlight'))
>  sys.stdout.write('</style>')
> -sys.stdout.write(highlight(data, lexer, formatter, outfile=None))
> +highlight(data, lexer, formatter, outfile=sys.stdout)
> -- 
> 2.11.1
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[roy]

On 04/03/2017 12:35, John Keeping wrote:
> On Thu, Feb 23, 2017 at 03:48:23PM +0000, Roy Marples wrote:
>> This allows different versions of Python to be used rather than
>> forcing version specific encoding in each script.
>>
>> Signed-off-by: Roy Marples <roy at marples.name>
>> ---
>>  filter.c                       | 8 ++++++++
>>  filters/email-gravatar.py      | 3 ---
>>  filters/syntax-highlighting.py | 5 +----
>>  3 files changed, 9 insertions(+), 7 deletions(-)
> 
> Neat!  This definitely makes writing it easier to get it right with
> Python filters, but having filter_env_set seems unnecessary.
> 
> Is there a reason not to either:
> 
> 1) set PYTHONIOENCODING unconditionally early in startup
> 
> or
> 
> 2) set the environment in the child after forking

No reason.
I went this approach so that it's only set once when needed, happy to
re-base my work with either of the two above options.

Roy

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[roy]

On 06/03/2017 09:14, Roy Marples wrote:
> On 04/03/2017 12:35, John Keeping wrote:
>> On Thu, Feb 23, 2017 at 03:48:23PM +0000, Roy Marples wrote:
>>> This allows different versions of Python to be used rather than
>>> forcing version specific encoding in each script.
>>>
>>> Signed-off-by: Roy Marples <roy at marples.name>
>>> ---
>>>  filter.c                       | 8 ++++++++
>>>  filters/email-gravatar.py      | 3 ---
>>>  filters/syntax-highlighting.py | 5 +----
>>>  3 files changed, 9 insertions(+), 7 deletions(-)
>>
>> Neat!  This definitely makes writing it easier to get it right with
>> Python filters, but having filter_env_set seems unnecessary.
>>
>> Is there a reason not to either:
>>
>> 1) set PYTHONIOENCODING unconditionally early in startup
>>
>> or
>>
>> 2) set the environment in the child after forking
> 
> No reason.
> I went this approach so that it's only set once when needed, happy to
> re-base my work with either of the two above options.

Is there anything more you want me to do here?

Roy

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[john]

On Wed, Mar 08, 2017 at 07:01:59PM +0000, Roy Marples wrote:
> On 06/03/2017 09:14, Roy Marples wrote:
> > On 04/03/2017 12:35, John Keeping wrote:
> >> On Thu, Feb 23, 2017 at 03:48:23PM +0000, Roy Marples wrote:
> >>> This allows different versions of Python to be used rather than
> >>> forcing version specific encoding in each script.
> >>>
> >>> Signed-off-by: Roy Marples <roy at marples.name>
> >>> ---
> >>>  filter.c                       | 8 ++++++++
> >>>  filters/email-gravatar.py      | 3 ---
> >>>  filters/syntax-highlighting.py | 5 +----
> >>>  3 files changed, 9 insertions(+), 7 deletions(-)
> >>
> >> Neat!  This definitely makes writing it easier to get it right with
> >> Python filters, but having filter_env_set seems unnecessary.
> >>
> >> Is there a reason not to either:
> >>
> >> 1) set PYTHONIOENCODING unconditionally early in startup
> >>
> >> or
> >>
> >> 2) set the environment in the child after forking
> > 
> > No reason.
> > I went this approach so that it's only set once when needed, happy to
> > re-base my work with either of the two above options.
> 
> Is there anything more you want me to do here?

I was hoping someone else would jump in with an opinion, but this is
quite a quiet mailing list so it might take a while.

Having thought about it a bit more, my vote is for moving the setenv
into the child.  It keeps it where it is being used and I expect most
requests to hit zero or one exec filter(s) so we won't be calling
setenv() multiple times (and if we do, the cost will be dwarfed by the
cost of executing a new process anyway).

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[Jason]

Hey guys,

Sorry for the delay. I'm currently on the road traveling and won't be
properly back at my desk until the end of next week.

However, my initial reaction is that hard coding various
interpreter-specific environment variables in cgit itself is not
correct, and that this is something better left to the CGI environment
as it sees fit. However, we may benefit from explicit script level
configuration of unicode stuff.

I'll revisit this next week.

Jason

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[john]

On Sun, Mar 12, 2017 at 10:01:10AM -0700, Jason A. Donenfeld wrote:
> Sorry for the delay. I'm currently on the road traveling and won't be
> properly back at my desk until the end of next week.
> 
> However, my initial reaction is that hard coding various
> interpreter-specific environment variables in cgit itself is not
> correct, and that this is something better left to the CGI environment
> as it sees fit. However, we may benefit from explicit script level
> configuration of unicode stuff.

While I'm inclined to agree with this, in this particular case we
explicitly encode pages as UTF-8 so there is an argument that we should
be telling child processes that UTF-8 is the correct encoding.

Maybe we should be looking to change LANG instead, but I'm not sure how
reliably we can do that.  Is it safe to do something like:

	const char *lang = getenv("LANG");
	struct strbuf sb = STRBUF_INIT;

	if (!lang)
		lang = "C";
	strbuf_addf(&sb, "%.*s.UTF-8",
		    (int) (strchrnul(lang, '.') - lang), lang);
	setenv("LANG", sb.buf);

?

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[Jason]

On Sun, Mar 12, 2017 at 6:51 PM, John Keeping <john at keeping.me.uk> wrote:
> While I'm inclined to agree with this, in this particular case we
> explicitly encode pages as UTF-8 so there is an argument that we should
> be telling child processes that UTF-8 is the correct encoding.

That's a compelling argument, actually.

>
> Maybe we should be looking to change LANG instead, but I'm not sure how
> reliably we can do that.

I'm more onboard with that. Does changing LANG influence the PYTHON
variable implicitly?

> Is it safe to do something like:
>
>         const char *lang = getenv("LANG");
>         struct strbuf sb = STRBUF_INIT;
>
>         if (!lang)
>                 lang = "C";
>         strbuf_addf(&sb, "%.*s.UTF-8",
>                     (int) (strchrnul(lang, '.') - lang), lang);
>         setenv("LANG", sb.buf);

That's probably not too bad, though I wonder if we could get away with
just explicitly setting a more generic UTF-8 instead of trying to read
the user's language preferences.

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[john]

On Fri, Mar 17, 2017 at 07:07:02PM +0100, Jason A. Donenfeld wrote:
> On Sun, Mar 12, 2017 at 6:51 PM, John Keeping <john at keeping.me.uk> wrote:
> > While I'm inclined to agree with this, in this particular case we
> > explicitly encode pages as UTF-8 so there is an argument that we should
> > be telling child processes that UTF-8 is the correct encoding.
> 
> That's a compelling argument, actually.
> 
> >
> > Maybe we should be looking to change LANG instead, but I'm not sure how
> > reliably we can do that.
> 
> I'm more onboard with that. Does changing LANG influence the PYTHON
> variable implicitly?

Yes, if there is no explicit encoding requested then Python derives it
from the locale.

However, it only works if the locale actually exists on the system; for
example on my system I get:

$ LANG=en_GB.UTF-8 python2 -c 'import sys; print(sys.stdin.encoding)'
UTF-8
$ LANG=en_GB.ISO-8859-1 python2 -c 'import sys; print(sys.stdin.encoding)' 
ISO-8859-1

but I don't have C.UTF-8, so:

$ LC_ALL=C.UTF-8 python2 -c 'import sys; print(sys.stdin.encoding)'
ANSI_X3.4-1968

There's an open glibc bug [1] to support C.UTF-8 but for now it looks
like it's only available on Debian and derivatives.

> > Is it safe to do something like:
> >
> >         const char *lang = getenv("LANG");
> >         struct strbuf sb = STRBUF_INIT;
> >
> >         if (!lang)
> >                 lang = "C";
> >         strbuf_addf(&sb, "%.*s.UTF-8",
> >                     (int) (strchrnul(lang, '.') - lang), lang);
> >         setenv("LANG", sb.buf);
> 
> That's probably not too bad, though I wonder if we could get away with
> just explicitly setting a more generic UTF-8 instead of trying to read
> the user's language preferences.

Other people have already found that it's not quite that simple [2] if
we want it to work on all systems.

[1] https://sourceware.org/bugzilla/show_bug.cgi?id=17318
[2] https://github.com/commercialhaskell/stack/issues/856

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[roy]

On 17/03/17 20:04, John Keeping wrote:
> Other people have already found that it's not quite that simple [2] if
> we want it to work on all systems.

Patch author wants it to run on NetBSD as that's what I use :)

The default locale for daemons such as CGI processes should always be C
as that saves a lot of headache later.

Roy

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[Jason]

For the time being I'm going to NACK the PYTHONIOENCODING patch until
we have a good solution for modifying LANG. I don't think it's proper
to make a laundry list of interpreter-specific variables; it could get
out of control eventually.

What are our options? Wait for C.UTF-8 to be available everywhere?
Parse LANG to determine the actual language, and then remove the end
and add on ".utf-8", as in John's example?

https://github.com/commercialhaskell/stack/commit/478ca7118372fe9a500430cae36924ae64e147c6
This is pretty extensive here, but it's a bit complicated...

Jason

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[roy]

This allows different versions of Python to be used rather than
forcing version specific encoding in each script.

Signed-off-by: Roy Marples <roy at marples.name>
---
 filter.c                       | 2 ++
 filters/email-gravatar.py      | 3 ---
 filters/syntax-highlighting.py | 5 +----
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/filter.c b/filter.c
index 949c931..84dd0ed 100644
--- a/filter.c
+++ b/filter.c
@@ -55,6 +55,8 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
 		close(filter->pipe_fh[1]);
 		chk_non_negative(dup2(filter->pipe_fh[0], STDIN_FILENO),
 			"Unable to use pipe as STDIN");
+		/* Always input/output utf-8 for a Python filter. */
+		setenv("PYTHONIOENCODING", "utf-8", 1);
 		execvp(filter->cmd, filter->argv);
 		die_errno("Unable to exec subprocess %s", filter->cmd);
 	}
diff --git a/filters/email-gravatar.py b/filters/email-gravatar.py
index d70440e..8b98471 100755
--- a/filters/email-gravatar.py
+++ b/filters/email-gravatar.py
@@ -30,9 +30,6 @@ if email[-1] == '>':
 
 page = sys.argv[2]
 
-sys.stdin = codecs.getreader("utf-8")(sys.stdin.detach())
-sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())
-
 md5 = hashlib.md5(email.encode()).hexdigest()
 text = sys.stdin.read().strip()
 
diff --git a/filters/syntax-highlighting.py b/filters/syntax-highlighting.py
index 5888b50..936fdb7 100755
--- a/filters/syntax-highlighting.py
+++ b/filters/syntax-highlighting.py
@@ -29,9 +29,6 @@ from pygments.lexers import guess_lexer
 from pygments.lexers import guess_lexer_for_filename
 from pygments.formatters import HtmlFormatter
 
-
-sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8', errors='replace')
-sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8', errors='replace')
 data = sys.stdin.read()
 filename = sys.argv[1]
 formatter = HtmlFormatter(style='pastie')
@@ -52,4 +49,4 @@ except TypeError:
 sys.stdout.write('<style>')
 sys.stdout.write(formatter.get_style_defs('.highlight'))
 sys.stdout.write('</style>')
-sys.stdout.write(highlight(data, lexer, formatter, outfile=None))
+highlight(data, lexer, formatter, outfile=sys.stdout)
-- 
2.11.1

[PATCH] filter: set environment variable PYTHONIOENCODING to utf-8

[john]

On Thu, Mar 09, 2017 at 12:18:10AM +0000, Roy Marples wrote:
> This allows different versions of Python to be used rather than
> forcing version specific encoding in each script.
> 
> Signed-off-by: Roy Marples <roy at marples.name>

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  filter.c                       | 2 ++
>  filters/email-gravatar.py      | 3 ---
>  filters/syntax-highlighting.py | 5 +----
>  3 files changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/filter.c b/filter.c
> index 949c931..84dd0ed 100644
> --- a/filter.c
> +++ b/filter.c
> @@ -55,6 +55,8 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
>  		close(filter->pipe_fh[1]);
>  		chk_non_negative(dup2(filter->pipe_fh[0], STDIN_FILENO),
>  			"Unable to use pipe as STDIN");
> +		/* Always input/output utf-8 for a Python filter. */
> +		setenv("PYTHONIOENCODING", "utf-8", 1);
>  		execvp(filter->cmd, filter->argv);
>  		die_errno("Unable to exec subprocess %s", filter->cmd);
>  	}
> diff --git a/filters/email-gravatar.py b/filters/email-gravatar.py
> index d70440e..8b98471 100755
> --- a/filters/email-gravatar.py
> +++ b/filters/email-gravatar.py
> @@ -30,9 +30,6 @@ if email[-1] == '>':
>  
>  page = sys.argv[2]
>  
> -sys.stdin = codecs.getreader("utf-8")(sys.stdin.detach())
> -sys.stdout = codecs.getwriter("utf-8")(sys.stdout.detach())
> -
>  md5 = hashlib.md5(email.encode()).hexdigest()
>  text = sys.stdin.read().strip()
>  
> diff --git a/filters/syntax-highlighting.py b/filters/syntax-highlighting.py
> index 5888b50..936fdb7 100755
> --- a/filters/syntax-highlighting.py
> +++ b/filters/syntax-highlighting.py
> @@ -29,9 +29,6 @@ from pygments.lexers import guess_lexer
>  from pygments.lexers import guess_lexer_for_filename
>  from pygments.formatters import HtmlFormatter
>  
> -
> -sys.stdin = io.TextIOWrapper(sys.stdin.buffer, encoding='utf-8', errors='replace')
> -sys.stdout = io.TextIOWrapper(sys.stdout.buffer, encoding='utf-8', errors='replace')
>  data = sys.stdin.read()
>  filename = sys.argv[1]
>  formatter = HtmlFormatter(style='pastie')
> @@ -52,4 +49,4 @@ except TypeError:
>  sys.stdout.write('<style>')
>  sys.stdout.write(formatter.get_style_defs('.highlight'))
>  sys.stdout.write('</style>')
> -sys.stdout.write(highlight(data, lexer, formatter, outfile=None))
> +highlight(data, lexer, formatter, outfile=sys.stdout)
> -- 
> 2.11.1
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit