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

ci: Add pre-commit hooks for most of our linting #3560

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 11 additions & 11 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,26 @@ REQUEST.

The prefix must be one of:

- `fix`: for a bugfix
- `feat`: for a new feature
- `refactor`: for an improvement of an existing feature
- `perf`, `test`: for performance- or test-related changes
- `docs`: for documentation-related changes
- `build`, `ci`, `chore`: as appropriated for infrastructure changes
- `fix`: for a bugfix
- `feat`: for a new feature
- `refactor`: for an improvement of an existing feature
- `perf`, `test`: for performance- or test-related changes
- `docs`: for documentation-related changes
- `build`, `ci`, `chore`: as appropriated for infrastructure changes

- [ ] Does this modify the public API as defined in `docs/versioning.rst`?

- [ ] Does the PR title contain a `!` to indicate a breaking change?
- [ ] Is there section starting with `BREAKING CHANGE:` in the PR body
- [ ] Does the PR title contain a `!` to indicate a breaking change?
- [ ] Is there section starting with `BREAKING CHANGE:` in the PR body
that explains the breaking change?

- [ ] Is the PR ready to be merged?

- [ ] If not: is it marked as a draft PR?
- [ ] If not: is it marked as a draft PR?

- [ ] Does this PR close an existing issue?

- [ ] Is the issue correctly linked so it will be automatically closed
- [ ] Is the issue correctly linked so it will be automatically closed
upon successful merge (See closing keywords link in the sidebar)?

- The CI will initially report a missing milestone. One of the maintainers will
Expand All @@ -36,6 +36,6 @@ REQUEST.
- An automated workflow will assign labels based on changed files, and whether
or not reference files were changed. These do not have to be set manually.

- If you push updates, and you know they will be superceded later on, consider adding
- If you push updates, and you know they will be superseded later on, consider adding
`[skip ci]` in the commit message. This will instruct the CI system not to run any
jobs on this commit.
58 changes: 58 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -28,3 +28,61 @@ repos:
hooks:
- id: gersemi
args: ["-i", "--no-warn-about-unknown-commands"]

- repo: https://github.com/codespell-project/codespell
rev: v2.2.4
hooks:
- id: codespell
args: [
"-S", "*.ipynb,*.onnx,_build,*.svg",
"-I", "./CI/codespell_ignore.txt"
]
exclude: ^CI/.*$

- repo: local
hooks:
- id: license
name: license
language: system
entry: CI/check_license.py
files: \.(cpp|hpp|ipp|cu|cuh)$

- repo: local
hooks:
- id: include_guards
name: include_guards
language: system
entry: CI/check_include_guards.py --fail-global
files: \.hpp$

- repo: local
hooks:
- id: pragma_once
name: pragma_once
language: system
entry: CI/check_pragma_once.sh
files: \.hpp$

- repo: local
hooks:
- id: type_t
name: type_t
language: system
entry: CI/check_type_t.py
files: \.(cpp|hpp|ipp|cu|cuh)$

- repo: local
hooks:
- id: boost_test
name: boost_test
language: system
entry: CI/check_boost_test_macro.sh
files: ^Tests\/.*\.(cpp|hpp|ipp|cu|cuh)$

- repo: local
hooks:
- id: cmake_options
name: cmake_options
language: system
entry: docs/parse_cmake_options.py --write docs/getting_started.md
files: ^CMakeLists.txt$
25 changes: 16 additions & 9 deletions CI/check_boost_test_macro.sh
Original file line number Diff line number Diff line change
@@ -1,14 +1,21 @@
#!/bin/bash

files="$@"

if [ -z "$files" ]; then
files=$(find Tests -name "*.hpp" -or -name "*.cpp" -or -name "*.ipp")
fi

test_string="BOOST_TEST("
grep $test_string -n -r Tests --include "*.cpp" --include "*.hpp" --include "*.ipp"

status=$?
ec=0
for file in $files; do
grep -n "$test_string" "$file"
status=$?
if [ $status -ne 1 ]; then
echo "Found occurrences of '$test_string' in '$file'"
ec=1
fi
done

if [[ $status -eq 0 ]]; then
echo "Found occurrences of '$test_string'"
exit 1
else
echo "Did not find occurrences of '$test_string'"
exit 0
fi
exit $ec
25 changes: 15 additions & 10 deletions CI/check_include_guards.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def main():
input_help = """
Input files: either file path, dir path (will glob for headers) or custom glob pattern
"""
p.add_argument("input", help=input_help.strip())
p.add_argument("input", nargs="+", help=input_help.strip())
p.add_argument(
"--fail-local", "-l", action="store_true", help="Fail on local include guards"
)
Expand All @@ -90,15 +90,20 @@ def main():

