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

Do not add python executable to executable scripts when reloading #1242

Merged
merged 3 commits into from Nov 19, 2018

Conversation

@samdroid-apps
Copy link
Contributor

@samdroid-apps samdroid-apps commented Jan 21, 2018

Sometimes, the file in sys.argv[0] may use an interpreter
other than python. For example; NixOs replaces sys.argv[0] to a
wrapper shell script which changes the environment variables before
running the main entrypoint.

Currently, the reloader assumes that all scripts need the python
interpreter appended their command line. This patch changes that to
check if the script is set as executable, and does not use the
python command if it is.

@davidism
Copy link
Member

@davidism davidism commented Jan 23, 2018

This seems very prone to errors. What if the script isn't executable? I don't understand what Nix is doing, so I don't know how to evaluate this patch. Why is Nix doing something weird here? Why does Werkzeug need to work around that instead of the other way around?

@samdroid-apps
Copy link
Contributor Author

@samdroid-apps samdroid-apps commented Jan 29, 2018

Thanks for the reply @davidism 😄

This seems very prone to errors. What if the script isn't executable?

If a script is marked as executable; it should probably be executable.

I don't understand what Nix is doing, so I don't know how to evaluate this patch. Why is Nix doing something weird here? Why does Werkzeug need to work around that instead of the other way around?

I'm not sure who is wrong. I'm going to explain with an example of the flask binary; where reloading is currently broken in Nix.

With nix, you might get something where the flask binary is like:

#! /nix/store/hqi64wjn83nw4mnf9a5z9r4vmpl72j3r-bash-4.4-p12/bin/bash -e
export PATH=/nix/store/cwxxbpfz2i2j34f3sgmd6vdl6wv98c2s-python3-3.6.4/bin:/nix/store/i51148l3f065w38p8idp0w5jil0d2kjc-python3.6-Flask-0.12.2/bin:/nix/store/l6dnj5nh4d6ifgssl9cwfw0vqqj09zrn-python3.6-setuptools-36.4.0/bin${PATH:+:}$PATH
export PYTHONNOUSERSITE="true"
exec -a "$0" "/nix/store/i51148l3f065w38p8idp0w5jil0d2kjc-python3.6-Flask-0.12.2/bin/.flask-wrapped"  "${extraFlagsArray[@]}" "$@"

(an uguly bash script to set the PATH and other environment variables. This needs to happen early, so that the python interpreter is started in the correct environment)

Then the entrypoint is moved to .flask-wrapped:

#!/nix/store/cwxxbpfz2i2j34f3sgmd6vdl6wv98c2s-python3-3.6.4/bin/python3.6m

# -*- coding: utf-8 -*-
import sys;import site;import functools;sys.argv[0] = '/nix/store/i51148l3f065w38p8idp0w5jil0d2kjc-python3.6-Flask-0.12.2/bin/flask';functools.reduce(lambda k, p: site.addsitedir(p, k), ['/nix/store/i51148l3f065w38p8idp0w5jil0d2kjc-python3.6-Flask-0.12.2/lib/python3.6/site-packages','/nix/store/fkfjrajxckwgrxbx9qbky7jh7mv75m6q-python3.6-itsdangerous-0.24/lib/python3.6/site-packages','/nix/store/l6dnj5nh4d6ifgssl9cwfw0vqqj09zrn-python3.6-setuptools-36.4.0/lib/python3.6/site-packages','/nix/store/nrm9sc7rg99bix45mclh96s8fa8yma61-python3.6-click-6.7/lib/python3.6/site-packages','/nix/store/c1qlbn8508pis7ml4rrprf2zzb45i288-python3.6-Werkzeug-0.12.2/lib/python3.6/site-packages','/nix/store/6ywdg5wcvk00n79rwzrb03lihglnrm0n-python3.6-Jinja2-2.9.6/lib/python3.6/site-packages','/nix/store/sfpi2nb3bkgy16sgivlj6156hg0963nw-python3.6-markupsafe-1.0/lib/python3.6/site-packages'], site._init_pathinfo());
import re
import sys

from flask.cli import main

if __name__ == '__main__':
    sys.argv[0] = re.sub(r'(-script\.pyw?|\.exe)?$', '', sys.argv[0])
    sys.exit(main())

Here, nix has added a line to change the argv[0] value so that it reflects the value that was actually used the run the command, rather than the .flask-wrapped value. This is similar to how flask's man changes the argv[0] value to remove .exe.

