Don't quote executable name twice in script headers

#136 Declined
Repository
pypa
Branch
default
Author
  1. Stephen Drake
Reviewers
Description

Fixes a bug that affects installing scripts with easy_install on Jython. The resulting scripts have spurious escaped quotes in the header, eg:

#!/usr/bin/env \"/path/to/jython\"

Comments (8)

  1. Jason R. Coombs

    Hmm. Thanks for sharing. I'm reluctant to just remove some code that someone added without some indication why the code is no longer needed. Presumably that code was added at some point for a purpose, though admittedly, a lot of refactoring has happened around this code, which will make it harder to trace.

    Since the base class implements these methods, and there does seem to be excessive escaping happening here, I'm inclined to accept this change. Can you test on Jython on Windows with Jython installed in "C:\Program Files" (or similar) to ensure that scripts generated in that environment still work with this change (or at least were broken before this change)?

    1. Stephen Drake author

      Ah - I saw /usr/bin/env and neglected to consider Windows. I'll give it a go and see what happens.

      I've gone back and had a more thorough look through the history, and I think what I found will make you a bit more comfortable with this change.

      Since 04d5702, CommandSpec.from_string() has used shlex.split() to parse the command line. At that point CommandSpec.from_environment() wrapped from_string(), so it was necessary to quote the executable to prevent it being split on any spaces. This is where the problem for Jython was introduced, since its version of from_string() does not use shlex.split().

      After c867696, CommandSpec.from_environment() no longer called from_string() for the default case. The behaviour for Jython remained unchanged though, including in db94c1f when quoting of the executable was moved to JythonCommandSpec.

      Just out of curiosity, did you rebase the changeset rather than accepting the pull request so as not to clutter the repository with a short-lived named branch? I created the pull request using the online editor just to try it out and didn't notice that it created a branch until it was done. And I'm not sure that I gained anything from using it since I needed a local clone to test my change anyway!

  2. Jason R. Coombs

    Nice work Steve. Thanks.

    Yes, I rebased to avoid the short-lived but permanent branch name. I'm not picky though. I'm glad to accept the pull request however is easiest for you to produce it.

  3. Stephen Drake author

    Cool. I've gone back and done some testing on Windows now. Unfortunately script generation on Jython + Windows is broken anyway since the test for Windows (sys.platform == 'win32') is not satisfied. Jython has a bug for this - http://bugs.jython.org/issue2298 - and it turns out that their patched version of setuptools includes a similar fix to the one I've submitted here. Oh well :/

    I tested with jython.exe -m easy_install wheel, the resulting header lines are below.

    @3bccac59849f:

    #!/usr/bin/env \"C:\Users\sjd\Desktop\jython2.7.0\bin\jython.exe\"

    #!/usr/bin/env "\"C:\Users\sjd\Desktop\Jython 2.7.0\bin\jython.exe\""

    @58cba6f38e8a:

    #!C:\Users\sjd\Desktop\jython2.7.0\bin\jython.exe

    #!"C:\Users\sjd\Desktop\Jython 2.7.0\bin\jython.exe"

    The /usr/bin/env prefix has disappeared because is_sh() can now correctly identify jython.exe as not a shell script (it returns a truthy value for file not found).

  4. Stephen Drake author

    Great!

    Just a final note for anyone interested - I don't believe this affected Jython on Linux. TIL that Linux allows the interpreter in a shebang line to be another script, rather than requiring the /usr/bin/env workaround. The use of that feature might be how this went unnoticed. But the change committed in 58cba6f38e8a fixes a bug on Jython + Mac OS X (and presumably others) and is a partial fix for Jython + Windows pending a further change that will enable .exe generation.