clang-tidy parser and executor #26
No reviewers
Labels
No Label
bug
component
executor
component
framework
component
parser
component
UI
duplicate
enhancement
help wanted
invalid
priority
p0
priority
p1
priority
p2
priority
p3
question
wontfix
No Milestone
No project
No Assignees
3 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: JOJ/JOJ3#26
Loading…
Reference in New Issue
Block a user
No description provided.
Delete Branch "clang-tidy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
TODO:
==
when comparing keywords. We may want to use other comparing strategies if necessary (string.Contains()
?)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@ -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
@ -0,0 +1,262 @@
#include <vector>
need rebase and use the latest test convention
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.
@ -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.
@ -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.
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.
A current version of comment:
Test results summary
I followed the version in ve482 issue
16a59a7fc2
this might work for clang-tidy
When running
clang-tidy
on a single file, there are always errors like this: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:
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 upcmake
.eg. in 482 p2 i had a custom compile function doing
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.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
usingbear -- make
if we only useMakefile
.The commands above works, and when I call clang-tidy in my command line, the database error resolves, only two lines of error persists:
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:
Error occurs:
The error goes the same whether the
-p
flag isbuild
./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 usedcopyInCwd = true
and all files are copied@bomingzh
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 workIf I seperate the two commands into two stages and call the sandbox twice, the generated files won't be kept and shared.
Check
copyOutCached
andcopyInCached
in https://focs.ji.sjtu.edu.cn/git/FOCS-dev/JOJ3-examples/src/branch/compile/success/conf.tomli 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 😂)
not sure why you have this absolute path? have you hardcoded it somewhere or this is teh path "inside the sandbox"?
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 :-)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
For some simple commands, we can just call
bash -c
to do so.@bomingzh All resolved, can you have another review to see if it is ready to merge?
@ -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.@ -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?
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.
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.
All fixed @bomingzh
@ -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.Sorry that was a mistake
@ -0,0 +186,4 @@
}
}
// func generate_fingerprint(message ClangMessage) string {
I think we can completely remove this part.
@ -0,0 +37,4 @@
Trace: extract_trace(message),
Severity: extract_severity(message),
Fingerprint: "",
// Fingerprint: generate_fingerprint(message),
ditto
@ -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.
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:
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 checkingexecutorResult.exitStatus
if we know it will only return 0 and 1.OK
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 ofdummy
binary.I think you can run two steps in one stage. It will be something like this:
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.Interestingly, when I remove the "prepare" stage and move those commands together, clang-tidy report is different: -180 -> -185, and one more unclassified keyword
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.
All naming issues resolved
WIP: clang-tidy parser and executorto clang-tidy parser and executor