Skip to content
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.

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

Implement new sdist feature to download rust toolchain #2177

Draft
wants to merge 19 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
57f35c9
Initial draft to implement downloading rust toolchain during sdist
Owen-CH-Leung Aug 8, 2024
1ec7982
Enhance pep517 write-dist-info command to check for rust toolchain
Owen-CH-Leung Aug 13, 2024
6db3fdf
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 13, 2024
2a721a7
Fix window command
Owen-CH-Leung Aug 13, 2024
7b76520
Merge remote-tracking branch 'origin/impl_sdist_feature_to_install_ru…
Owen-CH-Leung Aug 13, 2024
136fee2
Fix failing cargo fmt and ruff CI jobs
Owen-CH-Leung Aug 13, 2024
908bec0
Fix failing codespell CI jobs
Owen-CH-Leung Aug 13, 2024
c6c7d72
Initial draft to implement downloading rust toolchain during sdist
Owen-CH-Leung Aug 8, 2024
d45b609
Enhance pep517 write-dist-info command to check for rust toolchain
Owen-CH-Leung Aug 13, 2024
978677c
Fix window command
Owen-CH-Leung Aug 13, 2024
620c8f3
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 13, 2024
22f5885
Fix failing cargo fmt and ruff CI jobs
Owen-CH-Leung Aug 13, 2024
e06953f
Fix failing codespell CI jobs
Owen-CH-Leung Aug 13, 2024
ec03119
Merge branch 'impl_sdist_feature_to_install_rust_toolchain' of https:…
Owen-CH-Leung Oct 19, 2024
cc62930
Add CI test to test pip install without toolchain
Owen-CH-Leung Oct 20, 2024
3c78b7c
Merge branch 'main' into impl_sdist_feature_to_install_rust_toolchain
Owen-CH-Leung Oct 20, 2024
bbea38e
Fix fmt & clippy CI job. Use upload & download artifact github action…
Owen-CH-Leung Oct 21, 2024
4b67e92
Fix pip install *.whl
Owen-CH-Leung Oct 21, 2024
0be8c05
Fix windows build
Owen-CH-Leung Oct 21, 2024
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
31 changes: 30 additions & 1 deletion src/build_context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use crate::module_writer::{
};
use crate::project_layout::ProjectLayout;
use crate::python_interpreter::InterpreterKind;
use crate::source_distribution::source_distribution;
use crate::source_distribution::{download_and_execute_rustup, source_distribution};
use crate::target::{Arch, Os};
use crate::{
compile, pyproject_toml::Format, BuildArtifact, Metadata23, ModuleWriter, PyProjectToml,
Expand Down Expand Up @@ -273,6 +273,22 @@ impl BuildContext {
let sdist_path =
source_distribution(self, pyproject, self.excludes(Format::Sdist)?)
.context("Failed to build source distribution")?;
if self.require_rust_toolchain() {
let mut target_path = sdist_path.clone().to_path_buf();
target_path.pop();
target_path.pop();
target_path.push("bin");

fs::create_dir_all(&target_path)
.context("Fail to create directory for installing rust toolchain")?;

let target_path_str = target_path
.to_str()
.context("Fail to construct target path for installing rust toolchain")?;

download_and_execute_rustup(target_path_str, target_path_str)
Copy link
Member

@messense messense Aug 9, 2024

Choose a reason for hiding this comment

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

Skip download if there is already a working rust toolchain (probably need to validate version with rust-version in Cargo.toml and Cargo.toml of every crate dependencies, I think we can get that from cargo metadata output).

.context("Failed to download rust toolchain")?;
}
Ok(Some((sdist_path, "source".to_string())))
}
None => Ok(None),
Expand Down Expand Up @@ -1132,6 +1148,19 @@ impl BuildContext {
}
Ok(wheels)
}
/// Check if user requires to install rust toolchain
///
/// Loop over `build-system.requires` defined in pyproject.toml and see if `rust-toolchain` is provided
Copy link
Member

Choose a reason for hiding this comment

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

This isn't going to work because pip will try to install rust-toolchain package from pypi.

Copy link
Member

Choose a reason for hiding this comment

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

Since we don't have rust-toolchain on pypi yet, we can try to use rust-toolchain.toml / legacy rust-toolchain file or rust-version in Cargo.toml of the binding crate, then fallback to Rust stable.

Copy link
Contributor Author

@Owen-CH-Leung Owen-CH-Leung Aug 9, 2024

Choose a reason for hiding this comment

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

@messense Thanks. So it means like if user provides a rust-toolchain.toml file in the project directory, that would be indicating that user wants to install rust-toolchain ?

