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

Adds the clang-format linter #3089

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Adds the clang-format linter #3089

merged 3 commits into from
Nov 20, 2023

Conversation

daltonv
Copy link
Contributor

@daltonv daltonv commented Nov 8, 2023

Fixes #763

Proposed Changes

  1. Adds new clang-format linter

Updates the regular descriptor files for a new linter, but limits this linter to only be installed on the new c_cpp flavor, as the clang-extra-tools package is rather large.

The c and cpp tests were updated to satisfy the clang-formats requirements too.

Readiness Checklist

Author/Contributor

  • Add entry to the CHANGELOG listing the change and linking to the corresponding issue (if appropriate)
  • If documentation is needed for this change, has that been included in this pull request

Reviewing Maintainer

  • Label as breaking if this is a large fundamental change
  • Label as either automation, bug, documentation, enhancement, infrastructure, or performance

@daltonv
Copy link
Contributor Author

daltonv commented Nov 8, 2023

Not sure why the CI tests are failing. Looks like the r linter tests are the ones failing.

@daltonv
Copy link
Contributor Author

daltonv commented Nov 8, 2023

Is there a way I can test this branch against another repo? ie run megalinter locally.

I want to ensure the megalinter run picks up the .clang-format file in the root of a repo by default. I would expect it to do this, but it's always nice to test it for real.

@echoix
Copy link
Collaborator

echoix commented Nov 8, 2023

Is there a way I can test this branch against another repo? ie run megalinter locally.

I want to ensure the megalinter run picks up the .clang-format file in the root of a repo by default. I would expect it to do this, but it's always nice to test it for real.

If you build the docker image locally, you can then use npx mega-linter-runner --nodockerpull --image <the name of your image:tag> with other options to run it.

@echoix
Copy link
Collaborator

echoix commented Nov 8, 2023

Not sure why the CI tests are failing. Looks like the r linter tests are the ones failing.

It was failing yesterday too #3086

@echoix
Copy link
Collaborator

echoix commented Nov 8, 2023

Overall it looks really great!

I myself have other questions for @nvuillam :

  • When does a linter descriptor need to include the way to call --version?

  • Do we support having a linter descriptor copied two times in order to be in two files? Doesn't this create duplicate entries?

@daltonv
Copy link
Contributor Author

daltonv commented Nov 8, 2023

Is there a way I can test this branch against another repo? ie run megalinter locally.
I want to ensure the megalinter run picks up the .clang-format file in the root of a repo by default. I would expect it to do this, but it's always nice to test it for real.

If you build the docker image locally, you can then use npx mega-linter-runner --nodockerpull --image <the name of your image:tag> with other options to run it.

Okay thanks for the tip. Good thing I tried it. There is actually an issue.

I set cli_config_arg_name: "--style". However to set a config file manually you must call clang-format --style=file:.clang-format note there cannot be a space between --style=file: and the file path.

I updated my descriptor to use ``cli_config_arg_name: "--style=file:"` however this does not fix it. When I run manually I see megalinter tries to run

[clang-format] command: ['clang-format', '--Werror', '--dry-run', '--style=file:', '/tmp/lint/.clang-format', <my c files>

Is there a way I can describe in the descriptor to not add a space?

@daltonv
Copy link
Contributor Author

daltonv commented Nov 8, 2023

Worst case scenario with the cli_config_arg_name: we could remove that and the config_file_name from the descriptor. clang-format itself will search for a root level .clang-format file.

@nvuillam
Copy link
Member

nvuillam commented Nov 13, 2023

When does a linter descriptor need to include the way to call --version?

When the argument is not -v :)

Do we support having a linter descriptor copied two times in order to be in two files? Doesn't this create duplicate entries?

With current ML architecture we have no choice, we just have to make sure that the install block lines are exactly the same, to avoid duplicates in Dockerfile

@daltonv
Copy link
Contributor Author

daltonv commented Nov 14, 2023

Just rebased onto main again. Sorry for the force push. I'm used to the rebase workflow for syncing with main. Let me know if you prefer merge commits and I can use them in the future.

@nvuillam
Copy link
Member

@daltonv we merge with "squash & merge" option so merge or rebase, the result is the same in the merge commit :)

@daltonv
Copy link
Contributor Author

daltonv commented Nov 19, 2023

@nvuillam sounds good. Just wanted to ask in case it made it easier to review.

@nvuillam
Copy link
Member

@daltonv I didn't want to merge just before a new release, but if you merge conflicts, I'll merge the PR and it will be available in beta, then in next release :)

@daltonv
Copy link
Contributor Author

daltonv commented Nov 20, 2023

@nvuillam no worries. I'll fix the conflicts now.

Modifies the c and cpp descriptor files so clang-format is added to them.

In addition this modifies the c and cpp test files so clang-format now
passes. Also adds "Werror" to cspell exceptions as this is a clang term.
Direct follow-up of the previous commit. Just adds the generated files
that are a result of adding clang-format.

This closes oxsecurity#763
clang-format has a very special syntax for passing a config file as an arg.
You must use --style=:myconfig.file.

To make this work with megalinter I updated the clang descriptor files
to correctly list the config file arg as "--style=:" and modified
the Linter.py file to not add a space to any config file args ending
in ":" as clang-format cannot have a space for its cli syntax here.
@echoix echoix merged commit 2d6ae24 into oxsecurity:main Nov 20, 2023
6 checks passed
@echoix
Copy link
Collaborator

echoix commented Nov 20, 2023

Thanks a lot!!

@vkucera
Copy link
Contributor

vkucera commented Nov 20, 2023

Thanks a lot @daltonv ! This is a significant improvement of MegaLinter's assets!

@vkucera
Copy link
Contributor

vkucera commented Nov 20, 2023

Is the option to format files in place with the -i option implemented as well?

@daltonv daltonv deleted the clang-format branch November 20, 2023 11:24
@daltonv
Copy link
Contributor Author

daltonv commented Nov 20, 2023

@vkucera I didn't add the -i option. Taking another look at the descriptor schema though it should be simple for me to add that as well, I'll make a new issue to track this and I should be able to implement and test this before the next release.

@daltonv
Copy link
Contributor Author

daltonv commented Nov 20, 2023

Adding -i support is tracked at #3137

@vkucera
Copy link
Contributor

vkucera commented Nov 20, 2023

@vkucera I didn't add the -i option. Taking another look at the descriptor schema though it should be simple for me to add that as well, I'll make a new issue to track this and I should be able to implement and test this before the next release.

Thanks a lot!

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.

Add Support for clang-format
4 participants