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-judgeis not started on the server so drone does not work?Also the tested score was -200 locally but -202 on the server😹
go-judgerestarted 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
16a59a7fc2this might work for clang-tidy
When running
clang-tidyon 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-tidytries 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=ONwhen setting upcmake.eg. in 482 p2 i had a custom compile function doing
DRONEoption was to workaround the server and JOJ which used different versions of clang (no c++20 on joj)CMAKE_EXPORT_COMPILE_COMMANDS=ONoption 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-tidywe 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.jsonusingbear -- makeif 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.txtfile 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
-pflag 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 = trueand all files are copied@bomingzh
Everything should run inside sandbox.
compile_commands.jsonshould 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
copyOutCachedandcopyInCachedin 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
DRONEoption 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 -cto 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
/wby 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-tidybuilt in rules based on Libtooling are usually very strong. An alternative approach can be checkingexecutorResult.exitStatusif 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 ofdummybinary.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
resultstatusparser 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