build: use cargo messages to locate built cdylib

davidhewitt · davidhewitt · commit 14732c2e9703 · 2022-07-24T21:58:27.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,12 +1,15 @@
 # Changelog
 
-## 1.4.1 (2022-07-05)
+## Unreleased
+### Changed
+- Locate cdylib artifacts by handling messages from cargo instead of searching target dir (fixes build on MSYS2). [#267](https://github.com/PyO3/setuptools-rust/pull/267)
+
 
+## 1.4.1 (2022-07-05)
 ### Fixed
 - Fix crash when checking Rust version. [#263](https://github.com/PyO3/setuptools-rust/pull/263)
 
 ## 1.4.0 (2022-07-05)
-
 ### Packaging
 - Increase minimum `setuptools` version to 62.4. [#246](https://github.com/PyO3/setuptools-rust/pull/246)
 
@@ -25,7 +28,6 @@
 - If the sysconfig for `BLDSHARED` has no flags, `setuptools-rust` won't crash anymore. [#241](https://github.com/PyO3/setuptools-rust/pull/241)
 
 ## 1.3.0 (2022-04-26)
-
 ### Packaging
 - Increase minimum `setuptools` version to 58. [#222](https://github.com/PyO3/setuptools-rust/pull/222)
 
diff --git a/examples/namespace_package/Cargo.toml b/examples/namespace_package/Cargo.toml
@@ -4,7 +4,7 @@ version = "0.1.0"
 edition = "2018"
 
 [lib]
-crate-type = ["cdylib"]
+crate-type = ["cdylib", "rlib"]
 
 [dependencies]
 pyo3 = { version = "0.16.5", features = ["extension-module"] }
diff --git a/setuptools_rust/_utils.py b/setuptools_rust/_utils.py
@@ -1,9 +1,17 @@
 import subprocess
 
 
-def format_called_process_error(e: subprocess.CalledProcessError) -> str:
+def format_called_process_error(
+    e: subprocess.CalledProcessError,
+    *,
+    include_stdout: bool = True,
+    include_stderr: bool = True,
+) -> str:
     """Helper to convert a CalledProcessError to an error message.
 
+    If `include_stdout` or `include_stderr` are True (the default), the
+    respective output stream will be added to the error message (if
+    present in the exception).
 
     >>> format_called_process_error(subprocess.CalledProcessError(
     ...     777, ['ls', '-la'], None, None
@@ -14,18 +22,22 @@ def format_called_process_error(e: subprocess.CalledProcessError) -> str:
     ... ))
     "`cargo 'foo bar'` failed with code 1\\n-- Output captured from stdout:\\nmessage"
     >>> format_called_process_error(subprocess.CalledProcessError(
+    ...     1, ['cargo', 'foo bar'], 'message', None
+    ... ), include_stdout=False)
+    "`cargo 'foo bar'` failed with code 1"
+    >>> format_called_process_error(subprocess.CalledProcessError(
     ...    -1, ['cargo'], 'stdout', 'stderr'
     ... ))
     '`cargo` failed with code -1\\n-- Output captured from stdout:\\nstdout\\n-- Output captured from stderr:\\nstderr'
     """
     command = " ".join(_quote_whitespace(arg) for arg in e.cmd)
     message = f"`{command}` failed with code {e.returncode}"
-    if e.stdout is not None:
+    if include_stdout and e.stdout is not None:
         message += f"""
 -- Output captured from stdout:
 {e.stdout}"""
 
-    if e.stderr is not None:
+    if include_stderr and e.stderr is not None:
         message += f"""
 -- Output captured from stderr:
 {e.stderr}"""
diff --git a/setuptools_rust/build.py b/setuptools_rust/build.py
@@ -1,6 +1,7 @@
 from __future__ import annotations
 
 import glob
+import json
 import os
 import platform
 import shutil
@@ -15,7 +16,8 @@
     DistutilsPlatformError,
 )
 from distutils.sysconfig import get_config_var
-from typing import Dict, List, NamedTuple, Optional, Set, Tuple, cast
+from pathlib import Path
+from typing import Dict, Iterable, List, NamedTuple, Optional, Set, Tuple, cast
 
 from setuptools.command.build import build as CommandBuild  # type: ignore[import]
 from setuptools.command.build_ext import build_ext as CommandBuildExt
@@ -139,11 +141,6 @@ def build_extension(
         quiet = self.qbuild or ext.quiet
         debug = self._is_debug_build(ext)
 
-        # Find where to put the temporary build files created by `cargo`
-        target_dir = _base_cargo_target_dir(ext, quiet=quiet)
-        if target_triple is not None:
-            target_dir = os.path.join(target_dir, target_triple)
-
         cargo_args = self._cargo_args(
             ext=ext, target_triple=target_triple, release=not debug, quiet=quiet
         )
@@ -154,7 +151,14 @@ def build_extension(
             rustflags.extend(["-C", "linker=" + linker])
 
         if ext._uses_exec_binding():
-            command = [self.cargo, "build", "--manifest-path", ext.path, *cargo_args]
+            command = [
+                self.cargo,
+                "build",
+                "--manifest-path",
+                ext.path,
+                "--message-format=json-render-diagnostics",
+                *cargo_args,
+            ]
 
         else:
             rustc_args = [
@@ -184,6 +188,7 @@ def build_extension(
                 self.cargo,
                 "rustc",
                 "--lib",
+                "--message-format=json-render-diagnostics",
                 "--manifest-path",
                 ext.path,
                 *cargo_args,
@@ -209,13 +214,17 @@ def build_extension(
         try:
             # If quiet, capture all output and only show it in the exception
             # If not quiet, forward all cargo output to stderr
-            stdout = subprocess.PIPE if quiet else sys.stderr.fileno()
             stderr = subprocess.PIPE if quiet else None
-            subprocess.run(
-                command, env=env, stdout=stdout, stderr=stderr, text=True, check=True
+            cargo_messages = subprocess.check_output(
+                command,
+                env=env,
+                stderr=stderr,
+                text=True,
             )
         except subprocess.CalledProcessError as e:
-            raise CompileError(format_called_process_error(e))
+            # Don't include stdout in the formatted error as it is a huge dump
+            # of cargo json lines which aren't helpful for the end user.
+            raise CompileError(format_called_process_error(e, include_stdout=False))
 
         except OSError:
             raise DistutilsExecError(
@@ -226,60 +235,64 @@ def build_extension(
         # Find the shared library that cargo hopefully produced and copy
         # it into the build directory as if it were produced by build_ext.
 
-        profile = ext.get_cargo_profile()
-        if profile:
-            # https://doc.rust-lang.org/cargo/reference/profiles.html
-            if profile in {"dev", "test"}:
-                profile_dir = "debug"
-            elif profile == "bench":
-                profile_dir = "release"
-            else:
-                profile_dir = profile
-        else:
-            profile_dir = "debug" if debug else "release"
-        artifacts_dir = os.path.join(target_dir, profile_dir)
         dylib_paths = []
+        package_id = ext.metadata(quiet=quiet)["resolve"]["root"]
 
         if ext._uses_exec_binding():
+            # Find artifact from cargo messages
+            artifacts = _find_cargo_artifacts(
+                cargo_messages.splitlines(),
+                package_id=package_id,
+                kind="bin",
+            )
             for name, dest in ext.target.items():
                 if not name:
                     name = dest.split(".")[-1]
-                exe = sysconfig.get_config_var("EXE")
-                if exe is not None:
-                    name += exe
 
-                path = os.path.join(artifacts_dir, name)
-                if os.access(path, os.X_OK):
-                    dylib_paths.append(_BuiltModule(dest, path))
-                else:
+                try:
+                    artifact_path = next(
+                        artifact
+                        for artifact in artifacts
+                        if Path(artifact).with_suffix("").name == name
+                    )
+                except StopIteration:
                     raise DistutilsExecError(
-                        "Rust build failed; "
-                        f"unable to find executable '{name}' in '{artifacts_dir}'"
+                        f"Rust build failed; unable to locate executable '{name}'"
                     )
-        else:
-            platform = sysconfig.get_platform()
-            if "win" in platform:
-                dylib_ext = "dll"
-            elif platform.startswith("macosx"):
-                dylib_ext = "dylib"
-            elif "wasm32" in platform:
-                dylib_ext = "wasm"
-            else:
-                dylib_ext = "so"
-
-            wildcard_so = "*{}.{}".format(ext.get_lib_name(quiet=quiet), dylib_ext)
 
-            try:
-                dylib_paths.append(
-                    _BuiltModule(
-                        ext.name,
-                        next(glob.iglob(os.path.join(artifacts_dir, wildcard_so))),
+                if os.environ.get("CARGO") == "cross":
+                    artifact_path = _replace_cross_target_dir(
+                        artifact_path, ext, quiet=quiet
                     )
+
+                dylib_paths.append(_BuiltModule(dest, artifact_path))
+        else:
+            # Find artifact from cargo messages
+            artifacts = tuple(
+                _find_cargo_artifacts(
+                    cargo_messages.splitlines(),
+                    package_id=package_id,
+                    kind="cdylib",
                 )
-            except StopIteration:
+            )
+            if len(artifacts) == 0:
+                raise DistutilsExecError(
+                    "Rust build failed; unable to find any build artifacts"
+                )
+            elif len(artifacts) > 1:
                 raise DistutilsExecError(
-                    f"Rust build failed; unable to find any {wildcard_so} in {artifacts_dir}"
+                    f"Rust build failed; expected only one build artifact but found {artifacts}"
+                )
+
+            artifact_path = artifacts[0]
+
+            if os.environ.get("CARGO") == "cross":
+                artifact_path = _replace_cross_target_dir(
+                    artifact_path, ext, quiet=quiet
                 )
+
+            # guaranteed to be just one element after checks above
+            dylib_paths.append(_BuiltModule(ext.name, artifact_path))
         return dylib_paths
 
     def install_extension(
@@ -668,23 +681,9 @@ def _prepare_build_environment(cross_lib: Optional[str]) -> Dict[str, str]:
     if cross_lib:
         env.setdefault("PYO3_CROSS_LIB_DIR", cross_lib)
 
-    env.pop("CARGO", None)
     return env
 
 
-def _base_cargo_target_dir(ext: RustExtension, *, quiet: bool) -> str:
-    """Returns the root target directory cargo will use.
-
-    If --target is passed to cargo in the command line, the target directory
-    will have the target appended as a child.
-    """
-    target_directory = ext._metadata(quiet=quiet)["target_directory"]
-    assert isinstance(
-        target_directory, str
-    ), "expected cargo metadata to contain a string target directory"
-    return target_directory
-
-
 def _is_py_limited_api(
     ext_setting: Literal["auto", True, False],
     wheel_setting: Optional[_PyLimitedApi],
@@ -771,3 +770,64 @@ def _split_platform_and_extension(ext_path: str) -> Tuple[str, str, str]:
     # rust.cpython-38-x86_64-linux-gnu to (rust, .cpython-38-x86_64-linux-gnu)
     ext_path, platform_tag = os.path.splitext(ext_path)
     return (ext_path, platform_tag, extension)
+
+
+def _find_cargo_artifacts(
+    cargo_messages: List[str],
+    *,
+    package_id: str,
+    kind: str,
+) -> Iterable[str]:
+    """Identifies cargo artifacts built for the given `package_id` from the
+    provided cargo_messages.
+
+    >>> list(_find_cargo_artifacts(
+    ...    [
+    ...        '{"some_irrelevant_message": []}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_id","target":{"kind":["cdylib"]},"filenames":["/some/path/baz.so"]}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_id","target":{"kind":["cdylib", "rlib"]},"filenames":["/file/two/baz.dylib", "/file/two/baz.rlib"]}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_other_id","target":{"kind":["cdylib"]},"filenames":["/not/this.so"]}',
+    ...    ],
+    ...    package_id="some_id",
+    ...    kind="cdylib",
+    ... ))
+    ['/some/path/baz.so', '/file/two/baz.dylib']
+    >>> list(_find_cargo_artifacts(
+    ...    [
+    ...        '{"some_irrelevant_message": []}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_id","target":{"kind":["cdylib"]},"filenames":["/some/path/baz.so"]}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_id","target":{"kind":["cdylib", "rlib"]},"filenames":["/file/two/baz.dylib", "/file/two/baz.rlib"]}',
+    ...        '{"reason":"compiler-artifact","package_id":"some_other_id","target":{"kind":["cdylib"]},"filenames":["/not/this.so"]}',
+    ...    ],
+    ...    package_id="some_id",
+    ...    kind="rlib",
+    ... ))
+    ['/file/two/baz.rlib']
+    """
+    for message in cargo_messages:
+        # only bother parsing messages that look like a match
+        if "compiler-artifact" in message and package_id in message and kind in message:
+            parsed = json.loads(message)
+            # verify the message is correct
+            if (
+                parsed.get("reason") == "compiler-artifact"
+                and parsed.get("package_id") == package_id
+            ):
+                for artifact_kind, filename in zip(
+                    parsed["target"]["kind"], parsed["filenames"]
+                ):
+                    if artifact_kind == kind:
+                        yield filename
+
+
+def _replace_cross_target_dir(path: str, ext: RustExtension, *, quiet: bool) -> str:
+    """Replaces target director from `cross` docker build with the correct
+    local path.
+
+    Cross artifact messages and metadata contain paths from inside the
+    dockerfile; invoking `cargo metadata` we can work out the correct local
+    target directory.
+    """
+    cross_target_dir = ext._metadata(cargo="cross", quiet=quiet)["target_directory"]
+    local_target_dir = ext._metadata(cargo="cargo", quiet=quiet)["target_directory"]
+    return path.replace(cross_target_dir, local_target_dir)
diff --git a/setuptools_rust/extension.py b/setuptools_rust/extension.py
diff --git a/tests/test_extension.py b/tests/test_extension.py
