Skip to content

report internalError when command execution returns errorcode / also some related cleanups and tests#5037

Merged
danmar merged 8 commits intocppcheck-opensource:mainfrom
firewave:internalerror
Aug 31, 2023
Merged

report internalError when command execution returns errorcode / also some related cleanups and tests#5037
danmar merged 8 commits intocppcheck-opensource:mainfrom
firewave:internalerror

Conversation

@firewave
Copy link
Copy Markdown
Collaborator

@firewave firewave commented May 4, 2023

Encountered while investigating https://trac.cppcheck.net/ticket/11708.

This has been like this since the introduction of internalError in b6bcdf2 (almost ten years ago to the day). Logging internal errors which bail out(!) of the analysis simply to std::cout for them possibly never to be seen (and also not affected the exitcode) is pretty bad IMO. They should always be visible.

I also removed the filename from the message as it is already available (and thus redundant) and its existence should be defined by the template.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented May 4, 2023

We probably should clean up the message a bit more. It is pretty redundant. There's even a case where it states again that an internal error happened which would lead to three instances of it in the message.

@firewave firewave force-pushed the internalerror branch 3 times, most recently from 28fb6ff to bd66963 Compare May 5, 2023 19:00
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Jun 8, 2023

I close this draft PR..

With b086873 I change the severity to error and report these in the normal Cppcheck error output.

Feel free to point out if there is something missing..

@danmar danmar closed this Jun 8, 2023
@firewave
Copy link
Copy Markdown
Collaborator Author

Will take a look at the other PR but this contained much more which should still be applied.

@firewave firewave reopened this Jul 28, 2023
@firewave firewave force-pushed the internalerror branch 3 times, most recently from e950017 to ced1b71 Compare August 8, 2023 15:17
@firewave firewave changed the title always report internalError as proper finding some internalError related cleanups and tests Aug 8, 2023
@firewave firewave force-pushed the internalerror branch 2 times, most recently from e770ee9 to 3b6d7ce Compare August 28, 2023 17:44
@firewave firewave marked this pull request as ready for review August 28, 2023 18:00
@firewave
Copy link
Copy Markdown
Collaborator Author

The failing job is caused by not being able to download our own 2.8 source archive from GitHub. It works locally so I assume it is a temporary issue. I will trigger the job again later.

@firewave
Copy link
Copy Markdown
Collaborator Author

firewave commented Aug 29, 2023

The RAII closing of the pipe causes a debug assertion in Visual Studio so I changed the errorhandling:
image

I had that queued up for another PR anyways and will add the proper error reporting in that.

@firewave firewave marked this pull request as draft August 29, 2023 17:07
@firewave firewave changed the title some internalError related cleanups and tests report internalError when command execution returns errorcode / also some related cleanups and tests Aug 29, 2023
@danmar
Copy link
Copy Markdown
Collaborator

danmar commented Aug 29, 2023

I think this looks promising. It seems to me that it's really important that there is some kind of critical error if the addon fails. I'm not sure what behavior you saw, did Cppcheck exit happily?

The RAII closing of the pipe causes a debug assertion in Visual Studio so I changed the errorhandling:

hmm spontanously I don't understand why there would be a debug assertion when the RAII code closes the pipe. Feel free to enlighten me.

@firewave
Copy link
Copy Markdown
Collaborator Author

I think this looks promising. It seems to me that it's really important that there is some kind of critical error if the addon fails. I'm not sure what behavior you saw, did Cppcheck exit happily?

We at least ignored the exitcode of the command. More information coming in a different PR.
I have been cleaning up the addon errorhandling in that but had lots of issues forcing errors so I keep losing motivation very fast.

hmm spontanously I don't understand why there would be a debug assertion when the RAII code closes the pipe. Feel free to enlighten me.

I assumed it was an issue when an invalid handle was passed into the close function but it turns out it asserts when the given executable doesn't exist on open since it tries to close some invalid internal lock within the Windows runtime code.
But the RAII code hid the actual exitcode of the command as mentioned above.

@firewave firewave marked this pull request as ready for review August 30, 2023 09:44
…n with Visual Studio caused by missing command
…and()` and actually fail when the command returns an exitcode / also refactored code a bit and added TODOs
@danmar danmar merged commit ad1caa8 into cppcheck-opensource:main Aug 31, 2023
@firewave firewave deleted the internalerror branch August 31, 2023 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants