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

Flask patch reversion causing breakage in development mode #72345

Open
deliciouslytyped opened this issue Oct 31, 2019 · 19 comments
Open

Flask patch reversion causing breakage in development mode #72345

deliciouslytyped opened this issue Oct 31, 2019 · 19 comments
Labels
0.kind: bug 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python

Comments

@deliciouslytyped
Copy link
Contributor

deliciouslytyped commented Oct 31, 2019

@samdroid-apps

originally reported in #33093 (and or #42924) and worked on in #33094 , reverted in pallets/werkzeug#1613

This is just my preliminary triage of an issue I ran into...details and/or repro to follow?

@davidism
Copy link

See pallets/werkzeug#1242 (comment) for why I reverted it in Werkzeug.

@deliciouslytyped
Copy link
Contributor Author

deliciouslytyped commented Oct 31, 2019

It might be reasonable to "just" write the wrapper in python. An extremely cursory skim of 42924 concurs.

Anyway. here's a repro to nix build -f repro.nix:

let
  pkgs = import ((import <nixpkgs> {}).fetchFromGitHub {
    owner = "NixOS";
    repo = "nixpkgs";
    rev = "def9d09806849d5a3c02d7a856ee26168541743d"; #master as of oct 31, 2019
    sha256 = "0x4lj4dc5wg7694i8phaa486a7bjhgfcwl37xp6p47q1qw7ngbkx";
    }) {};
  #pkgs = import <nixpkgs>;

  inherit (pkgs) runCommand python37Packages writeText;

  testscript = writeText "test.py" ''
    from flask import Flask

    app = Flask(__name__)

    @app.route('/')
    def index():
        return 'Hello World'
    '';
in
  runCommand "flask-test" { buildInputs = [ python37Packages.flask ]; } ''
    FLASK_ENV=development FLASK_APP=${testscript} flask run    
    #FLASK_APP=${testscript} flask run   
    exit 1 # always fail, its just a test
    ''

@deliciouslytyped
Copy link
Contributor Author

deliciouslytyped commented Oct 31, 2019

Something like the following seems to be a temporary workaround. This just applies the inverse of the reversion commit.

(pkgs.searx.override (old: {
          python3Packages = (pkgs.python3.override { packageOverrides =                

            let
              werkzeugPatch = pkgs.fetchpatch {
                url = "https://github.com/pallets/werkzeug/commit/726b25bdf9b7786641e19f50692b4895f3cfc3a7.patch";
                sha256 = "sha256:0c6kqhikxdl9vk6n5bl5kl1bqa2nmbpwwdlgdxkcg6zg31l24l4f";
                };
              reversedPatch = pkgs.runCommand "revWerkzeugPatch" { buildInputs = [ pkgs.patchutils ]; } ''
                interdiff -q '${werkzeugPatch}' /dev/null > $out
                '';
            in
              (self: super: {
                werkzeug = super.werkzeug.overrideAttrs (old: {
                  patches = (old.patches or []) ++ [ reversedPatch ];
                  });
                });

            }).pkgs; # todo is this correct?
          })).overrideAttrs (old: {
            src = lib.cleanSource ./../searx;
            })

@davidism
Copy link

You don't need to patch Werkzeug, you need to fix the wrapper script you generate.

@deliciouslytyped
Copy link
Contributor Author

I haven't thought about it much, but that raises some questions and I'm not sure what the way forward is yet.
For example, if we write this wrapper in python instead of bash, that's inconsistent with the rest of the system, maybe that's acceptable in this corner case.

@lopsided98
Copy link
Contributor

Another example of the bash wrapper causing problems is in mavproxy, where the map command attempts to import an executable Python file (that can also act as a standalone program) which has been wrapped by Nix.

@shuhaowu
Copy link
Contributor

Is there a workaround that would work for a single shell.nix file?

@ryneeverett
Copy link
Contributor

@shuhaowu As I recall the workarounds discussed here are for the same issue: https://discourse.nixos.org/t/python-flask-app-cant-find-dependencies/6380.

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/python-flask-app-cant-find-dependencies/6380/7