This change is quite similar to the workaround that already exists on windows regarding the .exe extension.

@FRidh
Copy link

@FRidh FRidh commented Feb 3, 2018

Sometimes, the file in `sys.argv[0]` may use an interpreter
other than python.  For example; NixOs replaces `sys.argv[0]` to a
wrapper shell script which changes the environment variables before
running the main entrypoint.

Currently, the reloader assumes that all scripts need the python
interpreter appended their command line.  This patch changes that to
check if the script is set as executable, and does not use the
python command if it is.
@davidism davidism added this to the 0.15 milestone Nov 19, 2018
@davidism davidism force-pushed the fix-reloader-nixos-2 branch from 81b0117 to 315b78c Nov 19, 2018
@davidism
Copy link
Member

@davidism davidism commented Nov 19, 2018

Rebased and added changelog. When I tried to do python example.py, subprocess failed because it was passed example.py but executable files need at least ./example.py. Called abspath on the Python script path to ensure this works.

@davidism davidism merged commit 8a38bca into pallets:master Nov 19, 2018
1 check passed
@samdroid-apps
Copy link
Contributor Author

@samdroid-apps samdroid-apps commented Nov 19, 2018

Awesome! Thanks for rebasing that.

@davidism
Copy link
Member

@davidism davidism commented Dec 8, 2018

This broke the reloader on Windows. The check you added returns True on Windows as well, where excluding sys.executable isn't correct. I'll figure out how to fix it.

@davidism
Copy link
Member

@davidism davidism commented Dec 8, 2018

Ended up separating the checks between os.name == "nt" and everything else, so they don't overlap.

@robsdedude
Copy link

@robsdedude robsdedude commented Apr 11, 2019

This change screwed me up. If you are working with e.g. NTFS (mounted in Linux) you can set all files to be executable or none. Great deal, I know. However, now things blow up in my face. And a shebang is not a proper solution as you might have started the server with a certain interpreter and you don't expect it be run with what ever interpreter is referenced in the shebang (changing the source code every time you want a different interpreter isn't a proper solution either).

@davidism
Copy link
Member

@davidism davidism commented Apr 11, 2019

A script marked as executable must have a corresponding #! python interpreter comment at the top.

If you are having issues because your Windows filesystem marks all files as executable in Docker or mounted in Linux, either add an interpreter comment, or use the flask run or python -m werkzeug.serving commands instead.

If you are having issues with different interpreters on different machines, you can either ensure the correct interpreter is on the PATH, or use different scripts for different deploy scenarios.

See #1482 and docker/docker.github.io#8609.

@pallets pallets locked as resolved and limited conversation to collaborators Apr 11, 2019
@pallets pallets unlocked this conversation Jul 15, 2019
@davidism
Copy link
Member

@davidism davidism commented Jul 15, 2019

@samdroid-apps due to the number of issues being raised, I'm going to need to revert this. There are many cases where this is causing more harm than good.

  • Docker on Windows mounts volumes as 777, so all files look executable inside a Linux container.
  • It's not clear from the error that Linux shows that if a file is marked executable it also needs an interpreter comment at the top.
  • A project may have a file with #!python3 in it for production, but may want to run with a specific Python in development, such as /path/to/venv/bin/python script.py without activating the venv.

Since this change, I have been getting a constant stream of bug reports here and questions on Stack Overflow. We're going to need to rethink how Nix fits in here.

@davidism
Copy link
Member

@davidism davidism commented Jul 15, 2019

I'm not familiar with Nix or the wrapper techniques it's using, but it might be enough for the .flask-wrapped script to set sys.executable.

davidism added a commit that referenced this issue Jul 15, 2019
Reverts #1242

In order to support NixOS wrappers, the reloader would call an
executable script directly. This caused issues with Windows, Docker,
and other common development environments, resulting in an
"Exec format error". Revert that change until a better workaround
can be found.
risicle pushed a commit to risicle/digitalmarketplace-buyer-frontend that referenced this issue Oct 11, 2019
This is a workaround for a change in Werkzeug 0.15.x (see pallets/werkzeug#1242).
The reloading server used by `make run-app` won't preserve the Python
executable if the running Python script is executable; this is a problem
if you want to use a specific version of Python (say if you are in a
venv).
@aaronchall
Copy link

@aaronchall aaronchall commented May 16, 2020

I'm currently studying how to fix this in NixOS:

NixOS/nixpkgs#72345 (comment)

If the problem is intractable on the NixOS side, maybe we can special case NixOS in werkzeug.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants