clang-tidy parser and executor #26

Merged
张泊明518370910136 merged 26 commits from clang-tidy into master 2024-05-18 02:50:13 +08:00

TODO:

  • I currently save the clang-tidy result in a json file, and not deleting it. We may want to parse it into other things (markdown etc.) and delete it.
  • I'm now assuming each issue has only one keyword. I'm also using == when comparing keywords. We may want to use other comparing strategies if necessary (string.Contains()?)
  • I'm now commenting out the fingerprint. I don't think we will need it, but if we do, I can write another one.
  • go-unit-tests won't let me push if there are errors in tests. I intended to use a mismatch between wanted score and returned score to indicate that there are clang-tidy issues. I now have to set the wanted score to -200 in order to push the codes.
TODO: * I currently save the clang-tidy result in a json file, and not deleting it. We may want to parse it into other things (markdown etc.) and delete it. * I'm now assuming each issue has only one keyword. I'm also using `==` when comparing keywords. We may want to use other comparing strategies if necessary (`string.Contains()`?) * I'm now commenting out the fingerprint. I don't think we will need it, but if we do, I can write another one. * go-unit-tests won't let me push if there are errors in tests. I intended to use a mismatch between wanted score and returned score to indicate that there are clang-tidy issues. I now have to set the wanted score to -200 in order to push the codes.
张佳澈520370910044 added the
component
parser
label 2024-05-01 00:45:44 +08:00
张佳澈520370910044 self-assigned this 2024-05-01 00:45:44 +08:00
张佳澈520370910044 added 4 commits 2024-05-01 00:45:45 +08:00
docs(README.md): Added link to go-judge Github repo
All checks were successful
continuous-integration/drone/push Build is passing
fb61ad33e3
feat(ClangTidy-struct): Setup of ClangTidy struct, copied from Dummy
All checks were successful
continuous-integration/drone/push Build is passing
be21a16293
test(examples/clang_tidy/sillycode.cpp): Auto formatting test files
Some checks failed
continuous-integration/drone/push Build is failing
eb4815f10d
feat(internal/executors/clang_tidy,-internal/parsers/clang_tidy,-cmd/joj3/main_test.go): Parsers and executors for clang-tidy
Some checks failed
continuous-integration/drone/pr Build is failing
continuous-integration/drone/push Build is failing
e133b13a84
Author
Member

Seems that go-judge is not started on the server so drone does not work?

Also the tested score was -200 locally but -202 on the server😹

Seems that `go-judge` is not started on the server so drone does not work? Also the tested score was -200 locally but -202 on the server😹

go-judge restarted on CI server

