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

cpplint does not lint C++ files with the .C suffix #3128

Closed
vkucera opened this issue Nov 18, 2023 · 16 comments · Fixed by #3149
Closed

cpplint does not lint C++ files with the .C suffix #3128

vkucera opened this issue Nov 18, 2023 · 16 comments · Fixed by #3149
Labels
bug Something isn't working

Comments

@vkucera
Copy link
Contributor

vkucera commented Nov 18, 2023

Describe the bug

Adding ".C" in CPP_CPPLINT_FILE_EXTENSIONS in .mega-linter.yml does not make cpplint lint C++ files with the .C suffix.
Adding ".c" works as expected.

To Reproduce
Steps to reproduce the behavior:

  1. Add ".C", ".c", in CPP_CPPLINT_FILE_EXTENSIONS in .mega-linter.yml
CPP_CPPLINT_FILE_EXTENSIONS: [".C", ".c", ".cpp", ".h", ".cc", ".hpp", ".cxx", ".cu", ".hxx", ".c++", ".hh", ".h++", ".cuh"]
  1. Commit identical files testCpplint.C and testCpplint.c with some C++ code.
  2. Make a PR.
  3. See error
  - Using [cpplint v1.6.1] https://megalinter.io/7.5.0/descriptors/cpp_cpplint
  - MegaLinter key: [CPP_CPPLINT]
  - Rules config: identified by [cpplint]
...
  +----MATCHING LINTERS-----+---------------------------------------------------------+----------------+------------+
  | Descriptor | Linter     | Criteria                                                | Matching files | Format/Fix |
  +------------+------------+---------------------------------------------------------+----------------+------------+
  | CPP        | cpplint    | .C|.c|.cpp|.h|.cc|.hpp|.cxx|.cu|.hxx|.c++|.hh|.h++|.cuh | 2              | no         |
  +------------+------------+---------------------------------------------------------+----------------+------------+
...
  [cpplint] testCpplint.C
  [cpplint] testCpplint.c
  --Error detail:
  Ignoring testCpplint.C; not a valid file name (cu, hh, cc, hpp, c, cuh, cxx, h++, cpp, h, hxx, c++)
  testCpplint.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
  Done processing testCpplint.C
  Done processing testCpplint.c

Expected behavior

Both files should be linted with the same errors reported.

Additional context

Adding only one suffix at a time

Include .C

CPP_CPPLINT_FILE_EXTENSIONS: [".C", ".cpp", ".h", ".cc", ".hpp", ".cxx", ".cu", ".hxx", ".c++", ".hh", ".h++", ".cuh"]
  | CPP        | cpplint    | .C|.cpp|.h|.cc|.hpp|.cxx|.cu|.hxx|.c++|.hh|.h++|.cuh | 1              | no         |
...
  [cpplint] testCpplint.C

No output is produced which is inconsistent with the case where both suffixes are added.

Include .c

CPP_CPPLINT_FILE_EXTENSIONS: [".c", ".cpp", ".h", ".cc", ".hpp", ".cxx", ".cu", ".hxx", ".c++", ".hh", ".h++", ".cuh"]
  | CPP        | cpplint    | .c|.cpp|.h|.cc|.hpp|.cxx|.cu|.hxx|.c++|.hh|.h++|.cuh | 1              | no         |
...
  [cpplint] testCpplint.c
  --Error detail:
  testCpplint.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
  Done processing testCpplint.c

Running cpplint directly

Executing cpplint locally on both files produces expected identical output.

$ cpplint --version
Cpplint fork (https://github.com/cpplint/cpplint)
cpplint 1.6.1
Python 3.8.10 (default, May 26 2023, 14:05:08) 
[GCC 9.4.0]
$ cpplint --extensions=C,c,c++,cc,cpp,cu,cuh,cxx,h,h++,hh,hpp,hxx testCpplint.C
testCpplint.C:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Done processing testCpplint.C
Total errors found: 1
$ cpplint --extensions=C,c,c++,cc,cpp,cu,cuh,cxx,h,h++,hh,hpp,hxx testCpplint.c
testCpplint.c:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
Done processing testCpplint.c
Total errors found: 1
@vkucera vkucera added the bug Something isn't working label Nov 18, 2023
@echoix
Copy link
Collaborator

echoix commented Nov 18, 2023

Interesting report! I clearly see that something isn't going quite right.

@echoix
Copy link
Collaborator

echoix commented Nov 18, 2023

Could you tell us what version of Megalinter it is running, and is it locally, or on CI (if so, is it GitHub Actions?). Is it using a flavor, if yes, which one? If you are locally, on what platform are you on? Is your file system case-sensitive? I know for example that on windows, NTFS is not case-sensitive, but case-preserving. This case of having a file with the same name, except the case of the extension would probably not work on windows.

There was some changes recently where we are starting to have a c and a cpp flavor in order to have more linters in that category, and having clang-format. But these aren't out yet. It might also help us to know if the same thing happens with the beta versions too (that are the main branch)

@vkucera
Copy link
Contributor Author

vkucera commented Nov 18, 2023

Hi @echoix , thanks for your quick feedback.
v7.5.0, GitHub action, no flavour, runs-on: ubuntu-latest.
Hope it helps.

@echoix
Copy link
Collaborator

echoix commented Nov 18, 2023

To try to discriminate between a problem with file handling or extension handling, a useful case to check would be

  1. file1.c and file2.C : varies the extension, without having the same name.
  2. Files3.C, files3.c, files3.C, files3.c
  3. folder1/file4.c and folder2/file4.C

@echoix
Copy link
Collaborator

echoix commented Nov 18, 2023

Hi @echoix , thanks for your quick feedback.

v7.5.0, GitHub action, no flavour, runs-on: ubuntu-latest.

Hope it helps.

Maybe try the beta flavor to make sure it isn't something fixed in another version :s

@vkucera
Copy link
Contributor Author

vkucera commented Nov 18, 2023

To try to discriminate between a problem with file handling or extension handling, a useful case to check would be

1. `file1.c` and `file2.C` : varies the extension, without having the same name.

2. `Files3.C`, `files3.c`, `files3.C`, `files3.c`

3. `folder1/file4.c` and `folder2/file4.C`

The name does not seem to matter. I created the testCpplint files only to provide a MWE. I see the same issue with many other files. The only factor that makes a difference is the extension upper case. I haven't tried other extensions though.

@vkucera
Copy link
Contributor Author

vkucera commented Nov 18, 2023

Hi @echoix , thanks for your quick feedback.
v7.5.0, GitHub action, no flavour, runs-on: ubuntu-latest.
Hope it helps.

Maybe try the beta flavor to make sure it isn't something fixed in another version :s

Will try.

@vkucera
Copy link
Contributor Author

vkucera commented Nov 19, 2023

Hi @echoix , thanks for your quick feedback.
v7.5.0, GitHub action, no flavour, runs-on: ubuntu-latest.
Hope it helps.

Maybe try the beta flavor to make sure it isn't something fixed in another version :s

Will try.

The beta version (55f2d6f) behaves the same.

@echoix
Copy link
Collaborator

echoix commented Nov 19, 2023

Ok thanks a lot! I won't investigate tonight, but it's pretty descriptive!

@vkucera
Copy link
Contributor Author

vkucera commented Nov 19, 2023

With files foo.A, bar.b, testCpplint.cxx and

CPP_CPPLINT_FILE_EXTENSIONS: [".A", ".b", ".c", ".C", ".h", ".cxx", ".cpp"]

I get the following:

  +----MATCHING LINTERS-----+--------------------------+----------------+------------+
  | Descriptor | Linter     | Criteria                 | Matching files | Format/Fix |
  +------------+------------+--------------------------+----------------+------------+
  | CPP        | cpplint    | .A|.b|.c|.C|.h|.cxx|.cpp | 3              | no         |
  +------------+------------+--------------------------+----------------+------------+
...
  [cpplint] bar.b
  [cpplint] foo.A
  [cpplint] testCpplint.cxx
...
  --Error detail:
  Ignoring bar.b; not a valid file name (hpp, c++, hh, cuh, c, cc, cu, h, hxx, cxx, cpp, h++)
  Ignoring foo.A; not a valid file name (hpp, c++, hh, cuh, c, cc, cu, h, hxx, cxx, cpp, h++)
  testCpplint.cxx:0:  No copyright message found.  You should have a line: "Copyright [year] <Copyright Owner>"  [legal/copyright] [5]
  Done processing bar.b
  Done processing foo.A
  Done processing testCpplint.cxx

Again, locally cpplint works as expected, when provided with the --extensions argument.

That makes me suspect that the following is happening:

  1. MegaLinter considers CPP_CPPLINT_FILE_EXTENSIONS only for collecting files to be checked with cpplint but it does not pass those extensions in the --extensions=... argument of the cpplint command and therefore cpplint always only checks files with prefixes from the default list of extensions, i.e. hpp, c++, hh, cuh, c, cc, cu, h, hxx, cxx, cpp, h++. Since it already includes "c", it always works for linting .c files.

  2. MegaLinter prints a linter's output only when it exits with a non-zero code. The "not a valid file name" error does not trigger a non-zero exit code of the cpplint command, therefore it is not reported when it is the only kind of error.

@nvuillam
Copy link
Member

@vkucera your analysis seems right ! :)

  • cpplint seems to ask for file extensions whereas we already send it a list of files
  • it does notreturn a status code > 0 when it receives options it can not handle... that's strange