headers = []

if os.path.isfile(args.input):
headers = [args.input]
elif os.path.isdir(args.input):
patterns = ["**/*.hpp", "**/*.h"]
headers = sum(
[glob(os.path.join(args.input, p), recursive=True) for p in patterns], []
)
else:
headers = glob(args.input, recursive=True)
if len(args.input) == 1:
if os.path.isdir(args.input[0]):
patterns = ["**/*.hpp", "**/*.h"]
headers = sum(
[
glob(os.path.join(args.input[0], p), recursive=True)
for p in patterns
],
[],
)
else:
headers = glob(args.input[0], recursive=True)
elif all([os.path.isfile(i) for i in args.input]):
headers = args.input

valid = True
nlocal = 0
Expand Down
8 changes: 4 additions & 4 deletions CI/check_license.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def check_git_dates(src):

def main():
p = argparse.ArgumentParser()
p.add_argument("input")
p.add_argument("input", nargs="+")
p.add_argument(
"--fix", action="store_true", help="Attempt to fix any license issues found."
)
Expand All @@ -99,13 +99,13 @@ def main():
args = p.parse_args()
print(args.exclude)

if os.path.isdir(args.input):
if len(args.input) == 1 and os.path.isdir(args.input[0]):
srcs = (
str(
check_output(
[
"find",
args.input,
args.input[0],
"-iname",
"*.cpp",
"-or",
Expand All @@ -123,7 +123,7 @@ def main():
)
srcs = filter(lambda p: not p.startswith("./build"), srcs)
else:
srcs = [args.input]
srcs = args.input

year = int(datetime.now().strftime("%Y"))

Expand Down
8 changes: 7 additions & 1 deletion CI/check_pragma_once.sh
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@

ec=0

for file in $(find Core Examples Tests Plugins -name "*.hpp"); do
files="$@"

if [ -z "$files" ]; then
files=$(find Core Examples Tests Plugins -name "*.hpp")
fi

for file in $files; do
res=$(grep -e "^[[:space:]]*#pragma once" $file)
if [[ "$res" != "#pragma once" ]]; then
ec=1
Expand Down
79 changes: 44 additions & 35 deletions CI/check_type_t.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,48 +48,57 @@ def handle_file(file: Path, fix: bool, c_type: str) -> list[tuple[int, str]]:

def main():
p = argparse.ArgumentParser()
p.add_argument("input")
p.add_argument("input", nargs="+")
p.add_argument("--fix", action="store_true", help="Attempt to fix C-style types.")
p.add_argument("--exclude", "-e", action="append", default=[])

args = p.parse_args()

exit_code = 0

# walk over all files
for root, _, files in os.walk("."):
root = Path(root)
for filename in files:
# get the full path of the file
filepath = root / filename
if filepath.suffix not in (
".hpp",
".cpp",
".ipp",
".h",
".C",
".c",
".cu",
".cuh",
):
continue

if any([fnmatch(str(filepath), e) for e in args.exclude]):
continue

for c_type in type_list:
changed_lines = handle_file(file=filepath, fix=args.fix, c_type=c_type)
if len(changed_lines) > 0:
exit_code = 1
print()
print(filepath)
for i, oline in changed_lines:
print(f"{i}: {oline}")

if github:
print(
f"::error file={filepath},line={i+1},title=Do not use C-style {c_type}::Replace {c_type} with std::{c_type}"
)
inputs = []

if len(args.input) == 1 and os.path.isdir(args.input[0]):
# walk over all files
for root, _, files in os.walk(args.input[0]):
root = Path(root)
for filename in files:
# get the full path of the file
filepath = root / filename
if filepath.suffix not in (
".hpp",
".cpp",
".ipp",
".h",
".C",
".c",
".cu",
".cuh",
):
continue

if any([fnmatch(str(filepath), e) for e in args.exclude]):
continue

inputs.append(filepath)
else:
for file in args.input:
inputs.append(Path(file))

for filepath in inputs:
for c_type in type_list:
changed_lines = handle_file(file=filepath, fix=args.fix, c_type=c_type)
if len(changed_lines) > 0:
exit_code = 1
print()
print(filepath)
for i, oline in changed_lines:
print(f"{i}: {oline}")

if github:
print(
f"::error file={filepath},line={i+1},title=Do not use C-style {c_type}::Replace {c_type} with std::{c_type}"
)

return exit_code

Expand Down
2 changes: 2 additions & 0 deletions CI/codespell_ignore.txt
Original file line number Diff line number Diff line change
Expand Up @@ -22,3 +22,5 @@ dthe
dthe
vart
pixelx
millepede
dependees
1 change: 0 additions & 1 deletion CITATION.cff
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ authors:
affiliation: Universite Paris-Saclay / CNRS/IN2P3
- given-names: Robert
family-names: Langenberg
affiliation: University of Massachussets
- given-names: Corentin
family-names: Allaire
affiliation: CERN
Expand Down
4 changes: 2 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ option(ACTS_BUILD_DOCS "Build documentation" OFF)
option(ACTS_SETUP_BOOST "Explicitly set up Boost for the project" ON)
option(ACTS_SETUP_EIGEN3 "Explicitly set up Eigen3 for the project" ON)
option(ACTS_BUILD_ODD "Build the OpenDataDetector" OFF)
# profiling related optios
# profiling related options
option(ACTS_ENABLE_CPU_PROFILING "Enable CPU profiling using gperftools" OFF)
option(ACTS_ENABLE_MEMORY_PROFILING "Enable memory profiling using gperftools" OFF)
set(ACTS_GPERF_INSTALL_DIR "" CACHE STRING "Hint to help find gperf if profiling is enabled")
Expand Down Expand Up @@ -298,7 +298,7 @@ find_package(Filesystem REQUIRED)

# the `<project_name>_VERSION` variables set by `setup(... VERSION ...)` have
# only local scope, i.e. they are not accessible her for dependencies added
# via `add_subdirectory`. this overrides the `project(...)` funcion for
# via `add_subdirectory`. this overrides the `project(...)` function for
# sub-projects such that the resulting `<project_name>_VERSION` has
# global scope and is accessible within the main project later on.
cmake_policy(SET CMP0048 NEW)
Expand Down
4 changes: 2 additions & 2 deletions CONTRIBUTING.rst
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ bugfix should happen in a different branch. The recommended procedure
for handling this situation is the following:

#. Get into a clean state of your working directory on your feature
branch (either by commiting open changes or by stashing them).
branch (either by committing open changes or by stashing them).
#. Checkout the branch the bugfix should be merged into (either *main*
or *release/X.Y.Z*) and get the most recent version.
#. Create a new branch for the bugfix.
Expand All @@ -238,7 +238,7 @@ Example: Backporting a feature or bugfix

Suppose you have a bugfix or feature branch that is eventually going to
be merged in ``main``. You might want to have the feature/bugfix
avilable in a patch (say ``0.25.1``) tag. To to that, find the
available in a patch (say ``0.25.1``) tag. To to that, find the
corresponding release branch, for this example that would be
``release/v0.25.X``. You must create a dedicated branch that **only**
contains the commits that relate to your feature/bugfix, otherwise the
Expand Down
4 changes: 2 additions & 2 deletions Fatras/include/ActsFatras/Digitization/PlanarSurfaceMask.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ struct PlanarSurfaceMask {
using Segment2D = std::array<Acts::Vector2, 2>;

/// Apply the mask on the segment
/// - If the semgent is full inside the surface, return unchanged
/// - If the segment is fully inside the surface, return unchanged
/// - Otherwise mask/clip the segment to fit into the bounds
///
/// @note Only PlaneSurface/DiscSurface are supported
Expand Down Expand Up @@ -64,7 +64,7 @@ struct PlanarSurfaceMask {
///
/// @param rBounds The radial disc for the masking
/// @param segment The track segment (on surface)
/// @param polarSegment The track segmetn (on surface, in polar)
/// @param polarSegment The track segment (on surface, in polar)
/// @param firstInside The indicator if the first is inside
///
/// @return a result wrapping a segment
Expand Down
2 changes: 1 addition & 1 deletion Fatras/include/ActsFatras/Digitization/Segmentizer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ struct Segmentizer {
/// Constructor with arguments
///
/// @param bin_ The bin corresponding to this step
/// @param path2D_ The start/end 2D position of the segement
/// @param path2D_ The start/end 2D position of the segment
/// @param activation_ The segment activation (clean: length) for this bin
ChannelSegment(Bin2D bin_, Segment2D path2D_, double activation_)
: bin(bin_), path2D(std::move(path2D_)), activation(activation_) {}
Expand Down
Loading
Loading