@aaronchall
Copy link

aaronchall commented May 1, 2020

@davidism mentioned this to me, so I want to try to take a look at this, no promises, but I'm interested in exploring the problem and reporting my findings...

Seems the issue is that we wrap the flask executable with a shell script that gets exec'd.

$ which flask
/run/current-system/sw/bin/flask
$ readlink $(which flask)
/nix/store/nf31p6mfqp9zz7d2jk4vrw111lg8xi8c-python3-3.7.6-env/bin/flask
$ cat $(which flask)
#! /nix/store/x7fr0bvnwvqvr3zg60di9jxvfwimcw7m-bash-4.4-p23/bin/bash -e
export NIX_PYTHONPREFIX='/nix/store/nf31p6mfqp9zz7d2jk4vrw111lg8xi8c-python3-3.7.6-env'
export NIX_PYTHONEXECUTABLE='/nix/store/nf31p6mfqp9zz7d2jk4vrw111lg8xi8c-python3-3.7.6-env/bin/python3.7'
export NIX_PYTHONPATH='/nix/store/nf31p6mfqp9zz7d2jk4vrw111lg8xi8c-python3-3.7.6-env/lib/python3.7/site-packages'
export PYTHONNOUSERSITE='true'
exec "/nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin/flask"  "$@"

Which has:

$ cat /nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin/flask
#! /nix/store/x7fr0bvnwvqvr3zg60di9jxvfwimcw7m-bash-4.4-p23/bin/bash -e
export PATH='/nix/store/55b9ip7xkpimaccw9pa0vacy5q94f5xa-python3-3.7.6/bin:/nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin'${PATH:+':'}$PATH
export PYTHONNOUSERSITE='true'
exec -a "$0" "/nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin/.flask-wrapped"  "$@"

Which has:

$ cat /nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin/.flask-wrapped
#!/nix/store/55b9ip7xkpimaccw9pa0vacy5q94f5xa-python3-3.7.6/bin/python3.7
# -*- coding: utf-8 -*-
import sys;import site;import functools;sys.argv[0] = '/nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/bin/flask';functools.reduce(lambda k, p: site.addsitedir(p, k), ['/nix/store/rzfbgssicvjwidnibg84622am22xps2f-python3.7-Flask-1.1.1/lib/python3.7/site-packages','/nix/store/qq0jadaag46ykjgnxmk2a5jwsvqd2w94-python3.7-itsdangerous-1.1.0/lib/python3.7/site-packages','/nix/store/89wngw5jmkry625a2gl75m3a8nqwbc3i-python3.7-click-7.0/lib/python3.7/site-packages','/nix/store/y2xz131cdi2iml0nccds045h7jz7vg84-python3.7-Werkzeug-0.16.1/lib/python3.7/site-packages','/nix/store/0j960a6nx3km659df72lhjkd9d65r4n5-python3.7-Jinja2-2.11.1/lib/python3.7/site-packages','/nix/store/g5zjksl70jjnha9ayj1glg12yb8f97zz-python3.7-MarkupSafe-1.1.1/lib/python3.7/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())

In our final python wrapper here, would it make sense to try to set sys.executable to the final flask-wrapped (any-wrapped?)?

We should retain the environment, we just need werkzeug to open this Python file, right?

in https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/interpreters/python/wrap-python.nix - which is inexplicably joined with a ; on the newlines, we have:

        # This preamble does two things:
        # * Sets argv[0] to the original application's name; otherwise it would be .foo-wrapped.
        #   Python doesn't support `exec -a`.
        # * Adds all required libraries to sys.path via `site.addsitedir`. It also handles *.pth files.
        preamble = ''
          import sys
          import site
          import functools
          sys.argv[0] = '"'$(readlink -f "$f")'"'
          functools.reduce(lambda k, p: site.addsitedir(p, k), ['"$([ -n "$program_PYTHONPATH" ] && (echo "'$program_PYTHONPATH'" | sed "s|:|','|g") || true)"'], site._init_pathinfo())
        '';

Put another way, we set sys.argv[0] to the original (actually in this case, prior) application's name (full path). Why do we do this? Perhaps if we did not werkzeug would just work?

Why does werkzeug assume the original executable is Python? Werkzeug is relaunching with a subprocess.call([sys.executable] + sys.argv) where sys.executable is python and argv[0] is the reassigned shell script (which we do in the preamble for what reason?)

I think, to make werkzeug's (and potentially other's) reloading to properly work on NixOS, we need to either

  1. set sys.executable = sys.argv.pop(0) (after the modification, and may break users expecting a Python executable) or
  2. remove the modification of sys.argv[0] (less surprising, less complex, but may break users expecting this to be something more like "flask" instead of ".flask-wrapped")

@FRidh - you created most of this yourself, would you mind weighing in?

@stale
Copy link

stale bot commented Nov 13, 2020

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Nov 13, 2020
@nh2
Copy link
Contributor

nh2 commented Dec 28, 2020

Definitely still a problem with FLASK_ENV=development flask run

@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Dec 28, 2020
@nh2
Copy link
Contributor

nh2 commented Dec 28, 2020

As a temporary workaround for flask users that requires code modification, this application.run(debug=True) works because it avoids the self-exec:

https://stackoverflow.com/questions/17309889/how-to-debug-a-flask-app/17309954#17309954

from flask import Flask

application = Flask(__name__)

@application.route("/")
def hello():
  return "Hello World!"

if __name__ == '__main__':
  application.run(debug=True)

when that file is run with python thatfile.py in nix-shell -p 'python3.withPackages (ps: with ps; [ flask ])'.

@davidism
Copy link

davidism commented Dec 29, 2020

It might be worth considering if this needs to work in the context of a system package manager like Nix. I might be misunderstanding, but Nix is intended to deploy working applications and manage system packages, not set up a development environment. The flask run command (and app.run(...)) should only be used in development, an installed application should not use it, and a system application shouldn't be using the flask command at all. Doubly so for running the reloader for a system application, I wouldn't expect the code to change, and if it did change presumably the package manager or init system is in charge of restarting the service.

Perhaps the flask entrypoint should be patched out since it wouldn't be useful for installed applications that depend on Flask?

@deliciouslytyped
Copy link
Contributor Author

deliciouslytyped commented Dec 29, 2020

@davidism , IIRC the python infrastructure provides some things in a shellhook, "development mode" things.

@davidism
Copy link

davidism commented Dec 29, 2020

Presumably that's in the context of "developing a system package to be installed with Nix", which would be using Flask as a dependency, not as the flask CLI and development server.

@deliciouslytyped
Copy link
Contributor Author

The substitution here also seems to work but I still haven't looked in detail enough at anything to know how anything works and how to fix it properly: https://mgdm.net/weblog/flask-on-nixos/
Namely:

myWerkzeug = python39Packages.werkzeug.overrideAttrs (oldAttrs: rec {
  postPatch = ''
    substituteInPlace src/werkzeug/_reloader.py \
      --replace "rv = [sys.executable]" "return sys.argv"
  '';

@sersorrel
Copy link
Contributor

the approach of changing argv[0] for python programs seems to actually date back all the way to 24ef871, by @ctheune (and committed by @domenkozar).

fwiw, I copied the flask wrappers into another directory, made them refer to each other, and removed the part in the python preamble that sets argv[0], and it seemed to work mostly fine; the most obvious problem is that flask apparently uses its argv[0] to refer to itself in usage messages (and I think this is quite common in python applications), so you get something like this:

$ ./a
Usage: c [OPTIONS] COMMAND [ARGS]...

  A general utility script for Flask applications.

(where c would in reality probably be .flask-wrapped) instead of Usage: flask [OPTIONS] COMMAND [ARGS]...

@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/cannot-get-flask-cli-to-see-python-plugin-without-using-nix-shell/17158/4

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: bug 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: python
Projects
None yet
Development

No branches or pull requests

10 participants