fix(consume): fixes issue where nethtest would not be recognized as valid t8n on certain systems (#1973)

* fixes issue where nethtest would not be recognized/found on certain systems depending on how it was compiled

Author: felix314159

src/ethereum_clis/clis/nethermind.py

@@ -22,7 +22,8 @@ class Nethtest(EthereumCLI):
     """Nethermind `nethtest` binary base class."""
 
     default_binary = Path("nethtest")
-    detect_binary_pattern = re.compile(r"^\d+\.\d+\.\d+-[a-zA-Z0-9]+(\+[a-f0-9]{40})?$")
+    # new pattern allows e.g. '1.2.3', in the past that was denied
+    detect_binary_pattern = re.compile(r"^\d+\.\d+\.\d+(-[a-zA-Z0-9]+)?(\+[a-f0-9]{40})?$")
     version_flag: str = "--version"
     cached_version: Optional[str] = None
 

src/ethereum_clis/ethereum_cli.py

@@ -8,6 +8,10 @@
 from re import Pattern
 from typing import Any, List, Optional, Type
 
+from pytest_plugins.logging import get_logger
+
+logger = get_logger(__name__)
+
 
 class UnknownCLIError(Exception):
     """Exception raised if an unknown CLI is encountered."""
@@ -78,46 +82,90 @@ def from_binary_path(cls, *, binary_path: Optional[Path], **kwargs) -> Any:
         """
         assert cls.default_tool is not None, "default CLI implementation was never set"
 
+        # ensure provided t8n binary can be found and used
         if binary_path is None:
+            logger.debug("Binary path of provided t8n is None!")
             return cls.default_tool(binary=binary_path, **kwargs)
 
-        resolved_path = Path(os.path.expanduser(binary_path)).resolve()
+        expanded_path = Path(os.path.expanduser(binary_path))
+        logger.debug(f"Expanded path of provided t8n: {expanded_path}")
+
+        resolved_path = expanded_path.resolve()
+        logger.debug(f"Resolved path of provided t8n: {resolved_path}")
+
         if resolved_path.exists():
-            binary_path = resolved_path
-        binary = shutil.which(binary_path)  # type: ignore
+            logger.debug("Resolved path exists")
+            binary = Path(resolved_path)
+        else:
+            logger.debug(
+                f"Resolved path does not exist: {resolved_path}\nTrying to find it via `which`"
+            )
 
-        if not binary:
-            raise CLINotFoundInPathError(binary=binary)
+            # it might be that the provided binary exists in path
+            filename = os.path.basename(resolved_path)
+            binary = shutil.which(filename)  # type: ignore
+            logger.debug(f"Output of 'which {binary_path}': {binary}")
 
-        binary = Path(binary)  # type: ignore[assignment]
+            if binary is None:
+                logger.error(f"Resolved t8n binary path does not exist: {resolved_path}")
+                raise CLINotFoundInPathError(binary=resolved_path)
+
+            assert binary is not None
+            logger.debug(f"Successfully located the path of the t8n binary: {binary}")
+            binary = Path(binary)
 
         # Group the tools by version flag, so we only have to call the tool once for all the
         # classes that share the same version flag
         for version_flag, subclasses in groupby(
             cls.registered_tools, key=lambda x: x.version_flag
         ):
+            logger.debug(
+                f"Trying this `version` flag to determine if t8n supported: {version_flag}"
+            )
+            # adding more logging reveals we check for `-v` twice..
+
             try:
                 result = subprocess.run(
                     [binary, version_flag], stdout=subprocess.PIPE, stderr=subprocess.PIPE
                 )
+                logger.debug(
+                    f"Subprocess:\n\tstdout: {result.stdout}\n\n\n\tstderr: {result.stderr}\n\n\n"  # type: ignore
+                )
+
                 if result.returncode != 0:
-                    raise Exception(f"Non-zero return code: {result.returncode}")
+                    logger.debug(f"Subprocess returncode is not 0! It is: {result.returncode}")
+                    continue  # don't raise exception, you are supposed to keep trying different version flags  # noqa: E501
 
                 if result.stderr:
-                    raise Exception(f"Tool wrote to stderr: {result.stderr.decode()}")
+                    logger.debug(f"Stderr detected: {result.stderr}")  # type: ignore
+                    continue
 
                 binary_output = ""
                 if result.stdout:
                     binary_output = result.stdout.decode().strip()
-            except Exception:
-                # If the tool doesn't support the version flag,
-                # we'll get an non-zero exit code.
+                    # e.g. 1.31.10+f62cfede9b4abfb5cd62d6f138240668620a2b0d should be treated as 1.31.10  # noqa: E501
+                    # if "+" in binary_output:
+                    #     binary_output = binary_output.split("+")[0]
+
+                    logger.debug(f"Stripped subprocess stdout: {binary_output}")
+
+                for subclass in subclasses:
+                    logger.debug(f"Trying subclass {subclass}")
+
+                    if subclass.detect_binary(binary_output):
+                        return subclass(binary=binary, **kwargs)
+
+                    logger.debug(
+                        f"T8n with version {binary_output} does not belong to subclass {subclass}"
+                    )
+
+            except Exception as e:
+                logger.debug(
+                    f"Trying to determine t8n version with flag `{version_flag}` failed: {e}"
+                )
                 continue
-            for subclass in subclasses:
-                if subclass.detect_binary(binary_output):
-                    return subclass(binary=binary, **kwargs)
 
-        raise UnknownCLIError(f"Unknown CLI: {binary_path}")
+        raise UnknownCLIError(f"Unknown CLI: {binary}")
 
     @classmethod
     def detect_binary(cls, binary_output: str) -> bool: