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.

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

Optimization: parse manifest only once #2898

Merged
merged 1 commit into from
Nov 12, 2021
Merged

Optimization: parse manifest only once #2898

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 4 additions & 4 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,21 +739,21 @@ impl Cfg {
let components_requested = !components.is_empty() || !targets.is_empty();
// If we're here, the toolchain exists on disk and is a dist toolchain
// so we should attempt to load its manifest
let manifest = if let Some(manifest) = distributable.get_manifest()? {
manifest
let desc = if let Some(desc) = distributable.get_toolchain_desc_with_manifest()? {
desc
} else {
// We can't read the manifest. If this is a v1 install that's understandable
// and we assume the components are all good, otherwise we need to have a go
// at re-fetching the manifest to try again.
return Ok(distributable.guess_v1_manifest());
};
match (distributable.list_components(), components_requested) {
match (desc.list_components(), components_requested) {
// If the toolchain does not support components but there were components requested, bubble up the error
(Err(e), true) => Err(e),
// Otherwise check if all the components we want are installed
(Ok(installed_components), _) => Ok(components.iter().all(|name| {
installed_components.iter().any(|status| {
let cname = status.component.short_name(&manifest);
let cname = status.component.short_name(&desc.manifest);
let cname = cname.as_str();
let cnameim = status.component.short_name_in_manifest();
let cnameim = cnameim.as_str();
Expand Down
2 changes: 2 additions & 0 deletions src/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,8 @@ pub fn this_host_triple() -> String {
"x86_64"
} else if cfg!(target_arch = "riscv64") {
"riscv64gc"
} else if cfg!(target_arch = "aarch64") {
"aarch64"
} else {
unimplemented!()
};
Expand Down
200 changes: 99 additions & 101 deletions src/toolchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -529,30 +529,21 @@ impl<'a> DistributableToolchain<'a> {

// Installed only.
pub(crate) fn add_component(&self, mut component: Component) -> Result<()> {
if !self.0.exists() {
return Err(RustupError::ToolchainNotInstalled(self.0.name.to_owned()).into());
}

let toolchain = &self.0.name;
let toolchain = ToolchainDesc::from_str(toolchain).expect("must be valid");

let prefix = InstallPrefix::from(self.0.path.to_owned());
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;

if let Some(manifest) = manifestation.load_manifest()? {
if let Some(desc) = self.get_toolchain_desc_with_manifest()? {
// Rename the component if necessary.
if let Some(c) = manifest.rename_component(&component) {
if let Some(c) = desc.manifest.rename_component(&component) {
component = c;
}

// Validate the component name
let rust_pkg = manifest
let rust_pkg = desc
.manifest
.packages
.get("rust")
.expect("manifest should contain a rust package");
let targ_pkg = rust_pkg
.targets
.get(&toolchain.target)
.get(&desc.toolchain.target)
.expect("installed manifest should have a known target");

if !targ_pkg.components.contains(&component) {
Expand All @@ -562,8 +553,12 @@ impl<'a> DistributableToolchain<'a> {
} else {
return Err(RustupError::UnknownComponent {
name: self.0.name.to_string(),
component: component.description(&manifest),
suggestion: self.get_component_suggestion(&component, &manifest, false),
component: component.description(&desc.manifest),
suggestion: self.get_component_suggestion(
&component,
&desc.manifest,
false,
),
}
.into());
}
Expand All @@ -574,13 +569,13 @@ impl<'a> DistributableToolchain<'a> {
remove_components: vec![],
};

manifestation.update(
&manifest,
desc.manifestation.update(
&desc.manifest,
changes,
false,
&self.download_cfg(),
&self.download_cfg().notify_handler,
&toolchain.manifest_name(),
&desc.toolchain.manifest_name(),
false,
)?;

Expand Down Expand Up @@ -737,17 +732,7 @@ impl<'a> DistributableToolchain<'a> {

// Installed only.
pub(crate) fn get_manifest(&self) -> Result<Option<Manifest>> {
if !self.0.exists() {
bail!(RustupError::ToolchainNotInstalled(self.0.name().to_owned()));
}

let toolchain = &self.0.name();
let toolchain = ToolchainDesc::from_str(toolchain)?;

let prefix = InstallPrefix::from(self.0.path().to_owned());
let manifestation = Manifestation::open(prefix, toolchain.target)?;

manifestation.load_manifest()
Ok(self.get_toolchain_desc_with_manifest()?.map(|d| d.manifest))
}

// Not installed only?
Expand Down Expand Up @@ -802,102 +787,53 @@ impl<'a> DistributableToolchain<'a> {
}
}

// Installed only.
pub fn list_components(&self) -> Result<Vec<ComponentStatus>> {
pub(crate) fn get_toolchain_desc_with_manifest(
&self,
) -> Result<Option<ToolchainDescWithManifest>> {
if !self.0.exists() {
bail!(RustupError::ToolchainNotInstalled(self.0.name.to_owned()));
}

let toolchain = &self.0.name;
let toolchain = ToolchainDesc::from_str(toolchain)
.context(RustupError::ComponentsUnsupported(self.0.name.to_string()))?;

let prefix = InstallPrefix::from(self.0.path.to_owned());
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;
Ok(manifestation
.load_manifest()?
.map(|manifest| ToolchainDescWithManifest {
toolchain,
manifestation,
manifest,
}))
}

if let Some(manifest) = manifestation.load_manifest()? {
let config = manifestation.read_config()?;

// Return all optional components of the "rust" package for the
// toolchain's target triple.
let mut res = Vec::new();

let rust_pkg = manifest
.packages
.get("rust")
.expect("manifest should contain a rust package");
let targ_pkg = rust_pkg
.targets
.get(&toolchain.target)
.expect("installed manifest should have a known target");

for component in &targ_pkg.components {
let installed = config
.as_ref()
.map(|c| component.contained_within(&c.components))
.unwrap_or(false);

let component_target = TargetTriple::new(&component.target());

// Get the component so we can check if it is available
let component_pkg = manifest
.get_package(component.short_name_in_manifest())
.unwrap_or_else(|_| {
panic!(
"manifest should contain component {}",
&component.short_name(&manifest)
)
});
let component_target_pkg = component_pkg
.targets
.get(&component_target)
.expect("component should have target toolchain");

res.push(ComponentStatus {
component: component.clone(),
name: component.name(&manifest),
installed,
available: component_target_pkg.available(),
});
}

res.sort_by(|a, b| a.component.cmp(&b.component));

Ok(res)
pub fn list_components(&self) -> Result<Vec<ComponentStatus>> {
if let Some(toolchain) = self.get_toolchain_desc_with_manifest()? {
toolchain.list_components()
} else {
Err(RustupError::ComponentsUnsupported(self.0.name.to_string()).into())
}
}

// Installed only.
pub(crate) fn remove_component(&self, mut component: Component) -> Result<()> {
// Overlapping code with get_manifest :/.
if !self.0.exists() {
return Err(RustupError::ToolchainNotInstalled(self.0.name.to_owned()).into());
}

let toolchain = &self.0.name;
let toolchain = ToolchainDesc::from_str(toolchain).expect("must be valid");

let prefix = InstallPrefix::from(self.0.path.to_owned());
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;

if let Some(manifest) = manifestation.load_manifest()? {
if let Some(desc) = self.get_toolchain_desc_with_manifest()? {
// Rename the component if necessary.
if let Some(c) = manifest.rename_component(&component) {
if let Some(c) = desc.manifest.rename_component(&component) {
component = c;
}

let dist_config = manifestation.read_config()?.unwrap();
let dist_config = desc.manifestation.read_config()?.unwrap();
if !dist_config.components.contains(&component) {
let wildcard_component = component.wildcard();
if dist_config.components.contains(&wildcard_component) {
component = wildcard_component;
} else {
return Err(RustupError::UnknownComponent {
name: self.0.name.to_string(),
component: component.description(&manifest),
suggestion: self.get_component_suggestion(&component, &manifest, true),
component: component.description(&desc.manifest),
suggestion: self.get_component_suggestion(&component, &desc.manifest, true),
}
.into());
}
Expand All @@ -908,13 +844,13 @@ impl<'a> DistributableToolchain<'a> {
remove_components: vec![component],
};

manifestation.update(
&manifest,
desc.manifestation.update(
&desc.manifest,
changes,
false,
&self.download_cfg(),
&self.download_cfg().notify_handler,
&toolchain.manifest_name(),
&desc.toolchain.manifest_name(),
false,
)?;

Expand Down Expand Up @@ -972,6 +908,68 @@ impl<'a> DistributableToolchain<'a> {
}
}

/// Helper type to avoid parsing a manifest more than once
pub(crate) struct ToolchainDescWithManifest {
toolchain: ToolchainDesc,
manifestation: Manifestation,
pub manifest: Manifest,
}

impl ToolchainDescWithManifest {
pub(crate) fn list_components(&self) -> Result<Vec<ComponentStatus>> {
let config = self.manifestation.read_config()?;

// Return all optional components of the "rust" package for the
// toolchain's target triple.
let mut res = Vec::new();

let rust_pkg = self
.manifest
.packages
.get("rust")
.expect("manifest should contain a rust package");
let targ_pkg = rust_pkg
.targets
.get(&self.toolchain.target)
.expect("installed manifest should have a known target");

for component in &targ_pkg.components {
let installed = config
.as_ref()
.map(|c| component.contained_within(&c.components))
.unwrap_or(false);

let component_target = TargetTriple::new(&component.target());

// Get the component so we can check if it is available
let component_pkg = self
.manifest
.get_package(component.short_name_in_manifest())
.unwrap_or_else(|_| {
panic!(
"manifest should contain component {}",
&component.short_name(&self.manifest)
)
});
let component_target_pkg = component_pkg
.targets
.get(&component_target)
.expect("component should have target toolchain");

res.push(ComponentStatus {
component: component.clone(),
name: component.name(&self.manifest),
installed,
available: component_target_pkg.available(),
});
}

res.sort_by(|a, b| a.component.cmp(&b.component));

Ok(res)
}
}

impl<'a> InstalledToolchain<'a> for DistributableToolchain<'a> {
fn installed_paths(&self) -> Result<Vec<InstalledPath<'a>>> {
let path = &self.0.path;
Expand Down