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 shebang in scripts installed with --symlink #286

Merged
merged 6 commits into from Aug 19, 2019

Conversation

@sbidoul
Copy link
Contributor

sbidoul commented Aug 18, 2019

Fixes #285

I also fixed a deprecation warning for an invalid escape sequence.

@sbidoul sbidoul force-pushed the sbidoul:fix285-sbi branch from d017126 to 2ed09cc Aug 18, 2019
@@ -107,6 +111,8 @@ def test_symlink_other_python(self):
assert_islink(self.tmpdir / 'site-packages2' / 'package1',
to=str(samples_dir / 'package1'))
assert_isfile(self.tmpdir / 'scripts2' / 'pkg_script')
with (self.tmpdir / 'scripts2' / 'pkg_script').open() as f:
assert f.readline().strip() == "#!mock_python"

This comment has been minimized.

Copy link
@takluyver

takluyver Aug 19, 2019

Owner

This makes me think we should probably do an abspath on it. I don't think we want shebangs with a relative path.

This comment has been minimized.

Copy link
@sbidoul

sbidoul Aug 19, 2019

Author Contributor

That should probably go very early in the code, just after argument parsing.
That probably involves searching in PATH too, because flit install --python python just works today as way to install in the current active virtualenv.

This comment has been minimized.

Copy link
@sbidoul

sbidoul Aug 19, 2019

Author Contributor

A reliable (although a bit inefficient) possibility could be something like this

def _normalize_python(python):
    if python == sys.executable:
        return python
    return subprocess.check_output([python, "-c", "import sys; print(sys.executable)"]).strip()

This comment has been minimized.

Copy link
@sbidoul

sbidoul Aug 19, 2019

Author Contributor

possibly with a shortcut if isabs(python)

This comment has been minimized.

Copy link
@sbidoul

sbidoul Aug 19, 2019

Author Contributor

In any case, one could argue that flit works better with this PR than not, if only for people providing a full path in --python. So maybe it could be merged as is? Alternatively, if you think the above idea about normalizing args.python is valid, I'm happy to add a commit with it to this PR.

This comment has been minimized.

Copy link
@takluyver

takluyver Aug 19, 2019

Owner

Yes, good points. In most cases I think os.path.abspath(shutil.which(self.python)) should give the right answer, but it wouldn't surprise me if there's some odd corner cases with symlinks and things where that's different from running it in a subprocess and getting sys.executable.

I agree that this PR is a strict improvement, so I'm happy to merge this as if if you prefer.

@sbidoul sbidoul force-pushed the sbidoul:fix285-sbi branch from 47c9249 to 77fae67 Aug 19, 2019
@sbidoul

This comment has been minimized.

Copy link
Contributor Author

sbidoul commented Aug 19, 2019

Interestingly, one can see in the last commit that shutil.which and subprocess seem to produce different results.

image

In particular, for use as shebang in scripts.
@sbidoul sbidoul force-pushed the sbidoul:fix285-sbi branch from ae04034 to ec47b8f Aug 19, 2019
@sbidoul

This comment has been minimized.

Copy link
Contributor Author

sbidoul commented Aug 19, 2019

So I amended the last test. This should be complete now :)

@takluyver

This comment has been minimized.

Copy link
Owner

takluyver commented Aug 19, 2019

Thanks. That difference is strange - it looks like shutil.which() is getting the wrong path somehow.

@takluyver takluyver merged commit 25e3c90 into takluyver:master Aug 19, 2019
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 64.28% of diff hit (target 84.86%)
Details
codecov/project 84.83% (-0.03%) compared to f5996dc
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@sbidoul sbidoul deleted the sbidoul:fix285-sbi branch Aug 19, 2019
@sbidoul

This comment has been minimized.

Copy link
Contributor Author

sbidoul commented Aug 19, 2019

Now if only pip uninstall would support those symlinks..

@sbidoul

This comment has been minimized.

Copy link
Contributor Author

sbidoul commented Nov 19, 2019

Now if only pip uninstall would support those symlinks..

Updat:e: pip >= 19.3 now uninstalls symlinks. (pypa/pip#6914)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants
You can’t perform that action at this time.