Windows console_scripts called from reloader

[androiddrew]

I am running apistar on python 3 and encountering an issue that has lead me to the werkzeug reloader.

(fresh) C:\Users\uskaxb07\PycharmProjects\testapi>apistar run
Starting up...
 * Restarting with stat
  File "C:\Users\uskaxb07\env\fresh\Scripts\apistar.exe", line 1
SyntaxError: Non-UTF-8 code starting with '\x90' in file C:\Users\uskaxb07\env\fresh\Scripts\apistar.exe on line 1, but no encoding declared; see http://python.org/dev/peps/pep-0263/ for details

Removing the use_reloader from the call to run_simple() works. After digging into a couple of comments I found that https://github.com/pallets/werkzeug/blob/master/werkzeug/_reloader.py#L118 is doing some encoding to latin1 but only when the os is windows and the python version is 2. Could this be causing the Non-UTF-8 Syntax Error that I am recieving in python3?

[davidism]

Is there a small example I can use to reproduce this? I'm not familiar with using apistar and don't regularly use Windows.

[androiddrew]

I dug into it a little further. The issue is actually with _get_args_for_reloading(). That method returns args that is ['python.exe', 'apistar.exe', 'run'] which when passed to subprocess.call() generates that error. It's no different than calling python.exe apistar.exe run from the command line.

So the issue is not with the environment encoding as I originally thought. Given that it would be a very common occurrence in other werkzueg based frameworks, I think a change to this method to account for the fact that it could be called via a console_script generated .exe would benefit all windows users.

I am changing the issue title to account for this.

[androiddrew]

If this method disposed of the sys.executable element of rv within the check for windows then reloading is successful:

def _get_args_for_reloading():
    """Returns the executable. This contains a workaround for windows
    if the executable is incorrectly reported to not have the .exe
    extension which can cause bugs on reloading.
    """
    rv = [sys.executable]
    py_script = sys.argv[0]
    if os.name == 'nt' and not os.path.exists(py_script) and \
       os.path.exists(py_script + '.exe'):
        py_script += '.exe'
        rv.pop()
    rv.append(py_script)
    rv.extend(sys.argv[1:])
    return rv

[davidism]

I'm happy to review a PR if you want to submit one.

[androiddrew]

I would definitely like to, but I am a little stuck on writing a test for this. I can only see this working with the unittest.mock library and patching sys.executable and os.name, but I assume that this wouldn't work for your project since it has to support automated testing in 27 also. Any advice?

Also what is the branching strategy used here? Do pull requests for issues go against the current maintenance branch?

[davidism]

You can branch from 0.12-maintenance. Let's see what a patch looks like first then figure out the test.

[androiddrew]

I did submit a Pull Request. It appears that the Travis CI is failing though for reasons unrelated to the changes I made. This is my first Pull Request so I don't know where to go from here.

[segevfiner]

This will happen when you have setuptools installed exe launchers. If you install via—up-to-date—pip, it installs a new kind of exe launchers (from distlib) that bundle the launcher script into the exe inside a zip which Python can execute directly (runpy docs).

This doesn't mean this shouldn't be fixed, but it's helpful to know in case you can't reproduce this. It will still happen if you install Flask in develop mode (pip install -e) for example.

[ewpete]

@androiddrew @davidism any update on this PR?

@segevfiner can you add a little more detail to your most recent comment? Are you suggesting that we should probably fix the root cause in setuptools as opposed to in Flask?

[segevfiner]

@ewpete The root cause is that when using setuptools generated launchers, the sys.argv[0] is not executable as a Python script since it's the launcher exe. The actual script is named like the exe with - "-script" appended and the exe loads and runs it. Do note that the exe can be executed directly (And so is a Python script with shebang which I believe is what setuptools generates on unices). But doing so for plain Python scripts on Windows is unreliable (When the pylauncher is not installed), since it will launch with the arbitary Python that is associated for .py files in the registry, which might be different than the current Python we are using. And, of course, the same applies for any random Python script we might be invoked from, which might not even be executable. pip installs instead (via distlib) a new kind of exe launcher that has the generated launcher script appended inside a zip at the end of the executable. This will be detected as a zipfile with a __main__.py script (A zipapp) and can be passed directly to Python for execution. To fix this in setuptools will mean generating a similar/same kindbof launchers in setuptools. And in the spirit of http://xkcd.com/1172/, this will likely break code that expects the setuptools launchers and tries using the "*-script.py" files to workaround the issue. I didn't check whether there is an issue/pr about this already. It's probably fine to have a small workaround here so that Werkzeug reloader works even in the presence of the current style of the setuptools launchers. But that's really up to the project maintainers. בתאריך 28 בנוב' 2017 20:46,‏ "ewpete" <notifications@github.com> כתב:

…

@androiddrew <https://github.com/androiddrew> @davidism <https://github.com/davidism> any update on this PR? @segevfiner <https://github.com/segevfiner> can you add a little more detail to your most recent comment? Are you suggesting that we should probably fix the root cause in setuptools as opposed to in Flask? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1136 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXlg_8OSR3NdbPJzM87dyKPUAA9NpXJ6ks5s7FT7gaJpZM4N5Fcb> .the

[androiddrew]

@ewpete This was my first PR. I did my best to write tests, follow the dev guide, and place the PR. I didn't have a lot of direction on how to do this, but I did make the suggested changes from the PR comments. I thought it was complete but never heard back on whether it met the project team's expectations. Haven't looked at it since, and I don't know what people want to do with it.

[davidism]

I have very limited time across multiple projects. Don't worry, haven't forgotten about this.

[androiddrew]

@davidism Thank you, not just for responding but for doing the good work of keeping these projects alive!

[konstantint]

While this is still not merged, one possible temporary fix is to monkeypatch the _get_args_for_reloading() function somewhere before it is being used. In case of a Flask script, adding this somewhere at the beginning of a manage.py file helps:

import werkzeug._reloader
import os, sys

def _get_args_for_reloading():
    rv = [sys.executable]
    py_script = sys.argv[0]
    if os.name == 'nt' and not os.path.exists(py_script) and \
       os.path.exists(py_script + '.exe'):
        py_script += '.exe'
    if os.path.splitext(rv[0])[1] == '.exe' and os.path.splitext(py_script)[1] == '.exe':
        rv.pop()
    rv.append(py_script)
    rv.extend(sys.argv[1:])
    return rv
werkzeug._reloader._get_args_for_reloading = _get_args_for_reloading