Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

werkzeug: patch reloader to find nix wrapped python scripts #33094

Closed

Conversation

Projects
None yet
3 participants
@samdroid-apps
Copy link
Contributor

commented Dec 27, 2017

Fixes #33093

Motivation for this change

Currently, the werkzeug will crash if the entrypoint is wrapped by Nix (for example;

This commit adds a patch to fix this. This patch is quite Nix specific; and therefore would not make sense to upstream.

An alternate way to fix this would be to stop Nix from changing the sys.argv[0] value when wrapping python scripts. However, I assume that the sys.argv[0] reassignment happens for a reason; and it is simple to patch werkzeug.

How to test

This can be tested using Flask.

Create a script main.py:

from flask import Flask

app = Flask(__name__)

@app.route('/')
def index():
    return 'Hello World'

Then run it using Flask:

nix-env -i python3.6-Flask
FLASK_APP=main.py flask run --reloader

When using the old version; this will crash during startup (as descried in #33093).

When using the patch; this will run and not crash. The reloader will as intended.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@FRidh

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

Actually, because the script is being given a shebang, it should not be run anymore like python script. This is a bug in werkzeug, it should not insert sys.executable in https://github.com/pallets/werkzeug/blob/0.13/werkzeug/_reloader.py#L58.

@samdroid-apps

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

Well; I'm not sure that Werkzeug is in the wrong here.

Other than reassigning the value in python, I'm not sure it is possible to get sys.argv[0] to refer to something other than a python file when running a python file. From the docs:

argv[0] is the script name (it is operating system dependent whether this is a full pathname or not).

https://docs.python.org/3/library/sys.html#sys.argv

I'm happy to change this patch to check the #! (tell me if that's what you would prefer 😄), but I'm not sure that this should be up-streamed. Upstream is doing something perfectly sensible; we are doing something quite crazy.

@FRidh

This comment has been minimized.

Copy link
Member

commented Dec 27, 2017

sys.argv[0] is the script name, which is an executable (in our case the wrapper). When subprocess.call calls it, it will just run it. And, in situations where there is no wrapper, the executable script would have a shebang, and so it would eventually just call the shebang.

Sphinx had made the same mistake a while ago
sphinx-doc/sphinx#3400

@samdroid-apps

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2017

Ah OK; I'll send a patch upstream and backport it here

@samdroid-apps samdroid-apps changed the title werkzerg: patch reloader to find nix wrapped python scripts werkzeug: patch reloader to find nix wrapped python scripts Dec 27, 2017

@samdroid-apps

This comment has been minimized.

Copy link
Contributor Author

commented Jan 21, 2018

cross reference to upstream: pallets/werkzeug#1242

@samdroid-apps

This comment has been minimized.

Copy link
Contributor Author

commented Dec 20, 2018

Fix merged upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.