`go-judge` restarted on CI server
张泊明518370910136 requested changes 2024-05-03 06:43:26 +08:00
@ -53,6 +53,11 @@ func TestMain(t *testing.T) {
{Score: 100, Comment: "executor status: run time: \\d+ ns, memory: \\d+ bytes"},
}},
}},
{"clang_tidy", []stage.StageResult{

ditto

ditto
zjc_he marked this conversation as resolved
@ -0,0 +1,262 @@
#include <vector>

need rebase and use the latest test convention

need rebase and use the latest test convention
Author
Member

I don't find the latest test convention, do you mean the new branches you just created? It seems they are no different from previous branches

I don't find the latest test convention, do you mean the new branches you just created? It seems they are no different from previous branches

In the master branch, we use submodules from FOCS-dev/JOJ3-examples to create and load test cases. They are not part of this repo now.

In the master branch, we use submodules from FOCS-dev/JOJ3-examples to create and load test cases. They are not part of this repo now.
zjc_he marked this conversation as resolved
@ -0,0 +20,4 @@
args += fmt.Sprint(arg)
args += " "
}
clang_tidy_Cmd := exec.Command("bash", "-c", args)

I do not think we need this executor. We can just run everything inside the sandbox.

I do not think we need this executor. We can just run everything inside the sandbox.
zjc_he marked this conversation as resolved
@ -0,0 +50,4 @@
}
return stage.ParserResult{
Score: get_score(formatted_messages, conf),
Comment: "",

We just put all the output of the parser in Comment.

We just put all the output of the parser in Comment.
Author
Member

Do you mean the json file which shows the whole detail of outputs? That might be really really long

Do you mean the json file which shows the whole detail of outputs? That might be really really long

The comment just gives human readable results. We may log the actual output for further details, but the comment will be replied in gitea issues. So we can just keep it short.

The comment just gives human readable results. We may log the actual output for further details, but the comment will be replied in gitea issues. So we can just keep it short.
Author
Member

A current version of comment:

Test results summary

  1. codequality-unchecked-malloc-result: 0
  2. codequality-no-global-variables: 0
  3. codequality-no-header-guard: 0
  4. codequality-no-fflush-stdin: 0
  5. readability-function-size: 0
  6. readability-identifier-naming: 0
  7. readability-redundant: 0
  8. readability-misleading-indentation: 0
  9. readability-misplaced-array-index: 0
  10. cppcoreguidelines-init-variables: 1
  11. bugprone-suspicious-string-compare: 0
  12. google-global-names-in-headers: 0
  13. clang-diagnostic: 1
  14. clang-analyzer: 0
  15. misc: 8
  16. performance: 4
  17. others: 187

I followed the version in ve482 issue

A current version of comment: ### Test results summary 1. codequality-unchecked-malloc-result: 0 2. codequality-no-global-variables: 0 3. codequality-no-header-guard: 0 4. codequality-no-fflush-stdin: 0 5. readability-function-size: 0 6. readability-identifier-naming: 0 7. readability-redundant: 0 8. readability-misleading-indentation: 0 9. readability-misplaced-array-index: 0 10. cppcoreguidelines-init-variables: 1 11. bugprone-suspicious-string-compare: 0 12. google-global-names-in-headers: 0 13. clang-diagnostic: 1 14. clang-analyzer: 0 15. misc: 8 16. performance: 4 17. others: 187 I followed the version in ve482 issue
zjc_he marked this conversation as resolved

16a59a7fc2

this might work for clang-tidy

https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3/commit/16a59a7fc2ce9d4b9d3796461bb7e0aa9c7236e2 this might work for clang-tidy
Author
Member

When running clang-tidy on a single file, there are always errors like this:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sillycode.cpp"
No compilation database found in /w or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
Error while processing /w/sillycode.cpp.

I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions?

Proposed solutions:

  1. Use customized executor, which can omit the error above and continue to parse the outputs.
  2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check.
When running `clang-tidy` on a single file, there are always errors like this: ``` Error while trying to load a compilation database: Could not auto-detect compilation database for file "sillycode.cpp" No compilation database found in /w or any parent directory fixed-compilation-database: Error while opening fixed database: No such file or directory json-compilation-database: Error while opening JSON database: No such file or directory Running without flags. Error while processing /w/sillycode.cpp. ``` I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions? Proposed solutions: 1. Use customized executor, which can omit the error above and continue to parse the outputs. 2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check.

When running clang-tidy on a single file, there are always errors like this:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sillycode.cpp"
No compilation database found in /w or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
Error while processing /w/sillycode.cpp.

I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions?

Proposed solutions:

  1. Use customized executor, which can omit the error above and continue to parse the outputs.
  2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check.

It is just a warning in stderr. Real output is still generated in stdout.

> When running `clang-tidy` on a single file, there are always errors like this: > ``` > Error while trying to load a compilation database: > Could not auto-detect compilation database for file "sillycode.cpp" > No compilation database found in /w or any parent directory > fixed-compilation-database: Error while opening fixed database: No such file or directory > json-compilation-database: Error while opening JSON database: No such file or directory > Running without flags. > Error while processing /w/sillycode.cpp. > ``` > I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions? > > Proposed solutions: > 1. Use customized executor, which can omit the error above and continue to parse the outputs. > 2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check. It is just a warning in stderr. Real output is still generated in stdout.
Owner

When running clang-tidy on a single file, there are always errors like this:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sillycode.cpp"
No compilation database found in /w or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
Error while processing /w/sillycode.cpp.

I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions?

Proposed solutions:

  1. Use customized executor, which can omit the error above and continue to parse the outputs.
  2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check.

It is just a warning in stderr. Real output is still generated in stdout.

in fact this is "important" for the score. Basically clang-tidy tries to figure out what flags were set at compile time. If no db can be found it assumes "standard flags". This is just a matter of using -DCMAKE_EXPORT_COMPILE_COMMANDS=ON when setting up cmake.

eg. in 482 p2 i had a custom compile function doing

cmake -S . -DDRONE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -B build
cmake --build build -j 16

DRONE option was to workaround the server and JOJ which used different versions of clang (no c++20 on joj)

CMAKE_EXPORT_COMPILE_COMMANDS=ON option is what creates the db. then clang-tidy will "search" for it. then the above error message will disappear and score might be a bit different.

> > When running `clang-tidy` on a single file, there are always errors like this: > > ``` > > Error while trying to load a compilation database: > > Could not auto-detect compilation database for file "sillycode.cpp" > > No compilation database found in /w or any parent directory > > fixed-compilation-database: Error while opening fixed database: No such file or directory > > json-compilation-database: Error while opening JSON database: No such file or directory > > Running without flags. > > Error while processing /w/sillycode.cpp. > > ``` > > I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions? > > > > Proposed solutions: > > 1. Use customized executor, which can omit the error above and continue to parse the outputs. > > 2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check. > > It is just a warning in stderr. Real output is still generated in stdout. in fact this is "important" for the score. Basically `clang-tidy` tries to figure out what flags were set at compile time. If no db can be found it assumes "standard flags". This is just a matter of using `-DCMAKE_EXPORT_COMPILE_COMMANDS=ON` when setting up `cmake`. eg. in 482 p2 i had a custom compile function doing ```sh cmake -S . -DDRONE=ON -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -B build cmake --build build -j 16 ``` `DRONE` option was to workaround the server and JOJ which used different versions of clang (no c++20 on joj) `CMAKE_EXPORT_COMPILE_COMMANDS=ON` option is what creates the db. then clang-tidy will "search" for it. then the above error message will disappear and score might be a bit different.
Author
Member

When running clang-tidy on a single file, there are always errors like this:

Error while trying to load a compilation database:
Could not auto-detect compilation database for file "sillycode.cpp"
No compilation database found in /w or any parent directory
fixed-compilation-database: Error while opening fixed database: No such file or directory
json-compilation-database: Error while opening JSON database: No such file or directory
Running without flags.
Error while processing /w/sillycode.cpp.

I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions?

Proposed solutions:

  1. Use customized executor, which can omit the error above and continue to parse the outputs.
  2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check.

It is just a warning in stderr. Real output is still generated in stdout.

Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?

> > When running `clang-tidy` on a single file, there are always errors like this: > > ``` > > Error while trying to load a compilation database: > > Could not auto-detect compilation database for file "sillycode.cpp" > > No compilation database found in /w or any parent directory > > fixed-compilation-database: Error while opening fixed database: No such file or directory > > json-compilation-database: Error while opening JSON database: No such file or directory > > Running without flags. > > Error while processing /w/sillycode.cpp. > > ``` > > I think we usually won't have a completed compilation databases uploaded together for most simple homework, which usually has one or two source files. Then when this error occurs, sandbox reports an error and won't proceed. Are there any good solutions? > > > > Proposed solutions: > > 1. Use customized executor, which can omit the error above and continue to parse the outputs. > > 2. Assume and request all uploaded files are together with compilation databases, if they are to require a clang-tidy check. > > It is just a warning in stderr. Real output is still generated in stdout. Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?
Owner

Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?

i think as soon as we use clang-tidy we should create the db. in my memory it's easier with a cmake.

have you manage to create the db, with the above commands?

> Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly? i think as soon as we use `clang-tidy` we should create the db. in my memory it's easier with a cmake. have you manage to create the db, with the above commands?

Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?

i think as soon as we use clang-tidy we should create the db. in my memory it's easier with a cmake.

have you manage to create the db, with the above commands?

Yes, we can also create that compile_commands.json using bear -- make if we only use Makefile.

> > Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly? > > i think as soon as we use `clang-tidy` we should create the db. in my memory it's easier with a cmake. > > have you manage to create the db, with the above commands? Yes, we can also create that `compile_commands.json` using `bear -- make` if we only use `Makefile`.
Author
Member

Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?

i think as soon as we use clang-tidy we should create the db. in my memory it's easier with a cmake.

have you manage to create the db, with the above commands?

The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists:

3059 warnings and 1 error generated.
Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp.

I think the non-zero return value exists as long as there are issues in clang-tidy reports, so there is not much we can do, I have to ignore the error in the parser.

Minor issue: User has to provide a CMakeLists.txt file if he wants to use clang-tidy.

Major issue: When clang-tidy is called in sandbox:

args = ["clang-tidy", "--header-filter=.*", "--quiet", "-checks=*", "sillycode.cpp","-p","/w/build"]

Error occurs:

LLVM ERROR: Cannot chdir into "/home/zjche/Desktop/JOJ3/examples/clang_tidy/build"!

The error goes the same whether the -p flag is build ./build /w/build. I think this is because sandbox uses its own filesystem, but I don't know why it still applies the path of the original file path... I've used copyInCwd = true and all files are copied
@bomingzh

> > Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly? > > i think as soon as we use `clang-tidy` we should create the db. in my memory it's easier with a cmake. > > have you manage to create the db, with the above commands? > > The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists: ``` 3059 warnings and 1 error generated. Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp. ``` I think the non-zero return value exists as long as there are issues in clang-tidy reports, so there is not much we can do, I have to ignore the error in the parser. Minor issue: User has to provide a `CMakeLists.txt` file if he wants to use clang-tidy. Major issue: When clang-tidy is called in sandbox: ``` args = ["clang-tidy", "--header-filter=.*", "--quiet", "-checks=*", "sillycode.cpp","-p","/w/build"] ``` Error occurs: ``` LLVM ERROR: Cannot chdir into "/home/zjche/Desktop/JOJ3/examples/clang_tidy/build"! ``` The error goes the same whether the `-p` flag is `build` `./build ` `/w/build`. I think this is because sandbox uses its own filesystem, but I don't know why it still applies the path of the original file path... I've used `copyInCwd = true` and all files are copied @bomingzh

Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly?

i think as soon as we use clang-tidy we should create the db. in my memory it's easier with a cmake.

have you manage to create the db, with the above commands?

The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists:

3059 warnings and 1 error generated.
Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp.

I think the non-zero return value exists as long as there are issues in clang-tidy reports, so there is not much we can do, I have to ignore the error in the parser.

Minor issue: User has to provide a CMakeLists.txt file if he wants to use clang-tidy.

Major issue: When clang-tidy is called in sandbox:

args = ["clang-tidy", "--header-filter=.*", "--quiet", "-checks=*", "sillycode.cpp","-p","/w/build"]

Error occurs:

LLVM ERROR: Cannot chdir into "/home/zjche/Desktop/JOJ3/examples/clang_tidy/build"!

The error goes the same whether the -p flag is build ./build /w/build. I think this is because sandbox uses its own filesystem, but I don't know why it still applies the path of the original file path... I've used copyInCwd = true and all files are copied
@bomingzh

Everything should run inside sandbox. compile_commands.json should also be generated inside sandbox.

> > > Then the returned value will always be non-zero and I have to ignore it in the parser. Is it fine since it may not report other errors correctly? > > > > i think as soon as we use `clang-tidy` we should create the db. in my memory it's easier with a cmake. > > > > have you manage to create the db, with the above commands? > > > > > > The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists: > ``` > 3059 warnings and 1 error generated. > Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp. > ``` > I think the non-zero return value exists as long as there are issues in clang-tidy reports, so there is not much we can do, I have to ignore the error in the parser. > > Minor issue: User has to provide a `CMakeLists.txt` file if he wants to use clang-tidy. > > Major issue: When clang-tidy is called in sandbox: > ``` > args = ["clang-tidy", "--header-filter=.*", "--quiet", "-checks=*", "sillycode.cpp","-p","/w/build"] > ``` > Error occurs: > ``` > LLVM ERROR: Cannot chdir into "/home/zjche/Desktop/JOJ3/examples/clang_tidy/build"! > ``` > The error goes the same whether the `-p` flag is `build` `./build ` `/w/build`. I think this is because sandbox uses its own filesystem, but I don't know why it still applies the path of the original file path... I've used `copyInCwd = true` and all files are copied > @bomingzh Everything should run inside sandbox. `compile_commands.json` should also be generated inside sandbox.
Author
Member

Everything should run inside sandbox. compile_commands.json should also be generated inside sandbox.

I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work

args = ["cmake", "-S", ".", "-DDRONE=ON", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-B", "build","&&","ls"]
Stderr: CMake Error: The source directory "/w/ls" does not exist.

If I seperate the two commands into two stages and call the sandbox twice, the generated files won't be kept and shared.

> Everything should run inside sandbox. compile_commands.json should also be generated inside sandbox. I can't find a method to call two commands in one stage, `;`,`&&`,`|` all don't seem to work ``` args = ["cmake", "-S", ".", "-DDRONE=ON", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-B", "build","&&","ls"] ``` ``` Stderr: CMake Error: The source directory "/w/ls" does not exist. ``` If I seperate the two commands into two stages and call the sandbox twice, the generated files won't be kept and shared.

Everything should run inside sandbox. compile_commands.json should also be generated inside sandbox.

I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work

args = ["cmake", "-S", ".", "-DDRONE=ON", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-B", "build","&&","ls"]
Stderr: CMake Error: The source directory "/w/ls" does not exist.

If I seperate the two commands into two stages and call the sandbox twice, the generated files won't be kept and shared.

Check copyOutCached and copyInCached in https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3-examples/src/branch/compile/success/conf.toml

> > Everything should run inside sandbox. compile_commands.json should also be generated inside sandbox. > > I can't find a method to call two commands in one stage, `;`,`&&`,`|` all don't seem to work > ``` > args = ["cmake", "-S", ".", "-DDRONE=ON", "-DCMAKE_EXPORT_COMPILE_COMMANDS=ON", "-B", "build","&&","ls"] > ``` > ``` > Stderr: CMake Error: The source directory "/w/ls" does not exist. > ``` > > If I seperate the two commands into two stages and call the sandbox twice, the generated files won't be kept and shared. Check `copyOutCached` and `copyInCached` in https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3-examples/src/branch/compile/success/conf.toml
张佳澈520370910044 added 1 commit 2024-05-05 14:24:25 +08:00
fix(cmd/joj3/main_test.go,-examples/clang-tidy,-internal/parsers/clang_tidy): Switched clang-tidy's executor to sandbox
Some checks failed
continuous-integration/drone/pr Build is failing
continuous-integration/drone/push Build is failing
ecb2f02182
张佳澈520370910044 added 1 commit 2024-05-05 14:30:37 +08:00
fix(internal/executors/clang_tidy): Removing unused clang-tidy executors
Some checks failed
continuous-integration/drone/pr Build is failing
continuous-integration/drone/push Build is failing
82c0146866
张佳澈520370910044 added 1 commit 2024-05-05 14:31:44 +08:00
fix(internal/executors/all.go): Fixing import bugs
Some checks failed
continuous-integration/drone/pr Build is failing
continuous-integration/drone/push Build is failing
2df84bb39a
张佳澈520370910044 added 1 commit 2024-05-05 17:55:26 +08:00
feat: merging from master
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
668676b63f
Owner

Yes, we can also create that compile_commands.json using bear -- make if we only use Makefile.

i had a look at both Makefile and CMakelist.txt, in the end i only went with the second one, while i was more used on make. cmake gives us much more flexibility.

in 151 weprovide templates with both make and cmake, but on drone we have our "own" CMakelist.txt and use it. this ensures that flags are effectively used (there are always a few students who fix the "bugs" preventing there program to compile, ie remove all the compilation flags 😂)

The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists:

3059 warnings and 1 error generated.
Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp.

not sure why you have this absolute path? have you hardcoded it somewhere or this is teh path "inside the sandbox"?

I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work

for this type of things, do we want to allow writing a simple bash function (eg. list of commands to run)? or is it better to push everything through a cmakelist/makefile?

@bomingzh

@zjc_he no need for the DRONE option on the new JOJ :-)

> Yes, we can also create that `compile_commands.json` using `bear -- make` if we only use Makefile. i had a look at both Makefile and CMakelist.txt, in the end i only went with the second one, while i was more used on `make`. cmake gives us much more flexibility. in 151 weprovide templates with both make and cmake, but on drone we have our "own" CMakelist.txt and use it. this ensures that flags are effectively used (there are always a few students who fix the "bugs" preventing there program to compile, ie remove all the compilation flags :joy:) > The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists: ``` 3059 warnings and 1 error generated. Error while processing /home/zjche/Desktop/JOJ3/examples/clang_tidy/sillycode.cpp. ``` not sure why you have this absolute path? have you hardcoded it somewhere or this is teh path "inside the sandbox"? > I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work for this type of things, do we want to allow writing a simple bash function (eg. list of commands to run)? or is it better to push everything through a cmakelist/makefile? @bomingzh @zjc_he no need for the `DRONE` option on the new JOJ :-)
张佳澈520370910044 added 1 commit 2024-05-05 19:31:15 +08:00
feat(examples/clang-tidy/sillycode): Linking clang-tidy examples to JOJ3-example repo
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
e5af7d0b29
张佳澈520370910044 added 1 commit 2024-05-05 19:39:55 +08:00
fix(examples/clang-tidy): removing old test files for clang-tidy
Some checks failed
continuous-integration/drone/push Build is failing
continuous-integration/drone/pr Build is failing
cd53640bfe
Author
Member

not sure why you have this absolute path? have you hardcoded it somewhere or this is teh path "inside the sandbox"?

This error shows the correct path in sandbox "/w/..." as long as I synced the version of my local clang-tidy to the server's

> not sure why you have this absolute path? have you hardcoded it somewhere or this is teh path "inside the sandbox"? This error shows the correct path in sandbox "/w/..." as long as I synced the version of my local clang-tidy to the server's
张佳澈520370910044 added 1 commit 2024-05-05 20:51:07 +08:00
fix(examples/clang-tidy): Update config files
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
f68bbef4a6
张佳澈520370910044 added 1 commit 2024-05-05 20:57:51 +08:00
fix(internal/clang_tidy/score.go): Unlisted keywords will be 0 point rather than 1
Some checks failed
continuous-integration/drone/pr Build is failing
continuous-integration/drone/push Build is failing
5c42a5f055
张佳澈520370910044 added 1 commit 2024-05-05 21:37:02 +08:00
feat(internal/parsers/clang_tidy): Added comments for clang-tidy parser
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
061664eb70
张佳澈520370910044 requested review from 张泊明518370910136 2024-05-05 21:38:24 +08:00
张佳澈520370910044 added 1 commit 2024-05-05 21:45:00 +08:00
fix(internal/parsers/clang_tidy/score.go): slightly fix the output format
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
430662daca

I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work

for this type of things, do we want to allow writing a simple bash function (eg. list of commands to run)? or is it better to push everything through a cmakelist/makefile?

For some simple commands, we can just call bash -c to do so.

[stages.executor.with.default]
args = ["bash", "-c", "touch a && ls"]
> > I can't find a method to call two commands in one stage, ;,&&,| all don't seem to work > > for this type of things, do we want to allow writing a simple bash function (eg. list of commands to run)? or is it better to push everything through a cmakelist/makefile? For some simple commands, we can just call `bash -c` to do so. ``` [stages.executor.with.default] args = ["bash", "-c", "touch a && ls"] ```
Author
Member

@bomingzh All resolved, can you have another review to see if it is ready to merge?

@bomingzh All resolved, can you have another review to see if it is ready to merge?
张泊明518370910136 reviewed 2024-05-07 02:46:16 +08:00
@ -0,0 +110,4 @@
}
func convert_paths_to_relative(messages *[]ClangMessage) {
currentDir := "/w"

I think it should be part of the config file with default value "/w". Since the work dir in the sandbox can also be changed, it should not be hardcoded.

I think it should be part of the config file with default value `"/w"`. Since the work dir in the sandbox can also be changed, it should not be hardcoded.
zjc_he marked this conversation as resolved
张泊明518370910136 reviewed 2024-05-07 02:48:08 +08:00
@ -0,0 +28,4 @@
formatted_messages := format(messages)
// TODO: Handle the json file (parse into markdown and delete it?)
json_file, _ := os.Create("./clangtidy_result.json")

Do we still need this file?

Do we still need this file?
Author
Member

I don't know. We will need this data for drone, but I don't know where to put it for now.

I don't know. We will need this data for drone, but I don't know where to put it for now.

I think running JOJ3 will be the only step in the future. It just provides coast-to-coast experience.

I think running JOJ3 will be the only step in the future. It just provides coast-to-coast experience.
Author
Member

What do you mean? JOJ3 will cover everything and we don't need drone anymore?

This data shows the details for all cases, we may need to parse them into markdown in the future, so if we don't need to save it into a file I'll just put it there and wait for future needs

What do you mean? JOJ3 will cover everything and we don't need drone anymore? This data shows the details for all cases, we may need to parse them into markdown in the future, so if we don't need to save it into a file I'll just put it there and wait for future needs

Yes. Drone/Gitea actions will only be used to trigger the running of JOJ3, and JOJ3 should be the only step. We can just leave the details in logs, so if any debug purpose, student can check the output in drone/gitea actions to see. So that we can only give simple and brief feedback and gitea issue comment, which should be enough for most cases.

Yes. Drone/Gitea actions will only be used to trigger the running of JOJ3, and JOJ3 should be the only step. We can just leave the details in logs, so if any debug purpose, student can check the output in drone/gitea actions to see. So that we can only give simple and brief feedback and gitea issue comment, which should be enough for most cases.
zjc_he marked this conversation as resolved
张佳澈520370910044 added 1 commit 2024-05-07 14:46:46 +08:00
feat(internal/parsers/clang_tidy): Allow working root dir to be set in config, default is "/w"
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
07ef611f95
张佳澈520370910044 added 1 commit 2024-05-07 14:52:25 +08:00
fix(internal/parsers/clang_tidy/parser.go): Removing unused local json file
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
87c2bc38d0
Author
Member

All fixed @bomingzh

All fixed @bomingzh
张泊明518370910136 reviewed 2024-05-07 14:59:19 +08:00
@ -0,0 +13,4 @@
type Conf struct {
Score int `default:"100"`
RootDir string `default:"\\w"`

Only a little confused about it. Does it mean /w by default as we run in linux? Or whether clang-tidy has some output format.

Only a little confused about it. Does it mean `/w` by default as we run in linux? Or whether clang-tidy has some output format.
Author
Member

Sorry that was a mistake

Sorry that was a mistake
zjc_he marked this conversation as resolved
张泊明518370910136 reviewed 2024-05-07 14:59:59 +08:00
@ -0,0 +186,4 @@
}
}
// func generate_fingerprint(message ClangMessage) string {

I think we can completely remove this part.

I think we can completely remove this part.
zjc_he marked this conversation as resolved
张泊明518370910136 reviewed 2024-05-07 15:00:09 +08:00
@ -0,0 +37,4 @@
Trace: extract_trace(message),
Severity: extract_severity(message),
Fingerprint: "",
// Fingerprint: generate_fingerprint(message),

ditto

ditto
zjc_he marked this conversation as resolved
张泊明518370910136 reviewed 2024-05-07 15:02:07 +08:00
@ -0,0 +27,4 @@
formatted_messages := format(messages)
// TODO: Handle unexpected errors from executor
// if executorResult.Status != stage.Status(envexec.StatusAccepted) {

I think we can uncomment it.

I think we can uncomment it.
Author
Member

No, as long as the clang-tidy report is not empty, its return value is 1 and the executor result status is Nonzero Exit Status:

main_test.go:117: actual[1].Results[0].Comment = Unexpected executor status: Nonzero Exit Status.
        Stderr: 17855 warnings and 1 error generated.
        Error while processing /w/src/sillycode.cpp.
No, as long as the clang-tidy report is not empty, its return value is 1 and the executor result status is Nonzero Exit Status: ``` main_test.go:117: actual[1].Results[0].Comment = Unexpected executor status: Nonzero Exit Status. Stderr: 17855 warnings and 1 error generated. Error while processing /w/src/sillycode.cpp. ```