Copy link

@padix-key padix-key Aug 9, 2024

Choose a reason for hiding this comment

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

To put my two cents in, I think that the rust toolchain should be present for the build, even if rust-toolchain.tomlis missing in which case a default toolchain would be used. Bonus points if [toolchain] can be specified directly in the pyproject.toml as e.g [tool.maturin.toolchain].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how about an additional command line flag such as maturin sdist --install-rust-toolchain ? (Or maybe when env variable MATURIN_INSTALL_RUST is set to true? ). This will be backward-compatible while keeping maturin flexible for users .

Choose a reason for hiding this comment

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

This would only be suitable in the case the user of the package knows what Maturin, Rust etc. is. However, the reason I brought up issue #2120, is that a typical user just wants to use a package and would probably do not like to think about the build backend of this package.

In other words, it would be nice in my opinion, if a user can install a source distribution of a package that uses maturin as build-system simply via pip install without knowing such internals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@messense @padix-key I've modified prepare_metadata_for_build_wheel to remove the check for rust-toolchain, and add logic in maturin pep517 write-dist-info command to check for toolchain from there ( and install from rustup if not present). Let me know what you think

Choose a reason for hiding this comment

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

I have actually barely insight into the maturin internals, I only know the user side 😃.

Copy link
Contributor

@mbway mbway Oct 21, 2024

Choose a reason for hiding this comment

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

I think automatically downloading an installation script and running it for the user and installing to non-standard locations is undesirable. I would not be happy if this took place on my system without any confirmation at least. I could also see it causing problems for CI systems which may accidentally install rustup each build if misconfigured.
An opt-in command line flag or environment variable seems like a good approach to me. If not passed it could print

rust toolchain (cargo) not found.
Please install it manually or pass `maturin sdist --auto-install-toolchain`

so discoverability wouldn't be an issue for the 'typical user' described as not being aware of what they need to have installed

pub fn require_rust_toolchain(&self) -> bool {
match &self.pyproject_toml {
Some(pyproject_toml) => pyproject_toml
.build_system
.requires
.iter()
.any(|req| req.name.as_ref() == "rust-toolchain"),
None => false,
}
}
}

/// Calculate the sha256 of a file
Expand Down
36 changes: 36 additions & 0 deletions src/source_distribution.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use crate::module_writer::{add_data, ModuleWriter};
use crate::pyproject_toml::SdistGenerator;
use crate::upload::http_agent;
use crate::{pyproject_toml::Format, BuildContext, PyProjectToml, SDistWriter};
use anyhow::{bail, Context, Result};
use cargo_metadata::{Metadata, MetadataCommand, PackageId};
Expand All @@ -8,9 +9,11 @@ use ignore::overrides::Override;
use normpath::PathExt as _;
use path_slash::PathExt as _;
use std::collections::HashMap;
use std::io::copy;
use std::path::{Path, PathBuf};
use std::process::Command;
use std::str;
use tempfile::NamedTempFile;
use tracing::debug;

/// Path dependency information.
Expand Down Expand Up @@ -735,3 +738,36 @@ where
None
}
}

/// Downloads the rustup installer script and executes it to install rustup
///
/// Inspired by https://github.com/chriskuehl/rustenv
pub fn download_and_execute_rustup(rustup_home: &str, cargo_home: &str) -> Result<()> {
let mut tf = NamedTempFile::new()?;
let agent = http_agent()?;
let response = agent.get("https://sh.rustup.rs").call()?.into_string()?;

copy(&mut response.as_bytes(), &mut tf)?;

#[cfg(unix)]
{
Command::new("sh")
.arg(tf.path())
.arg("-y")
.arg("--no-modify-path")
.env("RUSTUP_HOME", rustup_home)
.env("CARGO_HOME", cargo_home)
.status()?;
}

#[cfg(windows)]
{
Command::new("cmd")
.args(&["/C", tf.path(), "-y", "--no-modify-path"])
.env("RUSTUP_HOME", rustup_home)
.env("CARGO_HOME", cargo_home)
.status()?;
}

Ok(())
}
2 changes: 1 addition & 1 deletion src/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ fn http_agent() -> Result<ureq::Agent, UploadError> {

#[cfg(feature = "rustls")]
#[allow(clippy::result_large_err)]
fn http_agent() -> Result<ureq::Agent, UploadError> {
pub fn http_agent() -> Result<ureq::Agent, UploadError> {
Copy link
Member

Choose a reason for hiding this comment

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

there is a http_agent below that also need to be made public.

use std::sync::Arc;

let builder = ureq::builder().try_proxy_from_env(true);
Expand Down
Loading