Skip to content

build_rust: remove linker handling that broke cross compilation #269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 2, 2022
Merged

build_rust: remove linker handling that broke cross compilation #269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
## 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)
- No longer guess cross-compile environment using `HOST_GNU_TYPE` / `BUILD_GNU_TYPE` sysconfig variables. [#269](https://github.com/PyO3/setuptools-rust/pull/269)

### Fixed
- Fix RustBin build without wheel. [#273](https://github.com/PyO3/setuptools-rust/pull/273)
- Fix RustBin setuptools install. [#275](https://github.com/PyO3/setuptools-rust/pull/275)

Expand Down
154 changes: 8 additions & 146 deletions setuptools_rust/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
import glob
import json
import os
import pkg_resources
import platform
import shutil
import subprocess
Expand All @@ -20,6 +19,7 @@
from pathlib import Path
from typing import Dict, Iterable, List, NamedTuple, Optional, Set, Tuple, cast

import pkg_resources
from setuptools.command.build import build as CommandBuild # type: ignore[import]
from setuptools.command.build_ext import build_ext as CommandBuildExt
from setuptools.command.build_ext import get_abi3_suffix
Expand Down Expand Up @@ -131,23 +131,10 @@ def build_extension(
self, ext: RustExtension, forced_target_triple: Optional[str] = None
) -> List["_BuiltModule"]:

target_info = self._detect_rust_target(forced_target_triple)
if target_info is not None:
target_triple = target_info.triple
cross_lib = target_info.cross_lib
linker = target_info.linker
# We're ignoring target_info.linker_args for now because we're not
# sure if they will always do the right thing. Might help with some
# of the OS-specific logic if it does.

else:
target_triple = None
cross_lib = None
linker = None

target_triple = self._detect_rust_target(forced_target_triple)
rustc_cfgs = get_rustc_cfgs(target_triple)

env = _prepare_build_environment(cross_lib)
env = _prepare_build_environment()

if not os.path.exists(ext.path):
raise DistutilsFileError(
Expand All @@ -163,9 +150,6 @@ def build_extension(

rustflags = []

if linker is not None:
rustflags.extend(["-C", "linker=" + linker])
Comment on lines -166 to -167
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per discussion here I am now 100% in agreement that this was wrong and it is correct to remove linker handling from setuptools-rust.


if ext._uses_exec_binding():
command = [
self.cargo,
Expand Down Expand Up @@ -435,45 +419,12 @@ def _py_limited_api(self) -> _PyLimitedApi:

def _detect_rust_target(
self, forced_target_triple: Optional[str] = None
) -> Optional["_TargetInfo"]:
) -> Optional[str]:
assert self.plat_name is not None
cross_compile_info = _detect_unix_cross_compile_info()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With an appropriate implementation of setuptools_rust that handles all build environment correctly, I still think there's value in a best-effort attempt to recognise the correct rust build target for use-cases like crossenv. My preference would be to not completely remove this, however the heavy-handedness with which it splats the build environment absolutely needs fixing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about the right implementation for _detect_unix_cross_compile_info, but don't have any good ideas right now. I'm also not terribly familiar with rust, but wound up with responsibility for setuptools-rust packaging in Void because it is required by our python3-adblock package as well as python3-cryptography.

For the reasons I've noted earlier, I think looking for *_GNU_TYPE variables in the target sysinfo is wrong because it will reflect the cross or native build environment of the Python interpreter rather than the compilation intent for the package being built with setuptools-rust. The best method here would be determining the native triple for the rust compiler and comparing that to self.target, but I don't know how to determine a native triple short of hacks like looking at uname output.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the reasons I've noted earlier, I think looking for *_GNU_TYPE variables in the target sysinfo is wrong because it will reflect the cross or native build environment of the Python interpreter rather than the compilation intent for the package being built with setuptools-rust. The best method here would be determining the native triple for the rust compiler and comparing that to self.target, but I don't know how to determine a native triple short of hacks like looking at uname output.

That's what GNU autotools do though (look at uname).

However, Yocto project does not generally trust any guessing that the components might try to do: we prefer to pass in all three triplets (host/build/target) explicitly via pre-written config files or command line switches.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, here:
https://git.savannah.gnu.org/cgit/config.git/tree/config.guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the better check for cross-compilation in this project is just "is self.target (read from CARGO_BUILD_TARGET) defined?"

Aside from checking the *_GNU_TYPE macros to pick cross info, all the _detect_unix_cross_compile_info does is attempt to determine the cross_lib path and set a default PYO3_CROSS_LIB_DIR based on it. I'd be OK with preserving that logic. (It determines the wrong value in our Void setup, but because I explicitly set the environment variable to the right value beforehand, it has no ill effect.)

if cross_compile_info is not None:
cross_target_info = cross_compile_info.to_target_info()
if forced_target_triple is not None:
if (
cross_target_info is not None
and not cross_target_info.is_compatible_with(forced_target_triple)
):
self.warn(
f"Forced Rust target `{forced_target_triple}` is not "
f"compatible with deduced Rust target "
f"`{cross_target_info.triple}` - the built package "
f" may not import successfully once installed."
)

# Forcing the target in a cross-compile environment; use
# the cross-compile information in combination with the
# forced target
return _TargetInfo(
forced_target_triple,
cross_compile_info.cross_lib,
cross_compile_info.linker,
cross_compile_info.linker_args,
)
Comment on lines -458 to -463
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On reflection this line is clearly wrong - should have just been return _TargetInfo(forced_target_info)and not carry any of the detected cross-compile with it. (The user is already setting --target externally, they should be totally able to set the rest of the flags in this case.)

elif cross_target_info is not None:
return cross_target_info
else:
raise DistutilsPlatformError(
"Don't know the correct rust target for system type "
f"{cross_compile_info.host_type}. Please set the "
"CARGO_BUILD_TARGET environment variable."
)

elif forced_target_triple is not None:
if forced_target_triple is not None:
# Automatic target detection can be overridden via the CARGO_BUILD_TARGET
# environment variable or --target command line option
return _TargetInfo.for_triple(forced_target_triple)
return forced_target_triple

# Determine local rust target which needs to be "forced" if necessary
local_rust_target = _adjusted_local_rust_target(self.plat_name)
Expand All @@ -485,7 +436,7 @@ def _detect_rust_target(
# check for None first to avoid calling to rustc if not needed
and local_rust_target != get_rust_host()
):
return _TargetInfo.for_triple(local_rust_target)
return local_rust_target

return None

Expand Down Expand Up @@ -575,91 +526,6 @@ class _BuiltModule(NamedTuple):
path: str


class _TargetInfo(NamedTuple):
triple: str
cross_lib: Optional[str]
linker: Optional[str]
linker_args: Optional[str]

@staticmethod
def for_triple(triple: str) -> "_TargetInfo":
return _TargetInfo(triple, None, None, None)

def is_compatible_with(self, target: str) -> bool:
if self.triple == target:
return True

# the vendor field can be ignored, so x86_64-pc-linux-gnu is compatible
# with x86_64-unknown-linux-gnu
if _replace_vendor_with_unknown(self.triple) == target:
return True

return False


class _CrossCompileInfo(NamedTuple):
host_type: str
cross_lib: Optional[str]
linker: Optional[str]
linker_args: Optional[str]

def to_target_info(self) -> Optional[_TargetInfo]:
"""Maps this cross compile info to target info.

Returns None if the corresponding target information could not be
deduced.
"""
# hopefully an exact match
targets = get_rust_target_list()
if self.host_type in targets:
return _TargetInfo(
self.host_type, self.cross_lib, self.linker, self.linker_args
)

# the vendor field can be ignored, so x86_64-pc-linux-gnu is compatible
# with x86_64-unknown-linux-gnu
without_vendor = _replace_vendor_with_unknown(self.host_type)
if without_vendor is not None and without_vendor in targets:
return _TargetInfo(
without_vendor, self.cross_lib, self.linker, self.linker_args
)

return None


def _detect_unix_cross_compile_info() -> Optional["_CrossCompileInfo"]:
# See https://github.com/PyO3/setuptools-rust/issues/138
# This is to support cross compiling on *NIX, where plat_name isn't
# necessarily the same as the system we are running on. *NIX systems
# have more detailed information available in sysconfig. We need that
# because plat_name doesn't give us information on e.g., glibc vs musl.
host_type = sysconfig.get_config_var("HOST_GNU_TYPE")
build_type = sysconfig.get_config_var("BUILD_GNU_TYPE")

if not host_type or host_type == build_type:
# not *NIX, or not cross compiling
return None

if "apple-darwin" in host_type and (build_type and "apple-darwin" in build_type):
# On macos and the build and host differ. This is probably an arm
# Python which was built on x86_64. Don't try to handle this for now.
# (See https://github.com/PyO3/setuptools-rust/issues/192)
return None

stdlib = sysconfig.get_path("stdlib")
assert stdlib is not None
cross_lib = os.path.dirname(stdlib)

bldshared = sysconfig.get_config_var("BLDSHARED")
if not bldshared:
linker = None
linker_args = None
else:
[linker, _, linker_args] = bldshared.partition(" ")

return _CrossCompileInfo(host_type, cross_lib, linker, linker_args)


def _replace_vendor_with_unknown(target: str) -> Optional[str]:
"""Replaces vendor in the target triple with unknown.

Expand All @@ -672,7 +538,7 @@ def _replace_vendor_with_unknown(target: str) -> Optional[str]:
return "-".join(components)


def _prepare_build_environment(cross_lib: Optional[str]) -> Dict[str, str]:
def _prepare_build_environment() -> Dict[str, str]:
"""Prepares environment variables to use when executing cargo build."""

# Make sure that if pythonXX-sys is used, it builds against the current
Expand All @@ -692,10 +558,6 @@ def _prepare_build_environment(cross_lib: Optional[str]) -> Dict[str, str]:
"PYO3_PYTHON": os.environ.get("PYO3_PYTHON", sys.executable),
}
)

if cross_lib:
env.setdefault("PYO3_CROSS_LIB_DIR", cross_lib)
Comment on lines -696 to -697
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correction: this does not "force" PYO3_CROSS_LIB_DIR - setdefault will not overwrite any value already in the environment. So build environments should be untainted by this.

Whether setuptools-rust should be attempting to set this is a separate question. I think it's reasonable for it to make an educated guess if it's on balance helpful for users. As this value comes from the sysconfig, it seems reasonable to me to use that as a first guess.


return env


Expand Down