Skip to content

Commit 1218285

Browse files
rbtcollinsrbtcollins
authored
Merge pull request #2898 from mkeeter/parse-manifest-only-once
Optimization: parse manifest only once
2 parents 3b99e48 + 9e945ed commit 1218285

File tree

3 files changed

+105
-105
lines changed

3 files changed

+105
-105
lines changed

‎src/config.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -739,21 +739,21 @@ impl Cfg {
739739
let components_requested = !components.is_empty() || !targets.is_empty();
740740
// If we're here, the toolchain exists on disk and is a dist toolchain
741741
// so we should attempt to load its manifest
742-
let manifest = if let Some(manifest) = distributable.get_manifest()? {
743-
manifest
742+
let desc = if let Some(desc) = distributable.get_toolchain_desc_with_manifest()? {
743+
desc
744744
} else {
745745
// We can't read the manifest. If this is a v1 install that's understandable
746746
// and we assume the components are all good, otherwise we need to have a go
747747
// at re-fetching the manifest to try again.
748748
return Ok(distributable.guess_v1_manifest());
749749
};
750-
match (distributable.list_components(), components_requested) {
750+
match (desc.list_components(), components_requested) {
751751
// If the toolchain does not support components but there were components requested, bubble up the error
752752
(Err(e), true) => Err(e),
753753
// Otherwise check if all the components we want are installed
754754
(Ok(installed_components), _) => Ok(components.iter().all(|name| {
755755
installed_components.iter().any(|status| {
756-
let cname = status.component.short_name(&manifest);
756+
let cname = status.component.short_name(&desc.manifest);
757757
let cname = cname.as_str();
758758
let cnameim = status.component.short_name_in_manifest();
759759
let cnameim = cnameim.as_str();

‎src/test.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,8 @@ pub fn this_host_triple() -> String {
9797
"x86_64"
9898
} else if cfg!(target_arch = "riscv64") {
9999
"riscv64gc"
100+
} else if cfg!(target_arch = "aarch64") {
101+
"aarch64"
100102
} else {
101103
unimplemented!()
102104
};

‎src/toolchain.rs‎

Lines changed: 99 additions & 101 deletions
Original file line numberDiff line numberDiff line change
@@ -529,30 +529,21 @@ impl<'a> DistributableToolchain<'a> {
529529

530530
// Installed only.
531531
pub(crate) fn add_component(&self, mut component: Component) -> Result<()> {
532-
if !self.0.exists() {
533-
return Err(RustupError::ToolchainNotInstalled(self.0.name.to_owned()).into());
534-
}
535-
536-
let toolchain = &self.0.name;
537-
let toolchain = ToolchainDesc::from_str(toolchain).expect("must be valid");
538-
539-
let prefix = InstallPrefix::from(self.0.path.to_owned());
540-
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;
541-
542-
if let Some(manifest) = manifestation.load_manifest()? {
532+
if let Some(desc) = self.get_toolchain_desc_with_manifest()? {
543533
// Rename the component if necessary.
544-
if let Some(c) = manifest.rename_component(&component) {
534+
if let Some(c) = desc.manifest.rename_component(&component) {
545535
component = c;
546536
}
547537

548538
// Validate the component name
549-
let rust_pkg = manifest
539+
let rust_pkg = desc
540+
.manifest
550541
.packages
551542
.get("rust")
552543
.expect("manifest should contain a rust package");
553544
let targ_pkg = rust_pkg
554545
.targets
555-
.get(&toolchain.target)
546+
.get(&desc.toolchain.target)
556547
.expect("installed manifest should have a known target");
557548

558549
if !targ_pkg.components.contains(&component) {
@@ -562,8 +553,12 @@ impl<'a> DistributableToolchain<'a> {
562553
} else {
563554
return Err(RustupError::UnknownComponent {
564555
name: self.0.name.to_string(),
565-
component: component.description(&manifest),
566-
suggestion: self.get_component_suggestion(&component, &manifest, false),
556+
component: component.description(&desc.manifest),
557+
suggestion: self.get_component_suggestion(
558+
&component,
559+
&desc.manifest,
560+
false,
561+
),
567562
}
568563
.into());
569564
}
@@ -574,13 +569,13 @@ impl<'a> DistributableToolchain<'a> {
574569
remove_components: vec![],
575570
};
576571

577-
manifestation.update(
578-
&manifest,
572+
desc.manifestation.update(
573+
&desc.manifest,
579574
changes,
580575
false,
581576
&self.download_cfg(),
582577
&self.download_cfg().notify_handler,
583-
&toolchain.manifest_name(),
578+
&desc.toolchain.manifest_name(),
584579
false,
585580
)?;
586581

@@ -737,17 +732,7 @@ impl<'a> DistributableToolchain<'a> {
737732

738733
// Installed only.
739734
pub(crate) fn get_manifest(&self) -> Result<Option<Manifest>> {
740-
if !self.0.exists() {
741-
bail!(RustupError::ToolchainNotInstalled(self.0.name().to_owned()));
742-
}
743-
744-
let toolchain = &self.0.name();
745-
let toolchain = ToolchainDesc::from_str(toolchain)?;
746-
747-
let prefix = InstallPrefix::from(self.0.path().to_owned());
748-
let manifestation = Manifestation::open(prefix, toolchain.target)?;
749-
750-
manifestation.load_manifest()
735+
Ok(self.get_toolchain_desc_with_manifest()?.map(|d| d.manifest))
751736
}
752737

753738
// Not installed only?
@@ -802,102 +787,53 @@ impl<'a> DistributableToolchain<'a> {
802787
}
803788
}
804789

805-
// Installed only.
806-
pub fn list_components(&self) -> Result<Vec<ComponentStatus>> {
790+
pub(crate) fn get_toolchain_desc_with_manifest(
791+
&self,
792+
) -> Result<Option<ToolchainDescWithManifest>> {
807793
if !self.0.exists() {
808794
bail!(RustupError::ToolchainNotInstalled(self.0.name.to_owned()));
809795
}
810-
811796
let toolchain = &self.0.name;
812797
let toolchain = ToolchainDesc::from_str(toolchain)
813798
.context(RustupError::ComponentsUnsupported(self.0.name.to_string()))?;
814799

815800
let prefix = InstallPrefix::from(self.0.path.to_owned());
816801
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;
802+
Ok(manifestation
803+
.load_manifest()?
804+
.map(|manifest| ToolchainDescWithManifest {
805+
toolchain,
806+
manifestation,
807+
manifest,
808+
}))
809+
}
817810

818-
if let Some(manifest) = manifestation.load_manifest()? {
819-
let config = manifestation.read_config()?;
820-
821-
// Return all optional components of the "rust" package for the
822-
// toolchain's target triple.
823-
let mut res = Vec::new();
824-
825-
let rust_pkg = manifest
826-
.packages
827-
.get("rust")
828-
.expect("manifest should contain a rust package");
829-
let targ_pkg = rust_pkg
830-
.targets
831-
.get(&toolchain.target)
832-
.expect("installed manifest should have a known target");
833-
834-
for component in &targ_pkg.components {
835-
let installed = config
836-
.as_ref()
837-
.map(|c| component.contained_within(&c.components))
838-
.unwrap_or(false);
839-
840-
let component_target = TargetTriple::new(&component.target());
841-
842-
// Get the component so we can check if it is available
843-
let component_pkg = manifest
844-
.get_package(component.short_name_in_manifest())
845-
.unwrap_or_else(|_| {
846-
panic!(
847-
"manifest should contain component {}",
848-
&component.short_name(&manifest)
849-
)
850-
});
851-
let component_target_pkg = component_pkg
852-
.targets
853-
.get(&component_target)
854-
.expect("component should have target toolchain");
855-
856-
res.push(ComponentStatus {
857-
component: component.clone(),
858-
name: component.name(&manifest),
859-
installed,
860-
available: component_target_pkg.available(),
861-
});
862-
}
863-
864-
res.sort_by(|a, b| a.component.cmp(&b.component));
865-
866-
Ok(res)
811+
pub fn list_components(&self) -> Result<Vec<ComponentStatus>> {
812+
if let Some(toolchain) = self.get_toolchain_desc_with_manifest()? {
813+
toolchain.list_components()
867814
} else {
868815
Err(RustupError::ComponentsUnsupported(self.0.name.to_string()).into())
869816
}
870817
}
871818

872819
// Installed only.
873820
pub(crate) fn remove_component(&self, mut component: Component) -> Result<()> {
874-
// Overlapping code with get_manifest :/.
875-
if !self.0.exists() {
876-
return Err(RustupError::ToolchainNotInstalled(self.0.name.to_owned()).into());
877-
}
878-
879-
let toolchain = &self.0.name;
880-
let toolchain = ToolchainDesc::from_str(toolchain).expect("must be valid");
881-
882-
let prefix = InstallPrefix::from(self.0.path.to_owned());
883-
let manifestation = Manifestation::open(prefix, toolchain.target.clone())?;
884-
885-
if let Some(manifest) = manifestation.load_manifest()? {
821+
if let Some(desc) = self.get_toolchain_desc_with_manifest()? {
886822
// Rename the component if necessary.
887-
if let Some(c) = manifest.rename_component(&component) {
823+
if let Some(c) = desc.manifest.rename_component(&component) {
888824
component = c;
889825
}
890826

891-
let dist_config = manifestation.read_config()?.unwrap();
827+
let dist_config = desc.manifestation.read_config()?.unwrap();
892828
if !dist_config.components.contains(&component) {
893829
let wildcard_component = component.wildcard();
894830
if dist_config.components.contains(&wildcard_component) {
895831
component = wildcard_component;
896832
} else {
897833
return Err(RustupError::UnknownComponent {
898834
name: self.0.name.to_string(),
899-
component: component.description(&manifest),
900-
suggestion: self.get_component_suggestion(&component, &manifest, true),
835+
component: component.description(&desc.manifest),
836+
suggestion: self.get_component_suggestion(&component, &desc.manifest, true),
901837
}
902838
.into());
903839
}
@@ -908,13 +844,13 @@ impl<'a> DistributableToolchain<'a> {
908844
remove_components: vec![component],
909845
};
910846

911-
manifestation.update(
912-
&manifest,
847+
desc.manifestation.update(
848+
&desc.manifest,
913849
changes,
914850
false,
915851
&self.download_cfg(),
916852
&self.download_cfg().notify_handler,
917-
&toolchain.manifest_name(),
853+
&desc.toolchain.manifest_name(),
918854
false,
919855
)?;
920856

@@ -972,6 +908,68 @@ impl<'a> DistributableToolchain<'a> {
972908
}
973909
}
974910

911+
/// Helper type to avoid parsing a manifest more than once
912+
pub(crate) struct ToolchainDescWithManifest {
913+
toolchain: ToolchainDesc,
914+
manifestation: Manifestation,
915+
pub manifest: Manifest,
916+
}
917+
918+
impl ToolchainDescWithManifest {
919+
pub(crate) fn list_components(&self) -> Result<Vec<ComponentStatus>> {
920+
let config = self.manifestation.read_config()?;
921+
922+
// Return all optional components of the "rust" package for the
923+
// toolchain's target triple.
924+
let mut res = Vec::new();
925+
926+
let rust_pkg = self
927+
.manifest
928+
.packages
929+
.get("rust")
930+
.expect("manifest should contain a rust package");
931+
let targ_pkg = rust_pkg
932+
.targets
933+
.get(&self.toolchain.target)
934+
.expect("installed manifest should have a known target");
935+
936+
for component in &targ_pkg.components {
937+
let installed = config
938+
.as_ref()
939+
.map(|c| component.contained_within(&c.components))
940+
.unwrap_or(false);
941+
942+
let component_target = TargetTriple::new(&component.target());
943+
944+
// Get the component so we can check if it is available
945+
let component_pkg = self
946+
.manifest
947+
.get_package(component.short_name_in_manifest())
948+
.unwrap_or_else(|_| {
949+
panic!(
950+
"manifest should contain component {}",
951+
&component.short_name(&self.manifest)
952+
)
953+
});
954+
let component_target_pkg = component_pkg
955+
.targets
956+
.get(&component_target)
957+
.expect("component should have target toolchain");
958+
959+
res.push(ComponentStatus {
960+
component: component.clone(),
961+
name: component.name(&self.manifest),
962+
installed,
963+
available: component_target_pkg.available(),
964+
});
965+
}
966+
967+
res.sort_by(|a, b| a.component.cmp(&b.component));
968+
969+
Ok(res)
970+
}
971+
}
972+
975973
impl<'a> InstalledToolchain<'a> for DistributableToolchain<'a> {
976974
fn installed_paths(&self) -> Result<Vec<InstalledPath<'a>>> {
977975
let path = &self.0.path;

0 commit comments

Comments
 (0)