Ok, then we can assume clang-tidy do not have unexpected errors for now. clang-tidy built in rules based on Libtooling are usually very strong. An alternative approach can be checking executorResult.exitStatus if we know it will only return 0 and 1.

Ok, then we can assume clang-tidy do not have unexpected errors for now. `clang-tidy` built in rules based on Libtooling are usually very strong. An alternative approach can be checking `executorResult.exitStatus` if we know it will only return 0 and 1.
Author
Member

OK

OK
zjc_he marked this conversation as resolved
张佳澈520370910044 added 1 commit 2024-05-07 15:38:16 +08:00
fix(internal/parsers/clang_tidy/parser.go): Fix typo in default root dir
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
b0f3c880d5
张佳澈520370910044 added 1 commit 2024-05-07 15:39:36 +08:00
fix(internal/parsers/clang_tidy/formatter.go): remove unused code for fingerprints
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
09024b4c15
张佳澈520370910044 added 1 commit 2024-05-07 16:04:00 +08:00
fix(internal/parsers/clang_tidy/parser.go): check the return value of executor, if not 0 or 1 then report error
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
6429d820a9
Author
Member

All fixed

All fixed

In https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3-examples/src/branch/clang-tidy/sillycode/expected.json

I see {"name":"prepare","results":[{"score":0,"comment":"Failed to parse result: invalid character '-' in numeric literal"}]}. Is that comment expected? Now dummy parser is a real parser that parse the output of dummy binary.

