-
Notifications
You must be signed in to change notification settings - Fork 0
C++ and Python Linting + Formatting + Format checks, including pre-commit and pre-push for GPU-code unit testing + CI #5
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
Changes from 9 commits
76dacea
0d4acab
0bc8555
2de2cd4
8f00318
c3c7789
2acfaa6
d369b5e
9b6002c
3b6a29c
4ebd983
f180271
4476ef5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,119 @@ | ||
| --- | ||
| Language: Cpp | ||
| Standard: c++20 | ||
| BasedOnStyle: LLVM | ||
| IndentWidth: 4 | ||
| TabWidth: 4 | ||
| UseTab: Never | ||
| ColumnLimit: 100 | ||
| AccessModifierOffset: -4 | ||
| AlignAfterOpenBracket: Align | ||
| AlignConsecutiveAssignments: false | ||
| AlignConsecutiveDeclarations: false | ||
| AlignEscapedNewlines: Right | ||
| AlignOperands: true | ||
| AlignTrailingComments: true | ||
| AllowShortBlocksOnASingleLine: false | ||
| AllowShortCaseLabelsOnASingleLine: false | ||
| AllowShortFunctionsOnASingleLine: Empty | ||
| AllowShortIfStatementsOnASingleLine: false | ||
| AllowShortLoopsOnASingleLine: false | ||
| AlwaysBreakAfterDefinitionReturnType: None | ||
| AlwaysBreakAfterReturnType: None | ||
| AlwaysBreakBeforeMultilineStrings: false | ||
| AlwaysBreakTemplateDeclarations: Yes | ||
| BinPackArguments: true | ||
| BinPackParameters: true | ||
| BraceWrapping: | ||
| AfterClass: false | ||
| AfterControlStatement: false | ||
| AfterEnum: false | ||
| AfterFunction: true | ||
| AfterNamespace: false | ||
| AfterObjCDeclaration: false | ||
| AfterStruct: false | ||
| AfterUnion: false | ||
| AfterExternBlock: false | ||
| BeforeCatch: false | ||
| BeforeElse: false | ||
| IndentBraces: false | ||
| SplitEmptyFunction: true | ||
| SplitEmptyRecord: true | ||
| SplitEmptyNamespace: true | ||
| BreakBeforeBinaryOperators: None | ||
| BreakBeforeBraces: Attach | ||
| BreakBeforeInheritanceComma: false | ||
| BreakInheritanceList: BeforeColon | ||
| BreakBeforeTernaryOperators: true | ||
| BreakConstructorInitializersBeforeComma: false | ||
| BreakConstructorInitializers: BeforeColon | ||
| BreakAfterJavaFieldAnnotations: false | ||
| BreakStringLiterals: true | ||
| CommentPragmas: '^ IWYU pragma:' | ||
| CompactNamespaces: false | ||
| ConstructorInitializerAllOnOneLineOrOnePerLine: false | ||
| ConstructorInitializerIndentWidth: 4 | ||
| ContinuationIndentWidth: 4 | ||
| Cpp11BracedListStyle: true | ||
| DerivePointerAlignment: false | ||
| DisableFormat: false | ||
| ExperimentalAutoDetectBinPacking: false | ||
| FixNamespaceComments: true | ||
| ForEachMacros: | ||
| - foreach | ||
| - Q_FOREACH | ||
| - BOOST_FOREACH | ||
| IncludeBlocks: Regroup | ||
| IncludeCategories: | ||
| - Regex: '^<.*\.(cu|cu\.h|cuh|hpp|h)$' | ||
| Priority: 1 | ||
| - Regex: '^<.*\.(cpp|cc|c\+\+|cxx|c)$' | ||
| Priority: 2 | ||
| - Regex: '^<.*' | ||
| Priority: 3 | ||
| - Regex: '.*' | ||
| Priority: 4 | ||
| IncludeIsMainRegex: '(Test)?$' | ||
| IndentCaseLabels: true | ||
| IndentPPDirectives: None | ||
| IndentWrappedFunctionNames: false | ||
| JavaScriptQuotes: Leave | ||
| JavaScriptWrapImports: true | ||
| KeepEmptyLinesAtTheStartOfBlocks: true | ||
| MacroBlockBegin: '' | ||
| MacroBlockEnd: '' | ||
| MaxEmptyLinesToKeep: 1 | ||
| NamespaceIndentation: None | ||
| ObjCBinPackProtocolList: Auto | ||
| ObjCBlockIndentWidth: 2 | ||
| ObjCSpaceAfterProperty: false | ||
| ObjCSpaceBeforeProtocolList: true | ||
| PenaltyBreakAssignment: 2 | ||
| PenaltyBreakBeforeFirstCallParameter: 1 | ||
| PenaltyBreakComment: 300 | ||
| PenaltyBreakFirstLessLess: 120 | ||
| PenaltyBreakString: 1000 | ||
| PenaltyBreakTemplateDeclaration: 10 | ||
| PenaltyExcessCharacter: 1000000 | ||
| PenaltyReturnTypeOnItsOwnLine: 200 | ||
| PointerAlignment: Left | ||
| ReflowComments: true | ||
| SortIncludes: true | ||
| SortUsingDeclarations: true | ||
| SpaceAfterCStyleCast: false | ||
| SpaceAfterTemplateKeyword: true | ||
| SpaceBeforeAssignmentOperators: true | ||
| SpaceBeforeParens: ControlStatements | ||
| SpaceBeforeRangeBasedForLoopColon: true | ||
| SpaceInEmptyParentheses: false | ||
| SpacesBeforeTrailingComments: 1 | ||
| SpacesInAngles: false | ||
| SpacesInContainerLiterals: true | ||
| SpacesInCStyleCastParentheses: false | ||
| SpacesInParentheses: false | ||
| SpacesInSquareBrackets: false | ||
| StatementMacros: | ||
| - Q_UNUSED | ||
| - QT_REQUIRE_VERSION | ||
| ... | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| --- | ||
| # Clang-tidy configuration for CUDA/C++ project | ||
| Checks: > | ||
| -*, | ||
| bugprone-*, | ||
| cert-*, | ||
| cppcoreguidelines-*, | ||
| performance-*, | ||
| readability-*, | ||
| modernize-*, | ||
| -bugprone-easily-swappable-parameters, | ||
| -readability-identifier-length, | ||
| -readability-identifier-naming, | ||
| -readability-named-parameter, | ||
| -readability-implicit-bool-conversion, | ||
| -cppcoreguidelines-avoid-magic-numbers, | ||
| -cppcoreguidelines-pro-bounds-array-to-pointer-decay, | ||
| -cppcoreguidelines-pro-bounds-pointer-arithmetic, | ||
| -cppcoreguidelines-pro-type-union-access, | ||
| -cppcoreguidelines-pro-type-vararg, | ||
| -cppcoreguidelines-owning-memory, | ||
| -modernize-use-trailing-return-type, | ||
| -readability-magic-numbers | ||
|
|
||
| WarningsAsErrors: '' | ||
| HeaderFilterRegex: '.*' | ||
| FormatStyle: 'file' | ||
| CheckOptions: | ||
| - key: cppcoreguidelines-special-member-functions.AllowSoleDefaultDtor | ||
| value: true | ||
| - key: cppcoreguidelines-special-member-functions.AllowMissingMoveFunctions | ||
| value: true | ||
| - key: performance-unnecessary-value-param.AllowedTypes | ||
| value: 'std::vector;.*Iterator' | ||
| - key: modernize-use-nodiscard.Macros | ||
| value: 'CUDA_CHECK' | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| name: Format and Lint | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: | ||
| - main | ||
| - master | ||
|
|
||
| jobs: | ||
| format-and-lint: | ||
| name: Format and Lint Check | ||
| runs-on: ubuntu-latest | ||
|
|
||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Install pixi | ||
| uses: prefix-dev/setup-pixi@v0.9.0 | ||
| with: | ||
| pixi-version: "latest" | ||
| cache: true # Cache pixi installation | ||
|
|
||
| - name: Cache pixi environment | ||
| uses: actions/cache@v4 | ||
| with: | ||
| path: | | ||
| ~/.pixi | ||
| .pixi | ||
| key: ${{ runner.os }}-pixi-${{ hashFiles('pixi.lock') }} | ||
| restore-keys: | | ||
| ${{ runner.os }}-pixi- | ||
| - name: Install dependencies | ||
| run: pixi install | ||
|
|
||
| - name: Check formatting | ||
| run: pixi run format-check | ||
|
|
||
| - name: Lint code | ||
| run: pixi run lint | ||
|
|
||
|
Comment on lines
37
to
48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also add the tests here. It's actually more important to run the tests than it is to do formatting and linting.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue here is that the ci has no GPU, so no CUDA code will run. That's why it can be more of a pre-push thing
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see this makes sense. Let me look into this. |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Pre-commit hooks configuration for GenMetaBalls | ||
| # See https://pre-commit.com for more information | ||
|
|
||
| repos: | ||
| # Local hooks for custom formatting and linting | ||
| - repo: local | ||
| hooks: | ||
| # Format files (C++/CUDA + Python) | ||
| - id: format | ||
| name: Format files | ||
| entry: pixi run format | ||
| language: system | ||
| pass_filenames: false | ||
| always_run: true | ||
| stages: [pre-commit] | ||
|
|
||
| # Check formatting (C++/CUDA + Python) | ||
| - id: format-check | ||
| name: Check formatting | ||
| entry: pixi run format-check | ||
| language: system | ||
| pass_filenames: false | ||
| always_run: true | ||
| stages: [pre-commit] | ||
|
|
||
| # Lint files (C++/CUDA + Python) | ||
| - id: lint | ||
| name: Lint files | ||
| entry: pixi run lint | ||
| language: system | ||
| pass_filenames: false | ||
| always_run: true | ||
| stages: [pre-commit] | ||
|
|
||
| # Run tests (pre-push hook) | ||
| - id: test | ||
| name: Run all tests | ||
| entry: scripts/test-quiet.sh | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this not using the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does do that! (running
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. TBH I prefer a |
||
| language: system | ||
| pass_filenames: false | ||
| always_run: true | ||
| stages: [pre-push] | ||
| verbose: true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,34 @@ Let's get the ball rolling with blazing fast CUDA kernels! | |
|
|
||
| ## Installation | ||
|
|
||
| For initial installation, run: | ||
| ### Usage Setup | ||
|
|
||
| To simply run `genmetaballs`: | ||
|
|
||
| ```bash | ||
| pixi install | ||
| ``` | ||
|
|
||
| ### Development Setup | ||
|
|
||
| For development: | ||
|
|
||
| ```bash | ||
| pixi install | ||
| pixi run dev-setup | ||
| ``` | ||
|
|
||
| The `dev-setup` task sets up [pre-commit](https://pre-commit.com/) git hooks: | ||
| - **Pre-commit**: Formats and lints code before each commit | ||
| - **Pre-push**: Runs all tests before pushes | ||
|
|
||
| Run the hooks manually if needed: | ||
| ```bash | ||
| pixi run pre-commit-run # Run formatting/linting checks | ||
| pixi run pre-commit-run-push # Run all tests | ||
| ``` | ||
|
|
||
|
|
||
| ## Testing | ||
|
|
||
| ### C++/CUDA Tests | ||
|
|
@@ -35,3 +57,37 @@ To run both C++/CUDA and Python tests together: | |
| ```bash | ||
| pixi run test | ||
| ``` | ||
|
|
||
| ## Formatting & Linting | ||
|
|
||
| ### Formatting | ||
|
|
||
| Format all files (C++/CUDA + Python): | ||
| ```bash | ||
| pixi run format | ||
| ``` | ||
|
|
||
| Check formatting without modifying files: | ||
| ```bash | ||
| pixi run format-check | ||
| ``` | ||
|
|
||
| ### Linting | ||
|
|
||
| Lint all files: | ||
| ```bash | ||
| pixi run lint | ||
| ``` | ||
|
|
||
| Auto-fix linting issues: | ||
| ```bash | ||
| pixi run lint-fix | ||
| ``` | ||
|
|
||
| **Language-specific commands:** | ||
| - `format-cpp`, `format-python` - Format specific language | ||
| - `format-check-cpp`, `format-check-python` - Check formatting for specific language | ||
| - `lint-cpp`, `lint-python` - Lint specific language | ||
| - `lint-cpp-fix`, `lint-python-fix` - Auto-fix linting issues for specific language | ||
| - `lint-full` - C++/CUDA linting with compile_commands.json (more accurate) | ||
| - `lint-full-fix` - Auto-fix C++/CUDA issues using compile_commands.json | ||
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| from . import _genmetaballs_bindings as _gmbb | ||
|
|
||
|
|
||
| def gpu_add(a: list[float], b: list[float]) -> list[float]: | ||
| return _gmbb.gpu_add(a, b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Install pixi" step (line 18) is already implicitly running
pixi install --lockedunder the hood, so we probably don't need to explicitly runpixi installagain. Let's get rid of this in the future :)