From f7f6453287d4243234c3489018e618ce009d1aa4 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Sat, 9 Mar 2024 03:49:57 -0800 Subject: [PATCH] Communicate PEP 517 hook results via files (#2314) ## Summary In #1813, we were failing to install `scikit-image==0.19.3` from source in Python 3.11. Confusingly, though, the trace showed that the build command exited with status 0... The issue is that we get results from the PEP 517 hooks by reading from `stdout` -- that is, we `print` at the end of the script, and parse the printed output on the other side. It turns out that for `scikit-image`, in this case, there was output _after_ the wheel filename: ``` ... no previously-included directories found matching 'doc/gh-pages' adding license file 'LICENSE.txt' writing manifest file 'scikit_image.egg-info/SOURCES.txt' Copying scikit_image.egg-info to build/bdist.macosx-12.6-arm64/wheel/scikit_image-0.19.3-py3.11.egg-info running install_scripts scikit_image-0.19.3-cp311-cp311-macosx_14_0_arm64.whl INFO: ########### EXT COMPILER OPTIMIZATION ########### INFO: Platform : Architecture: aarch64 Compiler : clang CPU baseline : Requested : 'min' Enabled : NEON NEON_FP16 NEON_VFPV4 ASIMD Flags : none Extra checks: none CPU dispatch : Requested : 'max -xop -fma4' Enabled : ASIMDHP ASIMDDP ASIMDFHM Generated : none INFO: CCompilerOpt.cache_flush[864] : write cache to path -> /private/var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmp5ZPIbv/built-wheels-v0/pypi/scikit-image/0.19.3/hLW_f7wWeGDOPRlSazQXw/scikit-image-0.19.3.tar.gz/build/temp.macosx-12.6-arm64-3.11/ccompiler_opt_cache_ext.py ``` We need the `scikit_image-0.19.3-cp311-cp311-macosx_14_0_arm64.whl` line, but we were failing to find it due to all the extra output at the end (presumedly, some kind of `atexit` logging). This PR modifies the hooks to instead write their results to files that are passed in by the parent. On the other end, we then read the results back from disk. This makes it much more robust to "other" output in the script. Closes https://github.com/astral-sh/uv/issues/1813. ## Test Plan Ran `cargo run pip install scikit-image==0.19.3 --reinstall --no-cache-dir` on Python 3.11. --- crates/uv-build/src/lib.rs | 108 ++++++++++++++++++++++--------------- 1 file changed, 64 insertions(+), 44 deletions(-) diff --git a/crates/uv-build/src/lib.rs b/crates/uv-build/src/lib.rs index a1015cc44..97f15da70 100644 --- a/crates/uv-build/src/lib.rs +++ b/crates/uv-build/src/lib.rs @@ -5,7 +5,6 @@ use std::ffi::OsString; use std::fmt::{Display, Formatter}; use std::io; -use std::io::BufRead; use std::path::{Path, PathBuf}; use std::process::{ExitStatus, Output}; use std::str::FromStr; @@ -460,6 +459,7 @@ impl SourceBuild { &config_settings, &environment_variables, &modified_path, + &temp_dir, ) .await?; } @@ -592,6 +592,12 @@ impl SourceBuild { let metadata_directory = self.temp_dir.path().join("metadata_directory"); fs::create_dir(&metadata_directory)?; + // Write the hook output to a file so that we can read it back reliably. + let outfile = self + .temp_dir + .path() + .join("prepare_metadata_for_build_wheel.txt"); + debug!( "Calling `{}.prepare_metadata_for_build_wheel()`", pep517_backend.backend @@ -603,13 +609,17 @@ impl SourceBuild { prepare_metadata_for_build_wheel = getattr(backend, "prepare_metadata_for_build_wheel", None) if prepare_metadata_for_build_wheel: - print(prepare_metadata_for_build_wheel("{}", config_settings={})) + dirname = prepare_metadata_for_build_wheel("{}", config_settings={}) else: - print() + dirname = None + + with open("{}", "w") as fp: + fp.write(dirname or "") "#, pep517_backend.backend_import(), escape_path_for_python(&metadata_directory), self.config_settings.escape_for_python(), + escape_path_for_python(&outfile), }; let span = info_span!( "run_python_script", @@ -632,26 +642,12 @@ impl SourceBuild { &self.package_id, )); } - let message = output - .stdout - .lines() - .last() - .transpose() - .map_err(|err| err.to_string()) - .and_then(|last_line| last_line.ok_or("Missing message".to_string())) - .map_err(|err| { - Error::from_command_output( - format!( - "Build backend failed to return metadata directory with `prepare_metadata_for_build_wheel`: {err}" - ), - &output, - &self.package_id, - ) - })?; - if message.is_empty() { + + let dirname = fs::read_to_string(&outfile)?; + if dirname.is_empty() { return Ok(None); } - self.metadata_directory = Some(metadata_directory.join(message)); + self.metadata_directory = Some(metadata_directory.join(dirname)); Ok(self.metadata_directory.clone()) } @@ -732,22 +728,31 @@ impl SourceBuild { .map_or("None".to_string(), |path| { format!(r#""{}""#, escape_path_for_python(path)) }); + + // Write the hook output to a file so that we can read it back reliably. + let outfile = self + .temp_dir + .path() + .join(format!("build_{}.txt", self.build_kind)); + debug!( "Calling `{}.build_{}(metadata_directory={})`", pep517_backend.backend, self.build_kind, metadata_directory ); - let escaped_wheel_dir = escape_path_for_python(wheel_dir); let script = formatdoc! { r#" {} - print(backend.build_{}("{}", metadata_directory={}, config_settings={})) + wheel_filename = backend.build_{}("{}", metadata_directory={}, config_settings={}) + with open("{}", "w") as fp: + fp.write(wheel_filename) "#, pep517_backend.backend_import(), self.build_kind, - escaped_wheel_dir, + escape_path_for_python(wheel_dir), metadata_directory, - self.config_settings.escape_for_python() + self.config_settings.escape_for_python(), + escape_path_for_python(&outfile) }; let span = info_span!( "run_python_script", @@ -773,21 +778,19 @@ impl SourceBuild { &self.package_id, )); } - let stdout = String::from_utf8_lossy(&output.stdout); - let distribution_filename = stdout.lines().last(); - let Some(distribution_filename) = - distribution_filename.filter(|wheel| wheel_dir.join(wheel).is_file()) - else { + + let distribution_filename = fs::read_to_string(&outfile)?; + if !wheel_dir.join(&distribution_filename).is_file() { return Err(Error::from_command_output( format!( - "Build backend failed to build wheel through `build_{}()`", + "Build backend failed to produce wheel through `build_{}()`: `{distribution_filename}` not found", self.build_kind ), &output, &self.package_id, )); - }; - Ok(distribution_filename.to_string()) + } + Ok(distribution_filename) } } @@ -819,11 +822,18 @@ async fn create_pep517_build_environment( config_settings: &ConfigSettings, environment_variables: &FxHashMap, modified_path: &OsString, + temp_dir: &TempDir, ) -> Result<(), Error> { + // Write the hook output to a file so that we can read it back reliably. + let outfile = temp_dir + .path() + .join(format!("get_requires_for_build_{build_kind}.txt")); + debug!( "Calling `{}.get_requires_for_build_{}()`", pep517_backend.backend, build_kind ); + let script = formatdoc! { r#" {} @@ -834,8 +844,14 @@ async fn create_pep517_build_environment( requires = get_requires_for_build(config_settings={}) else: requires = [] - print(json.dumps(requires)) - "#, pep517_backend.backend_import(), build_kind, config_settings.escape_for_python() + + with open("{}", "w") as fp: + json.dump(requires, fp) + "#, + pep517_backend.backend_import(), + build_kind, + config_settings.escape_for_python(), + escape_path_for_python(&outfile) }; let span = info_span!( "run_python_script", @@ -858,16 +874,20 @@ async fn create_pep517_build_environment( package_id, )); } - let extra_requires = output - .stdout - .lines() - .last() - .transpose() - .map_err(|err| err.to_string()) - .and_then(|last_line| last_line.ok_or("Missing message".to_string())) - .and_then(|message| serde_json::from_str(&message).map_err(|err| err.to_string())); - let extra_requires: Vec = extra_requires.map_err(|err| { + // Read the requirements from the output file. + let contents = fs_err::read(&outfile).map_err(|err| { + Error::from_command_output( + format!( + "Build backend failed to read extra requires from `get_requires_for_build_{build_kind}`: {err}" + ), + &output, + package_id, + ) + })?; + + // Deserialize the requirements from the output file. + let extra_requires: Vec = serde_json::from_slice(&contents).map_err(|err| { Error::from_command_output( format!( "Build backend failed to return extra requires with `get_requires_for_build_{build_kind}`: {err}"