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

Use ${Python3_EXECUTABLE} instead of shebang on top of Python files #3745

Merged
merged 2 commits into from
Jan 22, 2022

Conversation

quantumsteve
Copy link
Contributor

Please review the developer documentation
on the wiki of this project that contains help and requirements.

Proposed changes

Describe what this PR changes and why. If it closes an issue, link to it here
with a supported keyword.

This removes the shebang on top of Python files. CMake was updated to use ${Python3_EXECUTABLE}.

I avoided files in the nexus sub-directory.

Fixes #3680

What type(s) of changes does this code introduce?

Delete the items that do not apply

  • Refactoring (no functional changes, no api changes)
  • Build related changes

Does this introduce a breaking change?

  • No

What systems has this change been tested on?

Checklist

Update the following with a yes where the items apply. If you're unsure about any of them, don't hesitate to ask. This is
simply a reminder of what we are going to look for before merging your code.

  • Yes. This PR is up to date with current the current state of 'develop'

@prckent
Copy link
Contributor

prckent commented Jan 20, 2022

There are a lot of utilities and examples touched here that are not run by CMake. Many of the same considerations as for Nexus apply. However there is an argument for requiring the "correct" python to have to be specified by the user.

Copy link
Contributor

@ye-luo ye-luo left a comment

Choose a reason for hiding this comment

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

examples, labs and utils need to keep the shebang as they are not invoked by CMake/CTest.

CMake/macros.cmake Outdated Show resolved Hide resolved
@markdewing
Copy link
Contributor

Keeping all the shebangs would be a reasonable (and safe) option. Removing them all seems like a temporary hack to find all places python scripts are called from cmake to address #3680.

@jtkrogel
Copy link
Contributor

I think keeping she-bangs in anything a user might invoke is desirable. If the default provided by the she-bang is somehow wrong for their environment, they can always invoke the_correct_python_exe script. This is just about sensible defaults and convenience.

Signed-off-by: Steven Hahn <hahnse@ornl.gov>
Copy link
Contributor

@prckent prckent left a comment

Choose a reason for hiding this comment

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

List of modified files LGTM

@ye-luo ye-luo enabled auto-merge January 21, 2022 23:44
@ye-luo
Copy link
Contributor

ye-luo commented Jan 21, 2022

Test this please

@ye-luo ye-luo merged commit db42f59 into QMCPACK:develop Jan 22, 2022
@quantumsteve quantumsteve deleted the python3_executable branch January 22, 2022 01:47
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.

Invoke python all via Python3_EXECUTABLE
5 participants