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

Remove executable permissions from files that lack shebang lines #1621

Conversation

musicinmybrain
Copy link
Contributor

@musicinmybrain musicinmybrain commented Jul 5, 2021

A file that does not have a shebang (“#!/usr/bin/python3” or similar)
cannot be executed as a script, so its filesystem permissions should not
include the executable bit.

This corresponds to miguelgrinberg/python-socketio#748 and miguelgrinberg/python-engineio#240. Here, there are a couple of example files (example/app.py and example/app_namespace.py that have #!/usr/bin/env python shebangs, so their executable permissions are retained.

@miguelgrinberg
Copy link
Owner

I would rather make everything consistent and remove the shebangs from the files that have them.

@musicinmybrain
Copy link
Contributor Author

I would rather make everything consistent and remove the shebangs from the files that have them.

Are you sure? Those two files have an if __name__ == '__main__': and actually do something reasonable (run the Flask development server) when invoked as scripts. It’s probably the most obvious way to try out the example.

You know the code best, of course, and if you confirm that’s what you want, I’ll add a commit that removes the shebang files and executable permissions from those two files. Running them with python3 -m example.app will still work.

@miguelgrinberg
Copy link
Owner

Are you sure?

Well, yes. If you don't think this is okay, then why are you removing the executable bits in all the scripts? Wouldn't it make more sense to add shebangs everywhere instead?

I don't really care which style is used, as long as it is consistent. I will never document running a Python script as ./app.py because that is a Unix-specific thing that does not work on Windows, as far as documentation it will always be python app.py.

@musicinmybrain
Copy link
Contributor Author

You confirmed your intent, so I added a commit to remove the remaining shebangs and executable permissions. Thank you for your feedback.

(If it matters, your position seems reasonable, and I have no reason to push an alternative approach.)

@miguelgrinberg miguelgrinberg merged commit ee2c4e9 into miguelgrinberg:main Jul 7, 2021
@miguelgrinberg
Copy link
Owner

Thanks!

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

2 participants