test: check for valgrind presence and set appropriate exit flags #17732

perhaps construct this string outside of the arg array assignment, or do:

 "--exit-on-first-error={}".format("yes" if exit_on_first_error else "no"),

I haven't tested this, but I expect --exit-on-first-error=no to throw an error on versions that don't have this option.

Many thanks! I am going through the comments and will adapt the code accordingly.

Will test it on my macOS Catalina, because valgrind can't be installed there. :)

You mean the previous comment? This one is easiest to test on e.g. a fresh Ubuntu 18.04 machine, which has valgrind 3.13. You can then compile it from source and install 3.15 to see the difference.

Yeah, I have one cheap VPS running, with Ubuntu 18.04. Will follow your recommendation. Thanks!

--exit-on-first-error=yes should always be used if the option --exit-on-first-error is supported :)

@jonatack Would prefer your solution, but because not all old versions support this flag I'll have to stick with the other variant. Will basically return an empty string when the flag should not be active, because this is the default for some older versions anyway.

--exit-on-first-error=yes should always be used if the option --exit-on-first-error is supported :)

But the original discussion was about disabling it for v3.15. Or am I missing something?

I'd rename exit_on_first_error to supports_exit_on_first_error for clarity.

Done!

I'd rename exit_on_first_error to supports_exit_on_first_error for clarity.

Then I will also put the expected version (major, minor) to 3.14, ok? If I understand @practicalswift correctly, every version that supports this flag should also have it activated by default.

If I understand @practicalswift correctly, every version that supports this flag should also have it activated by default.

Correct! :)

It is supported by Valgrind 3.14 and above.

(Unfortunately I said the wrong version number in the linked issue. I've now corrected that. Sorry for the confusion :))

This won't throw an error if --valgrind is used on a system without valgrind.

This will create an unnecessary valgrind execution in the normal case when Valgrind is not used (no --valgrind).

Better to do it as if use_valgrind and … instead :)

Yes, indeed. Thanks!

nit: suggestion for readability, format the args array like in line 90 above:

-            self.args = ["valgrind", "--suppressions={}".format(suppressions_file),
-                         "--gen-suppressions=all",
-                         "--exit-on-first-error=yes" if exit_on_first_error else "--exit-on-first-error=no",
-                         "--error-exitcode=1", "--quiet"] + self.args
+            self.args = [
+                "valgrind",
+                "--suppressions={}".format(suppressions_file),
+                "--gen-suppressions=all",
+                "--exit-on-first-error=yes" if exit_on_first_error else "--exit-on-first-error=no",
+                "--error-exitcode=1",
+                "--quiet",
+            ] + self.args

Yes! Thanks :)

I believe this was added in 3.14: https://stackoverflow.com/a/50289059

Will adapt!

Will then simply produce an empty string if the flag should not be active, because it's "no" anyway for versions that support it.

Like: "--exit-on-first-error=yes" if exit_on_first_error else ""

Yes, I have valgrind-3.14.0 installed on debian, and valgrind --help does have the option for --exit-on-first-error=no|yes. Will test the updated version of this PR!

Just force pushed changes. :)

suggest wrapping long lines like here and the docstring just above

I think this can be simplified. Only one execution of valgrind --version should be needed to a.) check if Valgrind is present and b.) to parse what version it is, so try to merge the logic of is_valgrind_present and should_exit_valgrind_on_first_error.

I suggest get_valgrind_version which could return the tuple (valgrind_major, valgrind_minor) if valgrind is installed or None otherwise.

Something like this:

valgrind_version = get_valgrind_version()
if valgrind_version and valgrind_version >= (3, 14):
    # add --exit-on-first-error=true

:)

suggest hoisting these two magic values to constants

if version.startswith("valgrind-") :)

Believe it or not, I was just typing the same changes. Thanks!

nit: VALGRIND_MINIMUM_VERSION_MAJOR

nit: "or None if not found"

I'd move all this below if use_valgrind. Don't forget to throw if valgrind_version == None

Ok, will do it. Many thanks!

The scope can be narrowed for these and the meaning could be clarified by doing it like this on L103 instead :)

MIN_VALGRIND_VERSION_EARLY_EXIT = (3, 14)
if valgrind_version and valgrind_version >= MIN_VALGRIND_VERSION_EARLY_EXIT:

Use shutil.which("valgrind") to avoid shelling out unnecessarily :)

Yup, much better!

1. Have you seen instances where the Valgrind version identifier contains b or v? :) If not, perhaps the simpler version_info.split(".") could do?

2. Check the indentation here so that it matches the four spaces used in the rest of this file :) Applies throughout this PR.

I must admit that I wasn't sure abut valgrind's versioning. But I will adapt the code of course. Will also update the indentations.

I think the logic is wrong here. This will only run valgrind if valgrind_version >= MIN_VALGRIND_VERSION_EARLY_EXIT :)

The only impact valgrind_version >= MIN_VALGRIND_VERSION_EARLY_EXIT should have is to enable --exit-on-first-error=yes. self.args should be set both for old and new versions of Valgrind :)

Yeah, just saw it. Will reorganize the code.

I think you want to do this check right after valgrind_version = self.get_valgrind_version(). With the current logic the exception will be thrown also in the case where valgrind is present (but with an older version).

I suggest testing the PR with the following alternatives to make sure all code paths are working as expected:

1.) Without valgrind in PATH 2.) With new valgrind version (>= 3.14) in PATH 3.) With old valgrind version (< 3.14) in PATH

:)

Suggested alternative:

def get_valgrind_version(self):
    """Returns valgrind version tuple (major, minor), or None if valgrind is not found"""
    if not shutil.which("valgrind"):
        return None
    version = (
        subprocess.run(["valgrind", "--version"], stdout=subprocess.PIPE)
        .stdout.decode("utf-8")
        .rstrip()
        .split("valgrind-")[1:][0]
    )
    return tuple(int(i) for i in version.split(".")[:2])

Much better.

This can be reduced to if valgrind_version >= MIN_VALGRIND_VERSION_EARLY_EXIT: since the not valgrind_version case is handled on L103.

nit: alphabetical order

consider extracting this to a function like below, or just put "--exit-on-first-error=yes" if version >= MIN_VALGRIND_VERSION_EARLY_EXIT else "" in the array after all :)

-            exit_on_first_error_flag = ""
-            if valgrind_version >= MIN_VALGRIND_VERSION_EARLY_EXIT:
-                exit_on_first_error_flag = "--exit-on-first-error=yes"

             self.args = ["valgrind",
                             "--suppressions={}".format(suppressions_file),
                             "--gen-suppressions=all",
-                            exit_on_first_error_flag,
+                            self.valgrind_early_exit(valgrind_version),
 
+    def valgrind_early_exit(self, version):
+        return "--exit-on-first-error=yes" if version >= MIN_VALGRIND_VERSION_EARLY_EXIT else ""

Ok, let's change it! :)

Suggest:

-            self.args = ["valgrind",
-                            "--suppressions={}".format(suppressions_file),
-                            "--gen-suppressions=all",
-                            exit_on_first_error_flag,
-                            "--error-exitcode=1",
-                            "--quiet"] + self.args
+            self.args = [
+                "valgrind",
+                "--suppressions={}".format(suppressions_file),
+                "--gen-suppressions=all",
+                self.valgrind_early_exit(valgrind_version),
+                "--error-exitcode=1",
+                "--quiet",
+            ] + self.args

Also done. Will do another force push. :)

nit: s/Returns/Return/ like the other docstrings

Ok :)