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

Linter usability improvements. #3709

Merged
merged 7 commits into from
Nov 21, 2022
Merged

Linter usability improvements. #3709

merged 7 commits into from
Nov 21, 2022

Conversation

fruffy
Copy link
Collaborator

@fruffy fruffy commented Nov 20, 2022

Fixes #3704 and fixes #3705. If this is merged in, the run-clang-format and run-cpplint scripts should be obsolete.

@fruffy fruffy force-pushed the fruffy/linters_fixes branch 4 times, most recently from 74436ca to 9087ce2 Compare November 20, 2022 14:47
@fruffy fruffy force-pushed the fruffy/linters_fixes branch 2 times, most recently from d1f326e to 10225b9 Compare November 20, 2022 14:59
@fruffy fruffy force-pushed the fruffy/linters_fixes branch from 10225b9 to 772a048 Compare November 20, 2022 15:13
@fruffy
Copy link
Collaborator Author

fruffy commented Nov 20, 2022

@mbudiu-vmw Making the linters part of CI is a little tricky because you have to run CMake. CMake requires a series of dependencies that need to be installed first. So the CI linter check will not be as light-weight anymore.

@fruffy fruffy force-pushed the fruffy/linters_fixes branch 4 times, most recently from f552b08 to f00e85f Compare November 20, 2022 19:02
@fruffy fruffy marked this pull request as ready for review November 20, 2022 19:27
@fruffy fruffy requested a review from mihaibudiu November 20, 2022 19:27
@fruffy fruffy force-pushed the fruffy/linters_fixes branch from f00e85f to 5e3800a Compare November 20, 2022 21:40
@kamleshbhalui
Copy link
Contributor

I think we also have to strictly say which version of clang-format CI uses, and everyone has to do the formatting with the same version, as one version can break the formatting done by another version.

@fruffy
Copy link
Collaborator Author

fruffy commented Nov 21, 2022

I think we also have to strictly say which version of clang-format CI uses, and everyone has to do the formatting with the same version, as one version can break the formatting done by another version.

I do prefer always running the latest clang-format on CI because upgrading with pip3 is easy and the project moves fairly quickly. However, we can also fix it at the current version, which is 15.0.4.

set(CLANG_FORMAT_CMD clang-format)
add_custom_target(
clang-format
COMMAND ${CLANG_FORMAT_CMD} --Werror --dry-run -i ${CLANG_FORMAT_FILES}
Copy link
Contributor

Choose a reason for hiding this comment

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

my version of clang-format does not understand these flags

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would install the latest version (15.0.4) with pip and not with apt. I do not quite know how to bridge this otherwise.

tools/*.h
)
# Filter some folders from the CI checks.
list(FILTER P4C_LINT_LIST EXCLUDE REGEX "backends/p4tools/submodules")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this how extensions should declare exclusions too?
Maybe you can add a comment if that's correct.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is done specifically for the compiler files. Extensions are free to add files how they see fit.

@fruffy fruffy force-pushed the fruffy/linters_fixes branch 2 times, most recently from 7e4490c to 4b5bfb1 Compare November 21, 2022 17:50
@fruffy fruffy force-pushed the fruffy/linters_fixes branch from 4b5bfb1 to c9856c2 Compare November 21, 2022 18:05
@fruffy fruffy merged commit 60c6ddd into main Nov 21, 2022
@fruffy fruffy deleted the fruffy/linters_fixes branch November 21, 2022 21:06
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.

clang-format is not listed as a dependency cpplint list of files is not configurable
3 participants