Partial fix for #11797: If sourcefile in imported project is missing then report critical error fileNotFound #5498
Conversation
| "File not found", | ||
| "fileNotFound", | ||
| Certainty::normal)); | ||
| mExitCode = 1; |
There was a problem hiding this comment.
We should not arbitrary set the exitcode. Severrity::error will already do this based on --error-exitcode. This will cause the analysis to fail even if the user suppresses this without any indication why.
| unsigned int CppCheck::check(const ImportProject::FileSettings &fs) | ||
| { | ||
| if (!Path::isFile(fs.filename)) { |
There was a problem hiding this comment.
Other projects perform these checks with importproject.cpp so this duplicates logic. Having an controllable logic might be better though. But we should implement and this properly this properly for all projects. We should not introduce more half-baked or inconsistent behavior. We still trying to get rid of such things from years and years ago.
I also think this might be the wrong location to do this. I think this should be reported closer to the actual import but I think we might be lacking common code to place this in.
There was a problem hiding this comment.
We can put it in importproject also. But I wanted to achieve that the analysis will continue on other files that are found. I saw that if a file is missing when compile_commands.json is loaded then all analysis is aborted and that might be unfortunate. And putting this check here means that if the file has been removed after the project was loaded then we still detect this problem and bails out with an error message. Still.. maybe we can tune this and report it if simplecpp fails to read the source file instead.
There was a problem hiding this comment.
I understood the intention. I just want it to be consistent in implementation and test.
We should do in in the import step as we do with Path::acceptFile() so we know this as early as possible and not bail out on the first error. Similar to the changes I did for library and addon loading.
The problem is that we are not able to collect errors there - yet. That's what I added CmdLineLogger for. I want to collect all errors and report them with a proper ID after all options have been processed. But I need a few more things merged and changed first.
So this change is fine for now as we cannot solve it in a better way yet.
There was a problem hiding this comment.
I will post a draft PR soon after my pending stuff has been merged and I finished up most of the executor stuff. If things go well that should be as soon as next week.
There was a problem hiding this comment.
I just want it to be consistent in implementation and test.
hmm yes
I will post a draft PR soon after my pending stuff has been merged and I finished up most of the executor stuff.
ok sure.
If things go well that should be as soon as next week.
I would like to have this in next major release 2.13 so well it's not a hurry yet.
There was a problem hiding this comment.
I just want it to be consistent in implementation and test.
hmm yes
Simply adding tests with missing files for all projects would be sufficient.
| filename, | ||
| Severity::error, | ||
| "File not found", | ||
| "fileNotFound", |
There was a problem hiding this comment.
The error might be too generic. Something more specific would be better since there might be more errors like this coming soon.
| filename, | ||
| Severity::error, | ||
| "File not found", | ||
| "fileNotFound", |
There was a problem hiding this comment.
The error might be too generic. Something more specific would be better since there might be more errors like this coming soon.
| def test_vs_project_missing_file(): | ||
| with open('proj2/proj2.vcxproj', 'rt') as fin: | ||
| s = fin.read() | ||
| with open('proj2/test.vcxproj', 'wt') as fout: |
There was a problem hiding this comment.
We should use tmpdir if we are generating new files but none of the tests here currently do that.
|
thanks @firewave I think these are good review comments. |
As usual I think I sounded a bit harsh but that comes with my condition not being able to think straight and trying to get my point over. 🫤 Sorry about that. |
No description provided.