-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
57f35c9
1ec7982
6db3fdf
2a721a7
7b76520
136fee2
908bec0
c6c7d72
d45b609
978677c
620c8f3
22f5885
e06953f
ec03119
cc62930
3c78b7c
bbea38e
4b67e92
0be8c05
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -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) | ||
.context("Failed to download rust toolchain")?; | ||
} | ||
Ok(Some((sdist_path, "source".to_string()))) | ||
} | ||
None => Ok(None), | ||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't going to work because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we don't have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @messense Thanks. So it means like if user provides a There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about an additional command line flag such as There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @messense @padix-key I've modified There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have actually barely insight into the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there is a |
||
use std::sync::Arc; | ||
|
||
let builder = ureq::builder().try_proxy_from_env(true); | ||
|
There was a problem hiding this comment.
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
inCargo.toml
andCargo.toml
of every crate dependencies, I think we can get that fromcargo metadata
output).