I think you can run two steps in one stage. It will be something like this:

args = ["bash", "-c", "cmake -S . -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -B build && clang-tidy --header-filter=.* --quiet -checks=* src/*.cpp -p build"]
In https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3-examples/src/branch/clang-tidy/sillycode/expected.json I see `{"name":"prepare","results":[{"score":0,"comment":"Failed to parse result: invalid character '-' in numeric literal"}]}`. Is that comment expected? Now dummy parser is a real parser that parse the output of `dummy` binary. I think you can run two steps in one stage. It will be something like this: ``` args = ["bash", "-c", "cmake -S . -DCMAKE_EXPORT_COMPILE_COMMANDS=ON -B build && clang-tidy --header-filter=.* --quiet -checks=* src/*.cpp -p build"] ```
张佳澈520370910044 added 1 commit 2024-05-07 16:12:20 +08:00
fix(internal/parsers/clang_tidy/score.go): Add a missing keyword
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
84121aa6f4
Author
Member

Yes I was thinking about writing a parser that does nothing at all

Putting them into one stage will work

Yes I was thinking about writing a parser that does nothing at all Putting them into one stage will work

I prefer not providing such a parser. Just use resultstatus parser to check some basic status of the executor.

I prefer not providing such a parser. Just use `resultstatus` parser to check some basic status of the executor.
Author
Member

