Skip to content

Fix compilation bug in toolchain >= 1.70.0 #321

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

Closed
yasuo-ozu wants to merge 1 commit into from
+18 −6
Closed

Fix compilation bug in toolchain >= 1.70.0 #321

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
24 changes: 18 additions & 6 deletions setuptools_rust/build.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
from ._utils import format_called_process_error
from .command import RustCommand
from .extension import Binding, RustBin, RustExtension, Strip
from .rustc_info import get_rust_host, get_rust_target_list, get_rustc_cfgs
from .rustc_info import get_rust_host, get_rust_target_list, get_rustc_cfgs, get_rust_version
from semantic_version import Version


class build_rust(RustCommand):
Expand Down Expand Up @@ -142,6 +143,7 @@ def build_extension(

quiet = self.qbuild or ext.quiet
debug = self._is_debug_build(ext)
is_new_toolchain = get_rust_version() >= Version('1.70.0-nightly')
Copy link
Copy Markdown
Member

@adamreichold adamreichold Mar 29, 2023

Choose a reason for hiding this comment

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

I think a more description name would be helpful (today's new is tomorrow's old). Also do we need to include "nightly" here? Can we compare directly against "1.70.0" or this considered larger than "1.70.0-nightly" by Version?

Copy link
Copy Markdown
Contributor Author

@yasuo-ozu yasuo-ozu Mar 29, 2023

Choose a reason for hiding this comment

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

Thank you. I will withdraw this PR and fix it in another PR.

Copy link
Copy Markdown
Contributor Author

@yasuo-ozu yasuo-ozu Mar 29, 2023

Choose a reason for hiding this comment

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

I wrote Version('1.70.0-nightly') here, because I want is_new_toolchain to be True when get_rust_version() == Version("1.70.0-nightly").
But in fact Version('1.70.0-nightly') >= Version('1.70.0') is False.

Do you think something like the following is better?

rustc_version = get_rust_version()
is_new_toolchain = rustc_version.major > 1 or (rustc_version.major == 1 and rustc_version.minor >= 70)

Copy link
Copy Markdown
Member

@adamreichold adamreichold Mar 29, 2023

Choose a reason for hiding this comment

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

Yes, I think that would be preferable. (Especially if combined with a more descriptive name than "new".)


cargo_args = self._cargo_args(
ext=ext, target_triple=target_triple, release=not debug, quiet=quiet
Expand All @@ -160,11 +162,18 @@ def build_extension(
]

else:
rustc_args = [
"--crate-type",
"cdylib",
*ext.rustc_flags,
]
# If toolchain >= 1.70.0, use '--crate-type' option of cargo.
# See https://github.com/PyO3/setuptools-rust/issues/320
if is_new_toolchain:
rustc_args = [
*ext.rustc_flags,
]
else:
rustc_args = [
"--crate-type",
"cdylib",
*ext.rustc_flags,
]

# OSX requires special linker arguments
if rustc_cfgs.get("target_os") == "macos":
Expand All @@ -189,6 +198,9 @@ def build_extension(
):
rustc_args.extend(["-C", f"link-args=-sSIDE_MODULE=2 -sWASM_BIGINT"])

if is_new_toolchain:
cargo_args.extend(["--crate-type", "cdylib"])

command = [
self.cargo,
"rustc",
Expand Down