From 2bbcb36ad991ae34e90b32978d5581e60d43a111 Mon Sep 17 00:00:00 2001 From: Wim Looman Date: Mon, 7 Aug 2023 16:29:26 +0200 Subject: [PATCH] wip: Change timeout to apply to the overall build process, not per-target --- src/docbuilder/rustwide_builder.rs | 175 ++++++++++++++++++++++------- 1 file changed, 135 insertions(+), 40 deletions(-) diff --git a/src/docbuilder/rustwide_builder.rs b/src/docbuilder/rustwide_builder.rs index c95caf893..a3264e700 100644 --- a/src/docbuilder/rustwide_builder.rs +++ b/src/docbuilder/rustwide_builder.rs @@ -23,9 +23,12 @@ use rustwide::cmd::{Command, CommandError, SandboxBuilder, SandboxImage}; use rustwide::logging::{self, LogStorage}; use rustwide::toolchain::ToolchainError; use rustwide::{AlternativeRegistry, Build, Crate, Toolchain, Workspace, WorkspaceBuilder}; -use std::collections::{HashMap, HashSet}; -use std::path::Path; -use std::sync::Arc; +use std::{ + collections::{HashMap, HashSet}, + path::Path, + sync::Arc, + time::Instant, +}; use tracing::{debug, info, warn}; const USER_AGENT: &str = "docs.rs builder (https://github.com/rust-lang/docs.rs)"; @@ -244,9 +247,19 @@ impl RustwideBuilder { .run(|build| { (|| -> Result<()> { let metadata = Metadata::from_crate_root(build.host_source_dir())?; + let deadline = Instant::now() + .checked_add(limits.timeout()) + .context("deadline is not representable")?; - let res = - self.execute_build(HOST_TARGET, true, build, &limits, &metadata, true)?; + let res = self.execute_build( + HOST_TARGET, + true, + build, + &limits, + &metadata, + true, + deadline, + )?; if !res.result.successful { bail!("failed to build dummy crate for {}", self.rustc_version); } @@ -333,15 +346,15 @@ impl RustwideBuilder { return Ok(false); } - self.update_toolchain()?; - - info!("building package {} {}", name, version); - if is_blacklisted(&mut conn, name)? { info!("skipping build of {}, crate has been blacklisted", name); return Ok(false); } + self.update_toolchain()?; + + info!("building package {} {}", name, version); + let limits = Limits::for_crate(&mut conn, name)?; #[cfg(target_os = "linux")] if !self.config.disable_memory_limit { @@ -388,18 +401,56 @@ impl RustwideBuilder { default_target, other_targets, } = metadata.targets(self.config.include_default_targets); - let mut targets = vec![default_target]; - targets.extend(&other_targets); + + let cargo_args = metadata.cargo_args(&[], &[]); + let has_build_std = cargo_args.iter().any(|arg| arg.starts_with("-Zbuild-std")) + || cargo_args + .windows(2) + .any(|args| args[0] == "-Z" && args[1].starts_with("build-std")); + + let other_targets: Vec<_> = other_targets + .into_iter() + .filter(|target| { + // If the explicit target is not a tier one target, we need to install it. + if !docsrs_metadata::DEFAULT_TARGETS.contains(target) && !has_build_std { + // This is a no-op if the target is already installed. + if let Err(e) = self.toolchain.add_target(&self.workspace, target) { + info!("Skipping target {target} since it failed to install: {e}"); + return false; + } + } + true + }) + // Limit the number of targets so that no one can try to build all 200000 possible targets + .take(limits.targets()) + .collect(); + + let targets: Vec<_> = [&default_target] + .into_iter() + .chain(&other_targets) + .copied() + .collect(); // Fetch this before we enter the sandbox, so networking isn't blocked. build.fetch_build_std_dependencies(&targets)?; (|| -> Result { + let deadline = Instant::now() + .checked_add(limits.timeout()) + .context("deadline is not representable")?; + let mut has_docs = false; let mut successful_targets = Vec::new(); // Perform an initial build - let mut res = - self.execute_build(default_target, true, build, &limits, &metadata, false)?; + let mut res = self.execute_build( + default_target, + true, + build, + &limits, + &metadata, + false, + deadline, + )?; // If the build fails with the lockfile given, try using only the dependencies listed in Cargo.toml. let cargo_lock = build.host_source_dir().join("Cargo.lock"); @@ -421,6 +472,7 @@ impl RustwideBuilder { &limits, &metadata, false, + deadline, )?; } @@ -448,8 +500,7 @@ impl RustwideBuilder { successful_targets.push(res.target.clone()); // Then build the documentation for all the targets - // Limit the number of targets so that no one can try to build all 200000 possible targets - for target in other_targets.into_iter().take(limits.targets()) { + for target in other_targets { debug!("building package {} {} for {}", name, version, target); self.build_target( target, @@ -458,6 +509,7 @@ impl RustwideBuilder { local_storage.path(), &mut successful_targets, &metadata, + deadline, )?; } let (_, new_alg) = add_path_into_remote_archive( @@ -572,6 +624,7 @@ impl RustwideBuilder { Ok(successful) } + #[allow(clippy::too_many_arguments)] fn build_target( &self, target: &str, @@ -580,8 +633,10 @@ impl RustwideBuilder { local_storage: &Path, successful_targets: &mut Vec, metadata: &Metadata, + deadline: Instant, ) -> Result<()> { - let target_res = self.execute_build(target, false, build, limits, metadata, false)?; + let target_res = + self.execute_build(target, false, build, limits, metadata, false, deadline)?; if target_res.result.successful { // Cargo is not giving any error and not generating documentation of some crates // when we use a target compile options. Check documentation exists before @@ -600,7 +655,7 @@ impl RustwideBuilder { target: &str, build: &Build, metadata: &Metadata, - limits: &Limits, + deadline: Instant, ) -> Result> { let rustdoc_flags = vec![ "--output-format".to_string(), @@ -623,7 +678,7 @@ impl RustwideBuilder { items_with_examples: 0, }; - self.prepare_command(build, target, metadata, limits, rustdoc_flags)? + self.prepare_command(build, target, metadata, rustdoc_flags, deadline)? .process_lines(&mut |line, _| { if line.starts_with('{') && line.ends_with('}') { let parsed = match serde_json::from_str::>(line) { @@ -650,6 +705,9 @@ impl RustwideBuilder { ) } + // TODO(Nemo157): Look at pulling out a sub-builder for each crate-build + // that holds a lot of this state + #[allow(clippy::too_many_arguments)] fn execute_build( &self, target: &str, @@ -658,6 +716,7 @@ impl RustwideBuilder { limits: &Limits, metadata: &Metadata, create_essential_files: bool, + deadline: Instant, ) -> Result { let cargo_metadata = CargoMetadata::load_from_rustwide( &self.workspace, @@ -682,7 +741,7 @@ impl RustwideBuilder { // we have to run coverage before the doc-build because currently it // deletes the doc-target folder. // https://github.com/rust-lang/cargo/issues/9447 - let doc_coverage = match self.get_coverage(target, build, metadata, limits) { + let doc_coverage = match self.get_coverage(target, build, metadata, deadline) { Ok(cov) => cov, Err(err) => { info!("error when trying to get coverage: {}", err); @@ -692,10 +751,11 @@ impl RustwideBuilder { }; let successful = logging::capture(&storage, || { - self.prepare_command(build, target, metadata, limits, rustdoc_flags) + self.prepare_command(build, target, metadata, rustdoc_flags, deadline) .and_then(|command| command.run().map_err(Error::from)) - .is_ok() - }); + }) + .map_err(|e| info!("failed build: {e:?}")) + .is_ok(); // For proc-macros, cargo will put the output in `target/doc`. // Move it to the target-specific directory for consistency with other builds. @@ -732,9 +792,13 @@ impl RustwideBuilder { build: &'ws Build, target: &str, metadata: &Metadata, - limits: &Limits, mut rustdoc_flags_extras: Vec, + deadline: Instant, ) -> Result> { + let timeout = deadline + .checked_duration_since(Instant::now()) + .context("exceeded deadline")?; + // Add docs.rs specific arguments let mut cargo_args = vec![ "--offline".into(), @@ -783,22 +847,7 @@ impl RustwideBuilder { rustdoc_flags_extras.extend(UNCONDITIONAL_ARGS.iter().map(|&s| s.to_owned())); let cargo_args = metadata.cargo_args(&cargo_args, &rustdoc_flags_extras); - // If the explicit target is not a tier one target, we need to install it. - let has_build_std = cargo_args.windows(2).any(|args| { - args[0].starts_with("-Zbuild-std") - || (args[0] == "-Z" && args[1].starts_with("build-std")) - }) || cargo_args.last().unwrap().starts_with("-Zbuild-std"); - if !docsrs_metadata::DEFAULT_TARGETS.contains(&target) && !has_build_std { - // This is a no-op if the target is already installed. - self.toolchain - .add_target(&self.workspace, target) - .map_err(FailureError::compat)?; - } - - let mut command = build - .cargo() - .timeout(Some(limits.timeout())) - .no_output_timeout(None); + let mut command = build.cargo().timeout(Some(timeout)).no_output_timeout(None); for (key, val) in metadata.environment_variables() { command = command.env(key, val); @@ -885,7 +934,10 @@ pub(crate) struct BuildResult { #[cfg(test)] mod tests { use super::*; - use crate::test::{assert_redirect, assert_success, wrapper, TestEnvironment}; + use crate::{ + db::Overrides, + test::{assert_redirect, assert_success, wrapper, TestEnvironment}, + }; use serde_json::Value; fn remove_cache_files(env: &TestEnvironment, crate_: &str, version: &str) -> Result<()> { @@ -1218,6 +1270,49 @@ mod tests { }); } + #[test] + #[ignore] + fn test_timeout_skips_some_targets() { + wrapper(|env| { + let crate_ = "bs58"; + let version = "0.5.0"; + let mut builder = RustwideBuilder::init(env).unwrap(); + let get_targets = || -> i32 { + env.db() + .conn() + .query_one( + "SELECT json_array_length(releases.doc_targets) FROM releases;", + &[], + ) + .unwrap() + .get(0) + }; + + // Build once to time it and count how many targets are built + let start = Instant::now(); + assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + let timeout = start.elapsed() / 2; + let original_targets = get_targets(); + + // Build again with half the time and count how many targets are built + Overrides::save( + &mut env.db().conn(), + crate_, + Overrides { + memory: None, + targets: Some(original_targets as usize), + timeout: Some(timeout), + }, + )?; + assert!(builder.build_package(crate_, version, PackageKind::CratesIo)?); + let new_targets = get_targets(); + + assert!(new_targets < original_targets); + + Ok(()) + }); + } + #[test] #[ignore] fn test_implicit_features_for_optional_dependencies() {