Skip to content

fix broken sys.argv list#2777

Closed
miguelgrinberg wants to merge 1 commit intopallets:masterfrom
miguelgrinberg:fix-argv
Closed

fix broken sys.argv list#2777
miguelgrinberg wants to merge 1 commit intopallets:masterfrom
miguelgrinberg:fix-argv

Conversation

@miguelgrinberg
Copy link
Copy Markdown
Contributor

The cli.main() function captures the command-line arguments, and then attempts to change the arguments in the sys.argv list from flask ... to python -m flask .... The problem is that it misses the sys,argv[0] element, so the modified argument list begins with -m in the 0th element.

I honestly don't remember why sys.argv needs to be changed to the python -m form, but I assume there is a good reason for that, so this change adds sys.executable as the first argument.

Copy link
Copy Markdown
Contributor

@lepture lepture left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidism
Copy link
Copy Markdown
Member

The reloader in Werkzeug adds in sys.executable. Was something not working that requires this change? main has worked this way from the start.

@miguelgrinberg
Copy link
Copy Markdown
Contributor Author

@davidism as far as I can see this is broken with or without the reloader. Example app:

import sys
from flask import Flask

app = Flask(__name__)

@app.route('/')
def index():
    print(sys.argv)
    return 'hello'

If you run it with flask run it's all fine, but if you run it with python -m flask run the sys.argv list does not have the 0th element.

In general this isn't a problem because most apps do not need to use sys.argv directly. I suspect that is the reason why this was never seen before. I'm currently working on a little "experiment" to improve the cli and stumbled upon this.

@davidism
Copy link
Copy Markdown
Member

There is also pallets/werkzeug#531 (also by @miguelgrinberg) and pallets/werkzeug#1242. I feel like I don't understand clearly why Flask and Werkzeug are manipulating sys.executable and sys.argv the way they are, or where the actual fix needs to happen. Can we try to reconcile these three PRs?

@davidism
Copy link
Copy Markdown
Member

@miguelgrinberg (or anyone) could you comment on how the issues I linked above are related to this? I'm willing to merge one or more of these as soon as I understand where the fix should happen.

@miguelgrinberg
Copy link
Copy Markdown
Contributor Author

miguelgrinberg commented Jul 12, 2018

My understanding is that these issues come from the fact the when you start your application with "python -m module", Python does some unexpected changes to sys.argv and sys.path. If you are using the reloader, this becomes a problem, because the parent process somehow needs to figure out how to start the child process in the same way the parent process was started.

pallets/werkzeug#531 is an enhancement to Werkzeug that adds the lazy loading of the WSGI app, like Flask does, and it fixes sys.path as part of that.

This PR however, tries to make sure that sys.argv is set correctly, so that when Click comes and looks at it everything is at the expected place. This part of the code rarely run, because as_module is usually True. I have found this issue when trying to route app.run() through this same main() function so that it gets the same CLI as the flask command.

In my opinion these two issues are completely unrelated, and just share the fact that both try to compensate for the way Python handles the -m option. I wasn't familiar with pallets/werkzeug#1242, that seems to be yet another argument related fix, to make the reloader work when starting the app with a wrapper script instead of Python directly. I have not seen any such cases myself.

@davidism
Copy link
Copy Markdown
Member

One thing I remember taking a look at was that on Windows, the py command also produces a different sys.argv, since it looks like py -3 -m flask, and that failed. Does this address that as well?

@miguelgrinberg
Copy link
Copy Markdown
Contributor Author

I'm not sure this PR will handle that any better than it is currently handled, I wasn't even aware of this difference on Windows. I'll test on a Windows machine tonight and report back.

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants