NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
17.68k stars 13.83k forks source link

wrapPython breaks python programs that uses parantheses around future imports #40427

Open Mic92 opened 6 years ago

Mic92 commented 6 years ago

Issue description

In the bcc the future import spawns across multiple lines:

https://github.com/iovisor/bcc/blob/643356941ed0fe5aa3dbbab81e62f439c644b875/tools/deadlock_detector.py#L47

from __future__ import (
    absolute_import, division, unicode_literals, print_function
)

wrapPython will then insert the following code:

from __future__ import (
    absolute_import, division, unicode_literals, print_function
import sys;import site;import functools;sys.argv[0] = '/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/share/bcc/tools/deadlock_detector';functools.reduce(lambda k, p: site.addsitedir(p, k), ['/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/lib/python2.7/site-packages','/nix/store/fr7xxi5dmnrlvha5gmg3fv92lchy268v-python2.7-netaddr-0.7.19/lib/python2.7/site-packages','/nix/store/4d57d7z2y38k62ibb20iha8d99bqbibw-python2.7-setuptools-39.0.1/lib/python2.7/site-packages'], site._init_pathinfo());
)

which is not valid syntax

Steps to reproduce

build https://github.com/NixOS/nixpkgs/pull/40426/files and check the deadlock_detector tool

Mic92 commented 6 years ago

cc @FRidh

FRidh commented 6 years ago

Ugh... Related https://github.com/NixOS/nixpkgs/issues/11168

Mic92 commented 6 years ago

Multiline regexes are not that easy to handle though with sed.

FRidh commented 6 years ago

While I think it would be nice to get rid of patching executables, I do think it is the preferred solution. Maybe we should look into using whatever 2to3 uses to parse and modify.

Mic92 commented 6 years ago

That one maybe? https://docs.python.org/3.6/library/ast.html#module-ast

andersk commented 6 years ago

An ugly but perhaps simpler workaround is to keep the unmodified original script in a separate file and run it with exec?

import sys
import site
import functools
sys.argv[0] = '/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/share/bcc/tools/deadlock_detector'
functools.reduce(lambda k, p: site.addsitedir(p, k), ['/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/lib/python2.7/site-packages','/nix/store/fr7xxi5dmnrlvha5gmg3fv92lchy268v-python2.7-netaddr-0.7.19/lib/python2.7/site-packages','/nix/store/4d57d7z2y38k62ibb20iha8d99bqbibw-python2.7-setuptools-39.0.1/lib/python2.7/site-packages'], site._init_pathinfo())
exec(compile(open('original_script.py', 'r').read(), 'original_script.py', 'exec'))

Or with runpy.run_path:

import sys
import site
import functools
import runpy
sys.argv[0] = '/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/share/bcc/tools/deadlock_detector'
functools.reduce(lambda k, p: site.addsitedir(p, k), ['/nix/store/fra846qvw6k6l9lrwc7n7zw4v90rfc38-bcc-0.5.0/lib/python2.7/site-packages','/nix/store/fr7xxi5dmnrlvha5gmg3fv92lchy268v-python2.7-netaddr-0.7.19/lib/python2.7/site-packages','/nix/store/4d57d7z2y38k62ibb20iha8d99bqbibw-python2.7-setuptools-39.0.1/lib/python2.7/site-packages'], site._init_pathinfo())
runpy.run_path('original_script.py', run_name=__name__)
Mic92 commented 6 years ago

Does this change the original module name?

FRidh commented 6 years ago

Instead of having a shell script wrapper we would then have a Python wrapper. We could move files similar as to makeWrapper, that is py.test would exec .py.test-wrapped.

Yes, this works fine, but we should aim at getting rid of wrappers. We've been investigating also some other ways with wrappers, but the recurring issue is nested wrappers. At the same time I think this might actually be the easiest and most robust solution.

@abbradar what do you think?

andersk commented 6 years ago

@Mic92 exec preserves the module name; runpy.run_path changes it to the given run_name argument (so we just pass in __name__).

BTW if we no longer need a shell script wrapper, we can get rid of the argv[0] kludge, right?

FRidh commented 6 years ago

Correct. Also, when using runpy it fixes __file__ and can modify __name__ when needed.

stale[bot] commented 4 years ago

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.
Mic92 commented 3 years ago

still relevant.

stale[bot] commented 3 years ago

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

milahu commented 2 years ago

use an actual python parser (not a sed regex) to parse & patch the concrete syntax tree? downside of libcst: python dependency in "nix core tools"