We can adapt MegaLinter to such behavior, probably by defining a python class to override some methods, but it is quite uncommon compared to many other linters ;)

@vkucera
Copy link
Contributor Author

vkucera commented Nov 19, 2023

@nvuillam , if you can confirm that MegaLinter indeed does not call the cpplint command with the --extensions=... option, then simply adding it would fix the issue.
I guess it should not be complicated to parse CPP_CPPLINT_FILE_EXTENSIONS and provide the formatted list of extensions to the cpplint command.

@vkucera
Copy link
Contributor Author

vkucera commented Nov 19, 2023

If you can do this when .mega-linter.yml is parsed, it will work just fine.

CPP_CPPLINT_ARGUMENTS = "--extensions=" + ",".join(e[1:] for e in CPP_CPPLINT_FILE_EXTENSIONS)

Of course provided that the potential CPP_CPPLINT_ARGUMENTS in .mega-linter.yml is included properly as well.

@nvuillam
Copy link
Member

@vkucera why not just listing the extensions from the list of files sent as arguments and dynamically create --extensions= argument ? ^^

@vkucera
Copy link
Contributor Author

vkucera commented Nov 21, 2023

@vkucera why not just listing the extensions from the list of files sent as arguments and dynamically create --extensions= argument ? ^^

Which is equivalent to creating the --extensions argument directly from CPP_CPPLINT_FILE_EXTENSIONS, isn't it?

@nvuillam
Copy link
Member

@vkucera probably but in case there are other constraints (FILTER_REGEX_EXCLUDE, etc....) , that way we'll be sure to send only the file extensions related to the list of files, not more extensions that are not matching files in the repo :)

PR on the way

nvuillam added a commit that referenced this issue Nov 21, 2023
… --extensions parameter

Fixes #3128
Co-authored-by: Vít Kučera <26327373+vkucera@users.noreply.github.com>
nvuillam added a commit that referenced this issue Nov 26, 2023
… --extensions parameter (#3149)

* cpplint: Dynamically add the list of extensions from list of files in --extensions parameter

Fixes #3128
Co-authored-by: Vít Kučera <26327373+vkucera@users.noreply.github.com>

* [MegaLinter] Apply linters fixes

* Fix way to get extension

* remove dot

* [MegaLinter] Apply linters fixes

* Bug fix

---------

Co-authored-by: nvuillam <nvuillam@users.noreply.github.com>
Co-authored-by: Edouard Choinière <27212526+echoix@users.noreply.github.com>
Co-authored-by: nvuillam <nicolas.vuillamy@cloudity.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants