Skip to content

Don't cache the detected rustc-version, it's cheap enough to requery #2306

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

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Don't cache the detected rustc-version, it's cheap enough to requery #2306

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7c007fc
Add test that a new RustwideBuilder sees the existing toolchain version
Nemo157 Nov 8, 2023
f8d8d8b
Don't cache the detected rustc-version, it's cheap enough to requery
Nemo157 Nov 8, 2023
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
66 changes: 47 additions & 19 deletions src/docbuilder/rustwide_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@ pub struct RustwideBuilder {
storage: Arc<Storage>,
metrics: Arc<InstanceMetrics>,
registry_api: Arc<RegistryApi>,
rustc_version: String,
repository_stats_updater: Arc<RepositoryStatsUpdater>,
workspace_initialize_time: Instant,
}
Expand All @@ -106,7 +105,6 @@ impl RustwideBuilder {
storage: context.storage()?,
metrics: context.instance_metrics()?,
registry_api: context.registry_api()?,
rustc_version: String::new(),
repository_stats_updater: context.repository_stats_updater()?,
workspace_initialize_time: Instant::now(),
})
Expand Down Expand Up @@ -151,11 +149,10 @@ impl RustwideBuilder {
// +channel argument, but the +channel argument doesn't work for CI builds. So
// we fake the rustc version and install from scratch every time since we can't detect
// the already-installed rustc version.
if let Some(ci) = self.toolchain.as_ci() {
if self.toolchain.as_ci().is_some() {
self.toolchain
.install(&self.workspace)
.map_err(FailureError::compat)?;
self.rustc_version = format!("rustc 1.9999.0-nightly ({} 2999-12-29)", ci.sha());
self.add_essential_files()?;
return Ok(true);
}
Expand Down Expand Up @@ -219,12 +216,24 @@ impl RustwideBuilder {
}
}

self.rustc_version = self.detect_rustc_version()?;

let has_changed = old_version.as_deref() != Some(&self.rustc_version);
let has_changed = old_version != Some(self.rustc_version()?);
Ok(has_changed)
}

fn rustc_version(&self) -> Result<String> {
let version = self
.toolchain
.as_ci()
.map(|ci| {
// Detecting the rustc version relies on calling rustc through rustup with the
// +channel argument, but the +channel argument doesn't work for CI builds. So
// we fake the rustc version.
Ok(format!("rustc 1.9999.0-nightly ({} 2999-12-29)", ci.sha()))
})
.unwrap_or_else(|| self.detect_rustc_version())?;
Ok(version)
}

/// Return a string containing the output of `rustc --version`. Only valid
/// for dist toolchains. Will error if run with a CI toolchain.
fn detect_rustc_version(&self) -> Result<String> {
Expand All @@ -243,8 +252,8 @@ impl RustwideBuilder {
}

pub fn add_essential_files(&mut self) -> Result<()> {
self.rustc_version = self.detect_rustc_version()?;
let rustc_version = parse_rustc_version(&self.rustc_version)?;
let rustc_version = self.rustc_version()?;
let parsed_rustc_version = parse_rustc_version(&rustc_version)?;

info!("building a dummy crate to get essential files");

Expand All @@ -266,7 +275,7 @@ impl RustwideBuilder {

let mut build_dir = self
.workspace
.build_dir(&format!("essential-files-{rustc_version}"));
.build_dir(&format!("essential-files-{parsed_rustc_version}"));

// This is an empty library crate that is supposed to always build.
let krate = Crate::crates_io(DUMMY_CRATE_NAME, DUMMY_CRATE_VERSION);
Expand All @@ -281,10 +290,10 @@ impl RustwideBuilder {
let res =
self.execute_build(HOST_TARGET, true, build, &limits, &metadata, true)?;
if !res.result.successful {
bail!("failed to build dummy crate for {}", self.rustc_version);
bail!("failed to build dummy crate for {}", rustc_version);
}

info!("copying essential files for {}", self.rustc_version);
info!("copying essential files for {}", rustc_version);
assert!(!metadata.proc_macro);
let source = build.host_target_dir().join(HOST_TARGET).join("doc");
let dest = tempfile::Builder::new()
Expand All @@ -310,11 +319,7 @@ impl RustwideBuilder {
)?;
}

set_config(
&mut conn,
ConfigName::RustcVersion,
self.rustc_version.clone(),
)?;
set_config(&mut conn, ConfigName::RustcVersion, rustc_version)?;
Ok(())
})()
.map_err(|e| failure::Error::from_boxed_compat(e.into()))
Expand Down Expand Up @@ -691,7 +696,7 @@ impl RustwideBuilder {
.to_string()];
rustdoc_flags.extend(vec![
"--resource-suffix".to_string(),
format!("-{}", parse_rustc_version(&self.rustc_version)?),
format!("-{}", parse_rustc_version(self.rustc_version()?)?),
]);

let mut storage = LogStorage::new(log::LevelFilter::Info);
Expand Down Expand Up @@ -734,7 +739,7 @@ impl RustwideBuilder {

Ok(FullBuildResult {
result: BuildResult {
rustc_version: self.rustc_version.clone(),
rustc_version: self.rustc_version()?,
docsrs_version: format!("docsrs {}", crate::BUILD_VERSION),
successful,
},
Expand Down Expand Up @@ -1329,4 +1334,27 @@ mod tests {
Ok(())
})
}

#[test]
#[ignore]
fn test_new_builder_detects_existing_rustc() {
wrapper(|env: &TestEnvironment| {
let mut builder = RustwideBuilder::init(env)?;
builder.update_toolchain()?;
let old_version = builder.rustc_version()?;
drop(builder);

// new builder should detect the existing rustc version from the previous builder
// (simulating running `update-toolchain` and `build crate` in separate invocations)
let mut builder = RustwideBuilder::init(env)?;
assert!(builder.build_package(
DUMMY_CRATE_NAME,
DUMMY_CRATE_VERSION,
PackageKind::CratesIo
)?);
assert_eq!(old_version, builder.rustc_version()?);

Ok(())
})
}
}