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

cmake linting is not the same as the linting in the CI #309

Open
Yurlungur opened this issue Sep 21, 2020 · 4 comments
Open

cmake linting is not the same as the linting in the CI #309

Yurlungur opened this issue Sep 21, 2020 · 4 comments
Labels
good first issue Good for newcomers

Comments

@Yurlungur
Copy link
Collaborator

If I type make lint in a build directory, I get different output than if I call cpplint directly. Not sure what's going on here.

@pgrete
Copy link
Collaborator

pgrete commented Sep 22, 2020

In what way are they different? Are the make version and directly called version picking up the same config (CPPLINT.cfg)?

@Yurlungur
Copy link
Collaborator Author

That's a thought. I think the glob may also be different because cpplint with make lint didn't catch an error in meshblock.hpp that CI caught.

@AndrewGaspar
Copy link
Contributor

I think the glob may also be different

There actually isn't a glob - it currently requires the file to be listed on the target in cmake. Yeah, if you add new header files, you have to add it in src/CMakeLists.txt for it to get picked up.

I don't know, I'm open to this changing - we could definitely add a glob, and maybe remove the per source targets (currently we only run lints if a file has changed, which has a marginal incremental build performance benefit). I just prefer linting to happen as part of the build because otherwise it's easy to miss and you have to remember how to invoke it each time.

@Yurlungur
Copy link
Collaborator Author

Yeah, if you add new header files, you have to add it in src/CMakeLists.txt for it to get picked up.

I think I've probably missed adding a new header to the list, so that's what I ran into.

we could definitely add a glob, and maybe remove the per source targets

I think I'd prefer a glob as that would automatically catch everything. I like being able to type make lint as opposed to invoking it with python. That's easier to remember. It is a little annoying that the build won't complete while I'm experimenting and not really interested in style, but I agree it's nice it happens without me having to think about it.

@AndrewGaspar AndrewGaspar added the good first issue Good for newcomers label Sep 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants