Skip to content

Commit

Permalink
Merge pull request #2898 from mkeeter/parse-manifest-only-once
Browse files Browse the repository at this point in the history 
Optimization: parse manifest only once
  • Loading branch information
@rbtcollins
rbtcollins authored Nov 12, 2021
2 parents 3b99e48 + 9e945ed commit 1218285
Show file tree
Hide file tree
Showing 3 changed files with 105 additions and 105 deletions.
8 changes: 4 additions & 4 deletions src/config.rs
View file
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
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
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

0 comments on commit 1218285

Please sign in to comment.