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.

Sign up for GitHub

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

Jump to bottom

Refactor add_cpplint_files and add_clang_format files to support downstream projects, large file numbers. #3762

Merged
merged 8 commits into from
Dec 8, 2022
Merged

Refactor add_cpplint_files and add_clang_format files to support downstream projects, large file numbers. #3762

merged 8 commits into from
Dec 8, 2022

Conversation

fruffy-bfn
Copy link
Contributor

@fruffy-bfn fruffy-bfn commented Dec 7, 2022 •
edited by fruffy
Loading

The linters were unable to support large lists of files because of a Make/Unix limitation.

This PR does two things.

  1. It refactors how linter files are collected by setting a global property. Now downstream projects can directly write to this global property, which is then consumed by top-level P4C.
  2. It writes the collected linter files to a file, which is then piped into the appropriate linter. This way we avoid the "too many arguments" error that may occur.

davidbolvansky
davidbolvansky approved these changes Dec 7, 2022
View reviewed changes
Copy link
Contributor

@davidbolvansky davidbolvansky left a comment •
edited
Loading

Choose a reason for hiding this comment

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

changes in add_cpplint_files - LGTM.

davidbolvansky
davidbolvansky reviewed Dec 7, 2022
View reviewed changes
CMakeLists.txt Outdated
# Pipe the file into cpplint.
add_custom_target(
cpplint
COMMAND cat ${CPPLINT_FILE} | xargs -n 10000000 ${CPPLINT_CMD} ${CPPLINT_ARGS}
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need '-n 10000000'?

davidbolvansky
davidbolvansky approved these changes Dec 7, 2022
View reviewed changes
CMakeLists.txt Outdated
COMMAND ${CPPLINT_CMD} ${CPPLINT_ARGS} ${CPPLINT_FILES}
# Replace the semicolon-separators with space separators.
string(REPLACE ";" " " CPPLINT_FILES "${CPPLINT_FILES}")
# Write the list to a file.
Copy link
Contributor

@davidbolvansky davidbolvansky Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I would mention why need this workaround as me reach sh limitation.

Copy link
Contributor

Choose a reason for hiding this comment

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

And probably you do not need this if you use xargs -d ';'

cmake/P4CUtils.cmake
@@ -49,7 +49,8 @@ macro (p4c_add_library name symbol var)
endmacro(p4c_add_library)

# Add files with the appropriate path to the list of linted files
function(add_cpplint_files dir filelist)
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably some warning comment is needed that these macros could be used by downstream projects so it is important to leave them as macros, etc..

Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that we are adding cpplint files to a global property this might actually not be necessary any longer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great!

@davidbolvansky
Copy link
Contributor

Also please adjust patch title and description as this PR now fixes and touches more things.

Thanks for fixing this issue!

@fruffy fruffy changed the title add_cpplint_files must be a macro, not a function. Refactor add_cpplint_files and add_clang_format files to support downstream projects, large file numbers. Dec 7, 2022
@davidbolvansky
Copy link
Contributor

I will try it on our side again to ensure everything is fine.

@davidbolvansky
Copy link
Contributor

Okay, go ahead :)

@fruffy fruffy merged commit add599c into p4lang:main Dec 8, 2022
@fruffy-bfn fruffy-bfn deleted the fix_cpplint branch December 8, 2022 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fruffy fruffy fruffy left review comments

@davidbolvansky davidbolvansky davidbolvansky approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fruffy-bfn @davidbolvansky @fruffy