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

fix for #3758; keep python flags in script's first line #3759

Closed
wants to merge 2 commits into from

Conversation

mathieulongtin
Copy link

When installing python scripts, keep the flags passed to the Python interpreter

@andy-maier
Copy link

andy-maier commented May 31, 2016

-1

Some comments:

  1. The following form worked before but will not work with this change:

    #!/usr/bin/python {args}
    

    Will be changed to:

    #!/usr/bin/{abs-path-to-python} {args}
    
  2. The following form will work (env finds it when a path is specified), but likely does not result in what was intended:

    #!/usr/bin/env python {args}
    

    Will be changed to:

    #!/usr/bin/env {abs-path-to-python} {args}
    
  3. Not sure this is relevant, but the following forms worked before and also will not work with this change:

    #!/usr/bin/env python3.2 {args}
    #!python3.2 {args}
    

    Will be changed to:

    #!/usr/bin/{abs-path-to-python}3.2 {args}
    #!{abs-path-to-python}3.2 {args}
    
  4. A unit test is missing.

@andy-maier
Copy link

How about using a regexp, that matches on any of the following:

#!{word} {args}
#!/usr/bin/env {word} {args}

In both cases, {word} is a string without whitespace, and it gets replaced with {abs-path-to-python}.

@pfmoore
Copy link
Member

pfmoore commented May 31, 2016

@andy-maier the code is guarded by a line

if not firstline.startswith(b'#!python'):
    return False

so none of your examples will be affected. The only shebang line that is changed is

#!python <arbitrary text>

At the moment, it is replaced by

#!<full-path-to-current-python>

losing the <arbitrary text>

The change replaces it with

#!<full-path-to-current-python> <arbitrary text>

However, I am -1 on this change anyway. Not all flavours of Unix support supplying flags to the Python interpreter on the shebang line, so this addition would encourage people to write non-portable scripts (more non-portable than at present, anyway - this type of script doesn't work properly on Windows even now).

Project authors should be using console entry points rather than this type of script - and (because of the lack of portability) should not rely on being able to supply interpreter arguments via the shebang line.

@mathieulongtin
Copy link
Author

mathieulongtin commented May 31, 2016

If is not allowed, then wheel building should not keep it around. The only reason I did this patch is because the wheel included the in the scripts.

@dholth
Copy link
Member

dholth commented Jun 2, 2016

I didn't consider Python interpreter arguments when writing the wheel spec. The clause 'starts with b"python"'... was supposed to mean "followed by either a Windows or Unix line separator". Wheel's reference installer 'wheel.install' also replaces the entire shebang line without preserving arguments.

This was designed in the context of automatically-generated script wrappers stored in the wheel itself, with the interpreter changed to just 'python' with no path: https://bitbucket.org/pypa/wheel/src/c02b52942cac881ce0bbf6ad204d7160049cfb28/wheel/bdist_wheel.py?at=default&fileviewer=file-view-default#bdist_wheel.py-178

Later we decided to do all the entry point script wrapper generation at install time and not store that kind of script in the wheel at all, but you can still have non-entry-points scripts if you want or if the script language is not Python.

I understand that sometimes Python arguments like -bb are important, is there a better way to handle them?

@mathieulongtin
Copy link
Author

mathieulongtin commented Jun 2, 2016

Option A

Make an console_scripts script that re-execute itself with the wanted Python flags. Something like this:

import sys
if not sys.flags.ignore_environment:
    import os
    os.execv(sys.executable, [sys.executable, '-E'] + sys.argv)

No change to Pip, but all such scripts need to start Python twice.

Option B

Use this PR and document the wheel spec change.

It has been pointed out that the preferred way is entry_points/console_scripts anyway. scripts is for shell scripts or other executable. It's clear that anything in "scripts" may not be compatible across all platforms. So why not support extra parameters.

Option C

Change "console_scripts" so to support Python flags. A lot of work, and a questionable thing todo, according to some (see comments on http://stackoverflow.com/questions/37490935 and here).

@mathieulongtin
Copy link
Author

Btw, installing via the package rather than from a wheel yields the behavior proposed by this PR.

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I won't know if this change is a bad idea or not. Just pointing out a minor nit.

firstline = b'#!' + exename + os.linesep.encode("ascii")
# Keep anything after #!python; may include flags that should be
# passed to script
firstline = firstline.replace(b'python', exename).rstrip() + \
Copy link
Member

Choose a reason for hiding this comment

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

You might want to replace only once.

firstline.replace(b'python', exename, 1)

@pradyunsg
Copy link
Member

This should probably be rebased/merged with master; the CI needs to run on this.

@pradyunsg pradyunsg added S: awaiting response Waiting for a response/more information state: needs discussion This needs some more discussion type: enhancement Improvements to functionality labels Aug 21, 2017
@mathieulongtin
Copy link
Author

I don't mind updating my pull request if it will be accepted on principle. The discussion above wasn't positive.

@pfmoore
Copy link
Member

pfmoore commented Aug 23, 2017

IMO, your option "C" (support Python interpreter flags somehow in console_scripts) is the right option. As far as I am concerned, we should be deprecating the old-style script support, as it's highly prone to platform specific issues. I'd be against this PR (your option "B") for that reason.

Having said that, no-one that I am aware of is really motivated to propose or champion such a change, so in the absence of any such support for the proposal, the status quo will win. If you believe this is important enough to warrant spending time on it, I suggest you attempt to get support for the feature on distutils-sig (the change would need to go into setuptools, and probably other tools like flit, so it needs to be discussed on the wider forum, and not just on the pip tracker). Honestly, I don't think it'll be easy to get interest - there are workarounds, and the whole idea of forcing certain Python runtime flags seems controversial, as you say. But if it would be sufficiently useful for you to warrant putting in the effort, by all means do. Personally, I'd be +0 on it (I'd never use such a feature myself, and I wouldn't have any real interest in whether it got implemented or not, but I don't object to it in principle).

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Sep 1, 2017
@pradyunsg pradyunsg removed the S: awaiting response Waiting for a response/more information label Nov 4, 2017
@pradyunsg
Copy link
Member

@mathieulongtin Do you intend to take this PR forward? If not, could you close it?

@pradyunsg pradyunsg added the S: awaiting response Waiting for a response/more information label Nov 4, 2017
@mathieulongtin
Copy link
Author

mathieulongtin commented Nov 6, 2017

I don't have the energy to pursue C. Closing since nobody seems to care.

That being said, how do we solve the problem of installing software from wheels that is safe to run, independent of the user's environment? Wrap it in shell script like Java? Can't use pip for that.

@pradyunsg pradyunsg removed the S: awaiting response Waiting for a response/more information label Nov 6, 2017
@lock
Copy link

lock bot commented Jun 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 2, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
@pradyunsg pradyunsg removed the needs rebase or merge PR has conflicts with current master label Apr 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation state: needs discussion This needs some more discussion type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants