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

Loosen restrictions on shebang. #34

Merged
merged 1 commit into from May 22, 2018
Merged

Conversation

lorencarvalho
Copy link
Contributor

This commit loosens shiv's requirements when constructing a shebang
line: rather than enforcing that the target interpreter must exist, we
simply accept what the user provides.

We no longer append -sE to the shebang as these arguments can be
provided to the value given to the --python argument.

Shiv will now exit(1) if the resulting shebang would exceed the BIN_PRM
restrictions present on most unix-y OSes.

Fixes #17


After merging I'll cut a new release (0.0.26)

@coveralls

This comment has been minimized.

@warsaw
Copy link
Collaborator

warsaw commented May 22, 2018

What do you think about keeping -sE if sys.executable (the default) is used?

@lorencarvalho
Copy link
Contributor Author

lorencarvalho commented May 22, 2018

@warsaw I thought about that too, but ultimately felt like it would be confusing... say someone didn't want -sE, they'd have to explicitly set --python to be (effectively) the same as sys.executable. I feel like that would be weird 😕

@warsaw
Copy link
Collaborator

warsaw commented May 22, 2018

True, but OTOH if they just want the defaults (which should be the most common case, right?) they'll have to use --python to add the isolation.

I guess it's a 6 vs 1/2 dozen thing, so as long as the change is well communicated and the documentation is up to date, I don't have a strong opinion either way.

docs/history.rst Outdated
always including the `-s <https://docs.python.org/3/using/cmdline.html#cmdoption-s>`_ and
dependency, the performance implications are mitigated by limiting the length of ``sys.path``.
Internally, we always including the
`-s <https://docs.python.org/3/using/cmdline.html#cmdoption-s>`_ and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this still true then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice that I added "internally we..." 😉

@lorencarvalho
Copy link
Contributor Author

I guess it's a 6 vs 1/2 dozen thing, so as long as the change is well communicated and the documentation is up to date, I don't have a strong opinion either way.

Yah, I actually feel like the behavior is more obvious now, since it's non-typical for folks to be using those flags.

@warsaw
Copy link
Collaborator

warsaw commented May 22, 2018

Re: Internally - will an outside consumer know what that means? Maybe just suggest that for better isolation from the system's and user's Python environment, these flags can be used?

Re: default use of the flags - I'm not sure whether it's non-typical because folks don't think about it, or whether they really do no want isolation in general.

None of this is a blocker for me though. Just thoughts about usability.

This commit loosens shiv's requirements when constructing a shebang
line: rather than enforcing that the target interpreter must exist, we
simply accept what the user provides.

We no longer append `-sE` to the shebang as these arguments can be
provided to the value given to the `--python` argument.

Shiv will now exit(1) if the resulting shebang would exceed the BIN_PRM
restrictions present on most unix-y OSes.

Fixes #17
@lorencarvalho
Copy link
Contributor Author

@warsaw I updated the wording in the history doc to be more clear, but rather than try and tack on -sE I think it will be better to leave that up to individual users. That way using --python vs not supplying it will be similar enough to not be confusing. (and imagine if someone supplied -sE without knowing that we add it by default! confusing!).

@dan-blanchard
Copy link

I did not actually know about -sE until looking at shiv, and thought it was a cool thing that it had. It might be nice to have an --isolated flag that appended that to the shebang.

@lorencarvalho lorencarvalho deleted the fewer_shebang_opinions branch November 2, 2018 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants