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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

samdroid-apps
Copy link
Contributor

@samdroid-apps samdroid-apps commented Dec 27, 2017 •
edited

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
Copy link
Member

FRidh 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
Copy link
Contributor Author

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
Copy link
Member

FRidh 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
Copy link
Contributor Author

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
Copy link
Contributor Author

cross reference to upstream: pallets/werkzeug#1242

@FRidh FRidh mentioned this pull request Feb 3, 2018
Merged
@samdroid-apps
Copy link
Contributor Author

Fix merged upstream

@davidism
Copy link

davidism commented Jul 15, 2019 •
edited

I'm reverting this in Werkzeug due to the number of unintended side effects of this change.

@deliciouslytyped
Copy link
Contributor

@davidism thanks for leaving a note.
#33093 may need to be reopened.

@deliciouslytyped deliciouslytyped mentioned this pull request Oct 31, 2019
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
2.status: work-in-progress 6.topic: python 10.rebuild-darwin: 101-500 10.rebuild-linux: 101-500
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Werkzerg reloader crashes due to Nix wrapped scripts
5 participants
@samdroid-apps @FRidh @davidism @deliciouslytyped @GrahamcOfBorg