Interestingly, when I remove the "prepare" stage and move those commands together, clang-tidy report is different: -180 -> -185, and one more unclassified keyword

Interestingly, when I remove the "prepare" stage and move those commands together, clang-tidy report is different: -180 -> -185, and one more unclassified keyword
张佳澈520370910044 added 1 commit 2024-05-07 16:24:13 +08:00
feat(examples/clang-tidy/sillycode): putting two stages together
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
0393ba6e23

Will we output the relative path where errors occured in the future?

And the naming convention in this PR does not fit well for Golang style. Please check https://google.github.io/styleguide/go/guide#mixedcaps.

Will we output the relative path where errors occured in the future? And the naming convention in this PR does not fit well for Golang style. Please check <https://google.github.io/styleguide/go/guide#mixedcaps>.
张佳澈520370910044 added 1 commit 2024-05-10 23:23:23 +08:00
style(internal/parsers/clangtidy): fix naming convention
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
adefbfd1fa
张佳澈520370910044 added 1 commit 2024-05-10 23:42:40 +08:00
refactor(examples/clangtidy): rebase clangtidy example
All checks were successful
continuous-integration/drone/pr Build is passing
continuous-integration/drone/push Build is passing
30da9a4ebd
张佳澈520370910044 added 1 commit 2024-05-10 23:48:51 +08:00
refactor(examples/keyword/clangtidy): rebase clangtidy keyword example
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
30a891b6f6
张佳澈520370910044 added 1 commit 2024-05-10 23:57:22 +08:00
style(internal/parsers/clagtidy): fix more naming conventions
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
d04f83e9fc
张佳澈520370910044 added 1 commit 2024-05-10 23:59:40 +08:00
style(internal/parsers/clangtidy/convert.go): fix more and more naming conventions
All checks were successful
continuous-integration/drone/push Build is passing
continuous-integration/drone/pr Build is passing
3aa0b2e422
Author
Member

All naming issues resolved

All naming issues resolved
张泊明518370910136 changed title from WIP: clang-tidy parser and executor to clang-tidy parser and executor 2024-05-18 02:49:07 +08:00
张泊明518370910136 approved these changes 2024-05-18 02:49:26 +08:00
张泊明518370910136 merged commit a042ac01ea into master 2024-05-18 02:50:13 +08:00
Sign in to join this conversation.
No description provided.