<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">El jue., 4 abr. 2019 a las 0:41, Mads Kiilerich (<<a href="mailto:mads@kiilerich.com">mads@kiilerich.com</a>>) escribió:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On 4/3/19 10:28 PM, Thomas De Schampheleire wrote:<br>
> # HG changeset patch<br>
> # User Thomas De Schampheleire <<a href="mailto:thomas.de_schampheleire@nokia.com" target="_blank">thomas.de_schampheleire@nokia.com</a>><br>
> # Date 1554323208 -7200<br>
> # Wed Apr 03 22:26:48 2019 +0200<br>
> # Branch stable<br>
> # Node ID 0d91f710606b72027456a1183794ee38f26fe741<br>
> # Parent 06b511848fb1a3ead56de583e19df5f317bdf7e0<br>
> hooks: fix potentially invalid interpreter in git hooks (Issue #333)<br>
<br>
<br>
Thanks for iterating on this.<br>
<br>
<br>
> Commit 5e501b6ee639 introduced the use of 'sys.executable' as interpreter<br>
> for git hooks instead of 'python2' with the following argument:<br>
><br>
> "Windows doesn't necessarily have "python2" available in $PATH, but we<br>
> still want to make sure we don't end up invoking a python3. Using the<br>
> absolute path seems more safe."<br>
><br>
> But, sys.executable does not necessarily point to Python. When Kallithea is<br>
> started under uWSGI, sys.executable points to the uwsgi executable. As a<br>
> result, the interpreter encoded in the git hooks on the server repositories<br>
> would be:<br>
><br>
> #!/usr/bin/env /path/to/uwsgi<br>
><br>
> And pushing to such repo would result in following client errors:<br>
><br>
> $ git push<br>
> Password for 'http://user@localhost:5050':<br>
> Enumerating objects: 3, done.<br>
> Counting objects: 100% (3/3), done.<br>
> Writing objects: 100% (3/3), 241 bytes | 241.00 KiB/s, done.<br>
> Total 3 (delta 0), reused 0 (delta 0)<br>
> remote: unable to load configuration from hooks/pre-receive<br>
> To <a href="http://localhost:5050/gitrepo-new" rel="noreferrer" target="_blank">http://localhost:5050/gitrepo-new</a><br>
> ! [remote rejected] master -> master (pre-receive hook declined)<br>
> error: failed to push some refs to 'http://user@localhost:5050/gitrepo-new'<br>
<br>
<br>
Can we somehow make it show a more useful error message in this case? I <br>
guess not - none of our code is executed ...<br></blockquote><div><br></div><div>Actually, there is some of our code executed: SimpleGit. But, at that point you don't know that the git hook call is going to fail.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> The approach taken by this commit is:<br>
<br>
<br>
FWIW, this:<br>
<br>
> - Introduce a new configuration setting: hook_python_path, and use its value<br>
> as the Python interpreter to use in git hooks.<br>
> - If the value is absent or empty, fall back to the current logic<br>
> 'sys.executable' or if that is also invalid, 'python2'.<br>
<br>
<br>
and this:<br>
<br>
<br>
> - Let `kallithea-cli config-create` fill in its own interpreter as default<br>
> value, which should be valid as long as the virtual environment is not<br>
> moved/replaced.<br>
<br>
<br>
*could* be separate "milestones" and separate changesets. But no big <br>
deal. Split or keep together as you like.<br></blockquote><div><br></div><div><br></div><div>Yes, I considered it, but got a bit stuck with the comments referring to code that does not yet exist.</div><div>I'll think about it a second time.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> The main downside of this approach is its fragility in case a new virtualenv<br>
> is used. Previously, the configuration file was the same regardless of<br>
> virtualenv, but this is no longer true after this patch.<br>
<br>
<br>
But also, it is crucial that the *right* virtualenv is used, also if Git <br>
should be invoked directly, outside any virtual env. It is thus inherent <br>
that the new virtualenv *has* to be written to the hooks somehow. The <br>
previous use of same 'python2' was wrong and fragile. I thus don't think <br>
the new way is more fragile. Explicit expectations and failing early is <br>
*less* fragile ;-)<br></blockquote><div><br></div><div>In fact, my understanding was not fully complete before: when users push to a git repo, they actually communicate with our Git middleware, which itself uses dulwich. All this should normally happen inside the virtualenv (if used). Only when the hooks get called, there is a decoupling into another process. (I am theorizing here, not explicitly checked)<br></div><div> </div><div>So if that is correct, then I wonder if we could not avoid the decoupling.</div><div>We are already handling pull hooks in kallithea.lib.middleware.simplegit._handle_githooks .</div><div>Can we not handle push hooks pre-receive/post-receive in the same place? In this case, we are definitely in the right environment, we are and stay within Python. I guess it will also simplify the situation for Windows.</div><div><br></div><div>Am I overlooking something?<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
> diff --git a/development.ini b/development.ini<br>
> --- a/development.ini<br>
> +++ b/development.ini<br>
> @@ -117,6 +117,16 @@ use_htsts = false<br>
> ## number of commits stats will parse on each iteration<br>
> commit_parse_limit = 25<br>
> <br>
> +## Path to Python executable to be used in git hooks.<br>
> +## When empty, the value of 'sys.executable' at the time of installation<br>
> +## of the git hooks is used, which is correct in many cases but not when<br>
> +## using uwsgi.<br>
> +## When creating a config file via `kallithea-cli config-create`, the<br>
> +## Python executable used for that invocation is filled in below.<br>
> +## If you change this setting, you should reinstall the git hooks via<br>
> +## Admin > Settings > Remap and Rescan.<br>
> +hook_python_path =<br>
> +<br>
> ## path to git executable<br>
> git_path = git<br>
> <br>
> diff --git a/kallithea/lib/inifile.py b/kallithea/lib/inifile.py<br>
> --- a/kallithea/lib/inifile.py<br>
> +++ b/kallithea/lib/inifile.py<br>
> @@ -23,6 +23,7 @@ other custom values.<br>
> import logging<br>
> import re<br>
> import os<br>
> +import sys<br>
> <br>
> import mako.template<br>
> <br>
> @@ -40,6 +41,7 @@ default_variables = {<br>
> 'host': '127.0.0.1',<br>
> 'port': '5000',<br>
> 'uuid': lambda: 'VERY-SECRET',<br>
> + 'python_executable': sys.executable or '',<br>
> }<br>
> <br>
> <br>
> diff --git a/kallithea/lib/paster_commands/template.ini.mako b/kallithea/lib/paster_commands/template.ini.mako<br>
> --- a/kallithea/lib/paster_commands/template.ini.mako<br>
> +++ b/kallithea/lib/paster_commands/template.ini.mako<br>
> @@ -211,6 +211,16 @@ use_htsts = false<br>
> <%text>## number of commits stats will parse on each iteration</%text><br>
> commit_parse_limit = 25<br>
> <br>
> +<%text>## Path to Python executable to be used in git hooks.</%text><br>
> +<%text>## When empty, the value of 'sys.executable' at the time of installation</%text><br>
> +<%text>## of the git hooks is used, which is correct in many cases but not when</%text><br>
> +<%text>## using uwsgi.</%text><br>
<br>
<br>
Perhaps clarify that "used" means "written into the git hook script files".<br>
<br>
<br>
> +<%text>## When creating a config file via `kallithea-cli config-create`, the</%text><br>
> +<%text>## Python executable used for that invocation is filled in below.</%text><br>
> +<%text>## If you change this setting, you should reinstall the git hooks via</%text><br>
> +<%text>## Admin > Settings > Remap and Rescan.</%text><br>
> +hook_python_path = ${python_executable}<br>
<br>
<br>
Perhaps call the variable `python_executable` just like the internal <br>
name. Consistency makes it simpler to work with. If one name is better <br>
than the other, just use it everywhere. And while the current use of <br>
python_executable is to be written into hooks, that "policy" doesn't <br>
have to be "hardcoded" into the name of the setting - mentioning in the <br>
comments should be enough.<br>
<br>
And how about using template conditionals (and examples in comments, like)<br>
<br>
"""<br>
# python_executable = /srv/kallithea/venv/bin/python2<br>
# python_executable = (((a windows example)))<br>
%if python_executable:<br>
python_executable = ${python_executable}<br>
%endif<br>
"""<br>
<br>
Then we don't need the previous change or the " or ''" ... and we avoid <br>
putting trailing space in the .ini ;-)<br>
<br>
<br>
> <%text>## path to git executable</%text><br>
> git_path = git<br>
> <br>
> diff --git a/kallithea/model/scm.py b/kallithea/model/scm.py<br>
> --- a/kallithea/model/scm.py<br>
> +++ b/kallithea/model/scm.py<br>
> @@ -721,8 +721,11 @@ class ScmModel(object):<br>
> return choices, hist_l<br>
> <br>
> def _get_python_executable(self):<br>
> - """Return a Python executable for use in hooks."""<br>
> - return sys.executable or 'python2'<br>
> + """Return a Python executable for use in hooks.<br>
> +<br>
> + This is not simply sys.executable because under uwsgi it will be the<br>
> + uwsgi program itself."""<br>
<br>
<br>
I think it also would be very helpful to summarize how virtualenv should <br>
(and will) be handled.<br>
<br>
Perhaps, if you know mod_wsgi has similar problems, mention that too.<br>
<br>
Windows vs Posix might also deserve mentioning.<br>
<br>
(But also, I don't know how much we want to duplicate information here, <br>
in .ini, in commit message, (and perhaps in documentation if we can't <br>
automate it fully but have to explain).)<br>
<br>
<br>
> + return kallithea.CONFIG.get('hook_python_path', sys.executable) or 'python2'<br>
<br>
<br>
We want sys.executable, also if the .ini file sets it to the empty <br>
string. I guess it thus should be something like<br>
<br>
kallithea.CONFIG.get('python_executable') or sys.executable or 'python2'<br>
<br>
<br>
> def install_git_hooks(self, repo, force_create=False):<br>
> """<br>
> diff --git a/scripts/generate-ini.py b/scripts/generate-ini.py<br>
> --- a/scripts/generate-ini.py<br>
> +++ b/scripts/generate-ini.py<br>
> @@ -48,6 +48,7 @@ ini_files = [<br>
> },<br>
> },<br>
> {<br>
> + 'python_executable': '',<br>
> },<br>
<br>
<br>
(With the template proposed above, we don't need this.)<br></blockquote><div><br></div><div><br></div><div>Thanks for these comments, I generally agree.</div><div><br></div><div>But before going further, I'd like to hear your opinion on the idea of calling the hooks directly from our own code (see above).</div><div><br></div><div>Thanks,</div><div>Thomas<br></div><div><br></div><div> </div></div></div></div>