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 pre-built hooks in pre commit #402

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

paulgessinger
Copy link
Contributor

@paulgessinger paulgessinger commented Jan 30, 2025

This PR updates pre-commit to

  • Install ruff through pre-commit. pre-commit already ships with venv management, this way it just installs it on the fly
  • Use pre-compiled clang-format from PyPI. There's a PyPI repository that has static binaries of clang-format, and there's a pre-commit configuration that uses them. This has the benefit that pre-commit will install exactly the right clang-format version on the fly (we've had great success with this in ACTS).
  • Change the pre-commit script for the README links from python XXX to python3 XXX, as this is the default name of the python binary on most (all?) systems nowadays.

I tried to update the CI config to correctly run this, but let's see.

BEGINRELEASENOTES

  • Updated pre-commit configuration

ENDRELEASENOTES

@m-fila
Copy link
Contributor

m-fila commented Jan 30, 2025

Hi, I think we have local hooks on purpose as there was a preference in key4hep repos to get the dependencies from cvmfs rather than pip when possible.
The action could be probably updated to use different stack and remove pip

@paulgessinger
Copy link
Contributor Author

paulgessinger commented Jan 30, 2025

@m-fila I see the point. The benefit of using pre-commit to install hooks is that the setup on a local machine is ~ trivial, as you don't have to install anything beyond pre-commit. To that point, ruff already comes from pip, and you'll probablly be getting pre-commit from pip also, so I don't see a huge benefit of taking clang-format from cvmfs.

But it's your call in the end, of course.

Copy link
Contributor

@tmadlener tmadlener left a comment

Choose a reason for hiding this comment

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

As @m-fila has already said. The original idea behind running this on top of the Key4hep environment was that people were a bit less likely to run into potential issues, specifically around clang-format that sometimes changes its opinions on formatting for different versions. If we switch to pre-packaged hooks, we open the door for version mismatches and a bit more maintenance burden. On the other hand, I see the benefits of being able to run this without having the complete environment (especially for EDM4hep).

.pre-commit-config.yaml Outdated Show resolved Hide resolved
@paulgessinger
Copy link
Contributor Author

@tmadlener I would argue that you completely remove the possibility of version mismatches, because pre-commit will always install and use exactly the correct clang-format version. As long as the format check is run with pre-commit (which it already is), there should be no need to match versions.

@tmadlener
Copy link
Contributor

I would argue that you completely remove the possibility of version mismatches, because pre-commit will always install and use exactly the correct clang-format version.

pre-commit will, but other tools that pick up clang-tooling from elsewhere might not, so there is a chance that the auto-format hook of editors / IDEs disgaree with what is done in pre-commit. But presumably the clang-format hook also auto-fixes like other formatting hooks? In that case it might not be the biggest issue.

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