Skip to content

Commit

Permalink
daca@home: use --unsafe-exitcode (#5772)
Browse files Browse the repository at this point in the history 
the daca script will think that analysis crashed otherwise.
  • Loading branch information
@danmar
danmar committed Dec 17, 2023
1 parent 7c316fb commit 150ca20
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion tools/donate_cpu_lib.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,7 @@ def scan_package(cppcheck_path, source_path, libraries, capture_callstack=True):
# TODO: temporarily disabled timing information - use --showtime=top5_summary when next version is released
# TODO: remove missingInclude disabling when it no longer is implied by --enable=information
# Reference for GNU C: https://gcc.gnu.org/onlinedocs/cpp/Common-Predefined-Macros.html
options = libs + ' --check-library --inconclusive --enable=style,information --inline-suppr --disable=missingInclude --suppress=unmatchedSuppression --template=daca2'
options = libs + ' --unsafe-exitcode --check-library --inconclusive --enable=style,information --inline-suppr --disable=missingInclude --suppress=unmatchedSuppression --template=daca2'
options += ' --debug-warnings --suppress=autoNoType --suppress=valueFlowBailout --suppress=bailoutUninitVar --suppress=symbolDatabaseWarning'
options += ' -D__GNUC__ --platform=unix64'
options_rp = options + ' -rp={}'.format(dir_to_scan)
Expand Down

13 comments on commit 150ca20

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please always update the version number.

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danmar If the executable fails without that option than daca is completely broken right now and will be for months to come since the clients scripts are not updated very fast...or at all... That needs to be changed or reverted. It's also not a good change to introduce so close to a release.

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also probably break every integration of Cppcheck as it bypasses --error-exitcode (which is quite bad to begin with). That should be opt-in by default and only changed after a long transition period.

This is similar to my proposed change of --enable=style which was veto'd by use because of similar impact to integration - and I reckon the unsafe exitcode is much worse as it will actually break things instead of "just" leading to missing findings.

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somehow the "safe" cppcheck will have to return error exit code if there was critical errors.

I also hate to introduce this now so close to the release. But we are running a safety certification.. we would like to complete that.

This will also probably break every integration of Cppcheck as it bypasses --error-exitcode (which is quite bad to begin with).

I think you exaggerate. Are you saying that users have critical errors usually and wants that the exit code is 0 then?

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I revert the fix with #5775 temporarily... but I believe an update will be needed for cppcheck-2.13.. it is a safety problem if --suppress="*" will suppress all errors including critical errors and cppcheck will say that all went fine!

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the clients scripts are not updated very fast...or at all...

I thought updating the client version would somehow force the restart :-(

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you exaggerate. Are you saying that users have critical errors usually and wants that the exit code is 0 then?

That depends. I assume the usual integration (as in not just running the CLI) would be via the XML output and in that case the exitcode would be unimportant as you need to ignore it to interpret the XML results. If those contain some findings or errors that would result in a build failure - depending on the integration. In an IDE it would break the integration. In a regular CI it would probably be okay. If you have something like sonarqube which collects the data from various sources it would break things. It's also really awkward to handle if you would to be version agnostic.

That's why I suggested the additional critical or fatal severity which would integrate within the existing handling. I am not 100% sure how to adjust that to get the desired output though. But having one workflow instead of two should make things simpler.

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm.. just thinking.. if we want to keep the exitcode behavior exactly as it is I guess we could add an option --safe-exitcode and then we'll write in the safety manual that this option must be used. With that option a critical error will mean that the exitcode is non-zero. For the safety certification.. I don't know if it's acceptable but it's better than current behavior. I will not support that safety compliance reports are generated through the cli anyway; they will only be generated through the GUI.

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I suggested the additional critical or fatal severity which would integrate within the existing handling.

It of course makes sense. It's just that every parser has to be tweaked to handle those.. so some kind of nice transition would be nice. But yes.. here I was changing the interface very quickly myself..

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I believe an update will be needed for cppcheck-2.13

That's a bit too short-term as it needs to be done properly as we cannot flip-flop on opens. It's already a pain getting existing things in order and we still lack the proper framework for that. I guess I will get a draft together for the next version but I am not sure that can be put in place yet. Also the changes will probably too big to simply cherry-pick them for a 2.13.x safety release.

I thought updating the client version would somehow force the restart :-(

We have no auto-update yet (see ). Also as per instructions you pull a version and run that - and that's it. There's no mention that you need to update that. That's also why I want to move those options to the server-side so there is no need to update the client in such cases - see #5420).

Unfortunately there's too many things to work on which progress in different speeds and my condition isn't helping either. Also with some reviews taking as long as they do drags some things out much longer out than they have to.

hmm.. just thinking.. if we want to keep the behavior exactly as it is I guess we could add an option --safe-exitcode and then we'll write in the safety manual that this option must be used.

I think the whole safety/critical/checkers stuff should simply be opt-in for now and documented as work in progress. That way people which opt-in know that it is changing.

Also a problematic thing about this is that some of those critical errors are also because of bugs in Cppcheck and not simply because of missing configuration. That will cause people to just suppress/disable it since it causes issues and will never look at it. unmatchedSuppression being partially broken is not helping either in this case. To be honest there's still too many issues overall for this to be actually useful or usable.

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am usually an advocate of "secure by default" configuration with opt-out options which people can use to make themselves vulnerable knowingly but unfortunately that doesn't work well if you have legacy (existing) implementations/installations. There's definitely people out there which are still running 1.x versions.

That's why you need some long transition periods. Especially because since regular/non-rolling distros will ship with a certain version of something and will most likely stick with that until it goes EOL. Depending on the user we might still have people that are running years-old versions in production and depending on the way they update that grace period might not even be enough. But in that case they should expect breakage so for those cases it needs to be properly documented. That's why we really should get control of the Snap/Flatpark/etc. package and get it up-to-date so users can always easily run the latest version.

@danmar
Copy link
Owner Author

@danmar danmar commented on 150ca20 Dec 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok so well a --safe-exitcode feels safe for you then? It's an opt-in option so shouldn't affect existing integrations.

@firewave
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Make it opt-in instead. I will take a closer look at the big picture next week.

Please sign in to comment.