As a followup to #17633 we could use better error handling when valgrind executable is missing, or when something else goes wrong (e.g. #17633#pullrequestreview-326402329).
E.g. I ran into (burried in the logs) valgrind: Unknown option: --exit-on-first-error=yes valgrind: Use --help for more information or consult the user manual..
That would have been caught by checking the version number (3.13 doesn't have this flag).
I think that exit-on-first-error can be removed, so that the option can be used on Ubuntu Bionic, and added back (assuming Ubuntu later than Bionic) during debugging when the cause of a failure is not apparent.
when something else goes wrong
We have combine_logs.py, which should already handle all of this
Perhaps valgrind could be run without --exit-on-first-error=yes for users running Valgrind 3.13~3.14~ or older.
Due to the super long run-times I still think it makes sense to use --exit-on-first-error=yes where possible (Valgrind 3.14~3.15~ or later).
Edited: Messed up the version numbers. Sorry about that. --exit-on-first-error is supported by Valgrind 3.14 and above :)
Yeah, auto-detection would be even better
If anyone wants to tackle auto-detection:
>>> subprocess.run(["valgrind", "--version"], \
stdout=subprocess.PIPE).stdout.decode("utf-8").rstrip().split("valgrind-")[1:]
['3.15.0']
>>> tuple(int(i) for i in subprocess.run(["valgrind", "--version"], \
stdout=subprocess.PIPE).stdout.decode("utf-8").rstrip().split("valgrind-")[1:][0].split("."))
(3, 15, 0)
>>> tuple(int(i) for i in subprocess.run(["valgrind", "--version"], \
stdout=subprocess.PIPE).stdout.decode("utf-8").rstrip().split("valgrind-")[1:][0].split(".")) \
>= (3, 14, 0)
True
:)
Going to close this. Anyone wanting to use valgrind should really be using a version newer than 3.13 (latest is 3.19), and focal ships with 3.15. I don't think anyone else has opened an issue about the flag exit-on-first-error flag since this was opened.