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

A little consistency in shebangs and execute bits #136

Merged
merged 2 commits into from
Nov 11, 2021

Conversation

musicinmybrain
Copy link
Contributor

Adds a shebang to examples/rosen.py, to match the other examples.

Sets execute permission on all files with shebangs—the examples, plus setup.py—without which the shebangs have little value.

@brocksam
Copy link
Collaborator

brocksam commented Nov 4, 2021

Thanks :) Playing devil's advocate, are shebangs actually necessary at all? I always run my Python scripts by explicitly invoking the Python interpreter. Are these shebangs truly portable? And do others execute their scripts with shebangs directly?

@musicinmybrain
Copy link
Contributor Author

Thanks :) Playing devil's advocate, are shebangs actually necessary at all?

No.

Are these shebangs truly portable?

No.

And do others execute their scripts with shebangs directly?

Yes, sometimes. However, it doesn’t take much time as even a casual Python user to learn python3 script_to_run.py or similar.

I’m fully on board with the idea of removing all of the shebangs instead.

These are not really necessary—users can always pass the script name to
the Python interpreter explicitly; not really portable; and not useful
anyway unless the execute permission bit is set on the files in
question.
@brocksam
Copy link
Collaborator

brocksam commented Nov 4, 2021

Excellent, thank you! I'm going to give this a couple of days to see if @moorepants chimes in with a different opinion before I merge.

@moorepants
Copy link
Collaborator

moorepants commented Nov 4, 2021

I would suggest leaving the correct shebangs in the example scripts. If they are made executable and cyipopt is installed, then they work as expected. I'm not sure why to remove them. They are helpful for those that need/want them. The space character in some are incorrect though (I think).

@musicinmybrain
Copy link
Contributor Author

I would suggest leaving the correct shebangs in the example scripts. If they are made executable and cyipopt is installed, then they work as expected. I'm not sure why to remove them. They are helpful for those that need/want them. The space character in some are incorrect though (I think).

The space is allowed but not useful.


So if the shebangs stay, I suppose the following questions remain:

  • Add a shebang to examples/rosen.py?
  • Leave python3 in examples/hs071_scipy_jax.py and unversioned python in the others, or should they all be the same one way or the other?
  • Make files with shebangs executable in the git repo?

@moorepants
Copy link
Collaborator

moorepants commented Nov 5, 2021

Those are all good with me. I don't know the consequences of using python or python3. I don't know how many linux distros use them like debian/ubuntu or what mac does. On Ubuntu 20.04 python and python3 now point to python 3. So, maybe we stick with python in the shebang?

@brocksam
Copy link
Collaborator

brocksam commented Nov 5, 2021

I just tested this on MacOS (assuming a user has installed CyIpopt to use system Python) and using python doesn't work as it points to 2.7.x (python3 is needed in the shebang). A potentially niche use case, but I feel the shebangs cause more harm than use if they're not universally valid.

@musicinmybrain
Copy link
Contributor Author

For context, I’m working on a python-cyipopt package for Fedora Linux.

When building Python RPMs in Fedora, we rewrite all shebangs to /usr/bin/python3, because we need to ensure that RPM-installed scripts use the system Python regardless of the user environment. (Plain python is python3 if and only if the python-unversioned-command package is installed, and it never appears in shebang lines of correctly-packaged RPMs.) However, the question of portability remains valid in general.

Either approach is justifiable from my perspective—shebangs and executable bits, or neither—but the middle ground of shebangs without executable bits doesn’t make sense in my opinion. Different projects make different choices, usually for valid reasons, and I’ll follow whatever decision is reached here in the downstream package.

@musicinmybrain
Copy link
Contributor Author

(I just noticed that cyipopt/version.py and cyipopt/scipy_interface.py also have shebangs. These are more clear-cut, I think: they have no main routine or script-like content, so those shebangs can be removed.)

@brocksam
Copy link
Collaborator

brocksam commented Nov 5, 2021

Thanks for this, @musicinmybrain. Due to the risk of the shebangs not working universally, can this PR please remove all shebangs across the package (taking @moorepants's thumbs up on my comment this morning as agreement)?

@moorepants
Copy link
Collaborator

The things in the examples/ directory are not packaged with cyipopt (PyPi or conda packages). If you are packaging the examples in the fedora package, then we should get them working for that.

The shebangs aren't needed in anything that isn't intended to run as a script and can be removed.

@moorepants
Copy link
Collaborator

My thumbs up was intended for using python3 instead of python.

But, I'm fine removing them all. It's really not something to spend too much thought or typing on :)

These files have no script-like content at all, so the shebangs were
unambiguously not useful.
@musicinmybrain
Copy link
Contributor Author

The things in the examples/ directory are not packaged with cyipopt (PyPi or conda packages). If you are packaging the examples in the fedora package, then we should get them working for that.

I was planning to package them as part of a python-cyipopt-doc subpackage, where they would be installed into a documentation directory alongside the user manual. In this context, it makes sense (to me) for the examples to lack shebangs, as users can either read them as pure documentation or execute them manually with python3 /usr/share/doc/python-cyipopt-doc/examples/whatever.py. If the examples retained shebangs, I would make sure that they were installed with executable permissions and that the shebangs were correct for the distribution’s system-wide Python.

Due to the risk of the shebangs not working universally, can this PR please remove all shebangs across the package

Yes, I’ll add a commit.

@moorepants moorepants merged commit d6841be into mechmotum:master Nov 11, 2021
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.

3 participants