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

pre-commit CI and linter updates #176

Merged
merged 22 commits into from
Jun 6, 2023
Merged

pre-commit CI and linter updates #176

merged 22 commits into from
Jun 6, 2023

Conversation

loostrum
Copy link
Member

Pinning compiler and linter versions in environment.yml and using this conda environment in the pre-commit CI.
Will also fix linter errors. Then this PR closes #172

@loostrum
Copy link
Member Author

Interestingly some errors that the CI is catching (see https://github.com/nlesc-recruit/cudawrappers/actions/runs/5046918293/jobs/9053071511#step:10:65)
are not caught on my machine. The pointer arithmetic errors did show up at first, but disappeared later after fixing some other things. pre-commit run -a does not catch them. clang-tidy -p build/ tests/vector_add/vector_add.cpp does catch them. I'm not sure what the exact arguments to clang-tidy are as used by pre-commit.

@loostrum
Copy link
Member Author

loostrum commented May 25, 2023

@bouweandela I fixed most of the linter issues, but I'm stuck on a missing include of the vector_add kernel in the tests. pre-commit doesn't fail on this locally so I'm not sure whats wrong. Could you explain how the cmake stuff that handles the include of cuda code works? If I change #include "vector_add_kernel.cu" to #include "kernels/vector_add_kernel.cu" the include works, but compilation fails because the code isn't actually included as a string (I think).

In file included from /home/oostrum/recruit/cudawrappers/tests/vector_add/vector_add.cpp:43:
/home/oostrum/recruit/cudawrappers/tests/vector_add/kernels/vector_add_kernel.cu: In function 'void vector_add()':
/home/oostrum/recruit/cudawrappers/tests/vector_add/kernels/vector_add_kernel.cu:1:1: error: expected primary-expression before 'extern'
    1 | extern "C" __global__ void vector_add(float *c, float *a, float *b, int n) {
      | ^~~~~~

Edit: I just found this which looks like what you did?

@loostrum
Copy link
Member Author

building with -DBUILD_TESTING=True seems to have fixed it, all green now!

@loostrum loostrum marked this pull request as ready for review May 25, 2023 11:28
@loostrum loostrum requested review from bouweandela and csbnw May 25, 2023 11:29
@loostrum
Copy link
Member Author

@csbnw there are mostly style changes in this PR, but also a few constructors were made explicit so please check if this would break any existing code. I don't think so, but better safe than sorry.

@loostrum loostrum mentioned this pull request May 25, 2023
@bouweandela
Copy link
Member

Edit: I just found this which looks like what you did?

Yes, I think the implementation here is based on the answer by 'Martin R'

environment.yml Outdated Show resolved Hide resolved
@bouweandela
Copy link
Member

I'm not sure what the exact arguments to clang-tidy are as used by pre-commit.

What it comes down to is that our pre-commit configuration here:

- repo: https://github.com/pocc/pre-commit-hooks
rev: v1.3.5
hooks:
- id: clang-tidy
types_or: ['c++', 'cuda']
args: ['-p=build']

defines how we call the hook defined here:
https://github.com/pocc/pre-commit-hooks/blob/336fdd7c3cab698ead0b1c95157b9e74d3906b62/.pre-commit-hooks.yaml#L12-L17
which calls the executable defined here:
https://github.com/pocc/pre-commit-hooks/blob/336fdd7c3cab698ead0b1c95157b9e74d3906b62/setup.cfg#L28
which is this Python script:
https://github.com/pocc/pre-commit-hooks/blob/master/hooks/clang_tidy.py

I suspect the pre-commit hooks will only work correctly if you call them from the root of this repository and you call the build directory build because the -p flag is a relative path to the build directory. Maybe you did something slightly different on your own machine?

@loostrum
Copy link
Member Author

Thanks, I think something related to the build directory happened indeed because the CI wasn't building the test code but I was doing that locally. So the compilation database was different. In any case it's now passing both locally and on the CI so it should be fixed.

@bouweandela
Copy link
Member

Looks good to me now! Could you please address the merge conflict?

@loostrum
Copy link
Member Author

loostrum commented Jun 5, 2023

Thanks, done!

@loostrum loostrum merged commit e599206 into main Jun 6, 2023
@loostrum loostrum deleted the ci-and-linter-updates branch June 6, 2023 12:41
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.

Pre-commit GitHub Action checks are failing
2 participants