Support adding custom env vars

[konstin]

Currently, maturin and setuptools-rust are not self-contained: You need to install rust on the machine in order to build a package. We can improve this by downloading a matching rustup, using it to install cargo and rustc into a temporary directory and using this rust for the build. As long as the platform is supported by rustup, we get a self-contained build that doesn't mess with the host system. You can for example use pip install ... in a fresh python3-slim docker container and any rust dependency with a missing wheel will build and install without additional setup.

To inject this cache installation, we need to set PATH, RUST_HOME and CARGO_HOME whenever we call cargo or rustc.

With these changes, we can make bootstrapping maturin self-contained:

if not os.environ.get("MATURIN_NO_INSTALL_RUST") and not shutil.which("cargo"):
    from puccinialin import setup_rust

    print("Rust not found, installing into a temporary directory")
    extra_env = setup_rust()
    env = {**os.environ, **extra_env}
else:
    env = None

setup(
    version=version,
    cmdclass={"bdist_wheel": bdist_wheel},
    rust_extensions=[RustBin("maturin", args=cargo_args, cargo_manifest_args=["--locked"], env=env)],
    zip_safe=False,
)

"""Support installing rust before compiling (bootstrapping) maturin.

Installing a package that uses maturin as build backend on a platform without maturin
binaries, we install rust in a cache directory if the user doesn't have a rust
installation already. Since this bootstrapping requires more dependencies but is only
required if rust is missing, we check if cargo is present before requesting those
dependencies.

https://setuptools.pypa.io/en/stable/build_meta.html#dynamic-build-dependencies-and-other-build-meta-tweaks
"""

from __future__ import annotations

import os
import shutil
from typing import Any

# noinspection PyUnresolvedReferences
from setuptools.build_meta import *  # noqa:F403


def get_requires_for_build_wheel(_config_settings: dict[str, Any] = None) -> list[str]:
    if not os.environ.get("MATURIN_NO_INSTALL_RUST") and not shutil.which("cargo"):
        return ["puccinialin"]
    return []


def get_requires_for_build_sdist(_config_settings: dict[str, Any] = None) -> list[str]:
    if not os.environ.get("MATURIN_NO_INSTALL_RUST") and not shutil.which("cargo"):
        return ["puccinialin"]
    return []

[konstin]

The mingw CI failures are also on main

[davidhewitt]

Thanks! Implementation looks fine, though I slightly worry about the complexity.

I tried to look for prior art, as far as I can tell Cython / setup tools doesn't have a supported option and just leaves you to modify os.environ as part of setup.py. Is there a reason why that's not good enough for this use case here?

[davidhewitt]

(Mingw failures I've just resolved in #501. PyPy 3.10 is just flat out broken as far as I can tell until the next PyPy version is released.)

[konstin]

I tried to look for prior art, as far as I can tell Cython / setup tools doesn't have a supported option and just leaves you to modify os.environ as part of setup.py. Is there a reason why that's not good enough for this use case here?

os.environ is global state and i'd prefer to isolate those values, as far as that's possible for environment variables. In my experience that helps with debugging.

Mingw failures I've just resolved in #501. PyPy 3.10 is just flat out broken as far as I can tell until the next PyPy version is released.

Thanks I'll rebase.

[davidhewitt]

os.environ is global state and i'd prefer to isolate those values, as far as that's possible for environment variables. In my experience that helps with debugging.

I agree with this, though I am also willing to be a bit more lax for simplicity's sake in these "build system" components especially when it's sort of ok (IMO) to treat setup.py as an entrypoint setting up build configuration.

I guess the main cost of the isolation (as we see in this PR) is that we have to be careful to pass the env dict to all downstream processes; forgetting to do so once is probably easy to miss in review and will create bugs. Can we use ruff to forbid use of subprocess outside our own private wrapper which requires an Env? (Maybe this setting?)

Is there any place outside of subprocesses where these variables might matter? I think it seems clear enough from docs that we only intend for this to affect rustc / cargo. The only other subprocesses we call (as far as I see from a quick scan) are strip and lipo (on various platforms), but I think there is little need for those to be affected by this option.

I think as long as we have ways to avoid this getting out of sync later with some tests or lints, this gets 👍 from me.

[konstin]

Rebase to fix the tests and fixed the review comment

[davidhewitt]

I guess the main cost of the isolation (as we see in this PR) is that we have to be careful to pass the env dict to all downstream processes; forgetting to do so once is probably easy to miss in review and will create bugs. Can we use ruff to forbid use of subprocess outside our own private wrapper which requires an Env? (Maybe this setting?)

Any ideas what we can do to mitigate this concern? Does my suggestion to use ruff seem viable?

[konstin]

That's a though problem, i can't figure out a good solution: The subprocess module has a number of functions that create a subprocess, and we don't want to ban using them, we only want to ensure that env= is set explicitly so that you don't accidentally forget the environment (or set env=None if it's not a rustc/cargo call). I don't see how to do that with lint.flake8-tidy-imports, did you have something specific in mind?

[cassiersg]

As an independent use-case for this feature: I just needed this feature today: I am building two RustExtensions in a single setup(...), and the extensions need different RUSTFLAGS env vars. As far as I can tell, this is currently impossible. (I ended up monkey-patching _prepare_build_environment with some inspect magic.)

[cassiersg]

@konstin The subprocess doc actually says:

The recommended approach to invoking subprocesses is to use the run() function for all use cases it can handle. For more advanced use cases, the underlying Popen interface can be used directly.

Therefore, it might not be an issue to ban the other functions (the "older high-level API". Then, it remains to provide a wrapper for run as proposed by @davidhewitt (and maybe ban Popen - or we consider that it's use is unlikely / shows explicit intent).

Please let me know if I can help with this PR.

[davidhewitt]

Sorry for the long delay, I pushed a commit to add the lint rules as I wanted them. I'll wait for CI to run, and if it's green I'll merge and release.

Thanks for implementing & for the patience! 🙏

[davidhewitt]

test-cross is failing on main so will fix separately.