Remove lazy loading of registry This removes the need for interior mutability and makes the Project Send + Sync. Change-Id: I4fe3004f33e0d3502e4ea99a974603605183acdf Reviewed-on: https://pigweed-review.googlesource.com/c/pigweed/qg/+/126423 Reviewed-by: Alexei Frolov <frolv@google.com> Commit-Queue: Erik Gilling <konkers@google.com>
diff --git a/qg-cli/src/graph.rs b/qg-cli/src/graph.rs index d311063..13cd3cf 100644 --- a/qg-cli/src/graph.rs +++ b/qg-cli/src/graph.rs
@@ -23,7 +23,7 @@ pub async fn run(_args: &Command) -> Result<()> { let project = Project::locate()?; - let graph = project.registry()?.dot_dependency_graph(); + let graph = project.registry().dot_dependency_graph(); io::stdout().write_all(graph.as_bytes()).await?; Ok(())
diff --git a/qg-cli/src/target.rs b/qg-cli/src/target.rs index 07f5ccf..3b3b6a1 100644 --- a/qg-cli/src/target.rs +++ b/qg-cli/src/target.rs
@@ -24,7 +24,7 @@ #[allow(clippy::unused_async)] pub async fn run(_args: &Command) -> Result<()> { let project = Project::locate()?; - let registry = project.registry()?; + let registry = project.registry(); println!( "Project {}, {} total targets",
diff --git a/qg/src/project/mod.rs b/qg/src/project/mod.rs index c4ff0d4..35efbbf 100644 --- a/qg/src/project/mod.rs +++ b/qg/src/project/mod.rs
@@ -13,12 +13,12 @@ // the License. use std::{ - cell::{Ref, RefCell}, ffi::OsStr, path::{Path, PathBuf}, + sync::Arc, }; -use crate::{registry::Registry, target::Provider}; +use crate::registry::Registry; use crate::{Error, Result}; use manifest::Manifest; @@ -34,9 +34,7 @@ root: PathBuf, cache_dir: PathBuf, name: String, - // Use a RefCell to provide interior mutability on the registry so that it can - // be lazily loaded without taking a mutable reference on the Project. - registry: RefCell<Option<Registry>>, + registry: Arc<Registry>, } impl Project { @@ -93,18 +91,21 @@ /// - [`Error::ProjectNotFound`]: No project manifest found in `project_root`. pub fn load<P: AsRef<Path>>(project_root: P) -> Result<Self> { let project_root = project_root.as_ref(); - if !project_root.join(Self::MANIFEST_FILE).exists() { + let manifest_path = project_root.join(Self::MANIFEST_FILE); + if !manifest_path.exists() { return Err(Error::ProjectNotFound); } - let manifest = self::File::new(project_root.join(Self::MANIFEST_FILE)) - .deserialize_toml::<Manifest>()?; + let manifest_file = self::File::new(manifest_path); + let manifest = manifest_file.deserialize_toml::<Manifest>()?; + + let registry = Registry::parse_manifests(&manifest_file)?; Ok(Self { root: project_root.to_owned(), cache_dir: project_root.join(Self::CACHE_DIRECTORY), name: manifest.project.name, - registry: RefCell::new(None), + registry: Arc::new(registry), }) } @@ -131,17 +132,10 @@ return Err(Error::InvalidName(name.into())); } - let project = Self { - root: root.to_owned(), - cache_dir: root.join(Self::CACHE_DIRECTORY), - name: name.into(), - registry: RefCell::new(None), - }; - let manifest = Manifest::new(name); - project.root_manifest().serialize_toml(&manifest)?; + Self::relative_file(root, Path::new(Self::MANIFEST_FILE))?.serialize_toml(&manifest)?; - Ok(project) + Self::load(root) } /// Returns the root directory of the project. @@ -174,90 +168,10 @@ Self::relative_file(&self.cache_dir, Path::new(path.as_ref())) } - /// Returns the target registry for this project. Will locate and parse - /// manifests if they have not already been read. - /// - /// # Errors - /// Returns an error if manifest files could not be read or are improperly - /// structured. - pub fn registry(&self) -> Result<Ref<Registry>> { - { - // Scope the mutable borrow to just the existence check and - // manifest parsing. - let mut registry = self.registry.borrow_mut(); - if registry.is_none() { - *registry = Some(self.parse_manifests()?); - } - } - - Ok(Ref::map(self.registry.borrow(), |registry| { - registry - .as_ref() - .expect("registry should be valid due to being created above") - })) - } - - /// Reads `qg` manifest files starting from the project root into a registry - /// of available packages. - /// - /// # Errors - /// Returns an error if manifest files could not be read or are improperly - /// structured. - fn parse_manifests(&self) -> Result<Registry> { - let root_manifest_file = self.root_manifest(); - let root_manifest = root_manifest_file.deserialize_toml::<Manifest>()?; - let mut registry = Registry::new(); - - let project_provider = Provider::new( - &root_manifest.project.name, - root_manifest_file.path(), - false, - ); - registry.add_provider(project_provider)?; - - for (name, target) in root_manifest.targets { - registry.add_target(crate::Target::from_manifest( - &name, - &root_manifest.project.name, - target.namespace.is_global(), - target, - )?)?; - } - - for (provider, desc) in &root_manifest.providers { - if let Some(manifest) = &desc.manifest { - let provider_file = root_manifest_file.relative_file(manifest); - - let provider_data = provider_file.deserialize_toml::<manifest::ProviderFile>()?; - let global_provider = if let Some(desc) = &provider_data.provider { - desc.namespace.is_global() - } else { - false - }; - - registry.add_provider(Provider::new( - provider, - provider_file.path(), - global_provider, - ))?; - - for (name, target) in provider_data.targets { - registry.add_target(crate::Target::from_manifest( - &name, - provider, - global_provider || target.namespace.is_global(), - target, - )?)?; - } - } - } - - Ok(registry) - } - - fn root_manifest(&self) -> self::File { - self.file(Self::MANIFEST_FILE) - .expect("manifest path is not absolute") + /// Returns the target registry for this project. + #[must_use] + pub fn registry(&self) -> Arc<Registry> { + self.registry.clone() } fn relative_file(root: &Path, path: &Path) -> Result<self::File> { @@ -299,7 +213,7 @@ cache_dir: root.join("qg-cache"), root, name: "qg2".into(), - registry: RefCell::new(None), + registry: Arc::new(Registry::new()), }; assert_eq!( @@ -323,7 +237,7 @@ cache_dir: root.join("qg-cache"), root, name: "qg2".into(), - registry: RefCell::new(None), + registry: Arc::new(Registry::new()), }; assert!(matches!( @@ -364,7 +278,7 @@ let project = Project::load("./src/test_projects/dependency_test").unwrap(); assert_eq!(project.name, "dep-test"); - let registry = project.registry().unwrap(); + let registry = project.registry(); let a_id = registry.get_node_id("dep-test:a").unwrap(); let b_id = registry.get_node_id("dep-test:b").unwrap(); let c_id = registry.get_node_id("dep-test:c").unwrap();
diff --git a/qg/src/registry.rs b/qg/src/registry.rs index 6876f0f..b52f10e 100644 --- a/qg/src/registry.rs +++ b/qg/src/registry.rs
@@ -24,8 +24,14 @@ Directed, Direction, Graph, }; -use crate::target::{Provider, Target}; -use crate::{Error, Result}; +use crate::{ + project::{ + manifest::{self, Manifest}, + File, + }, + target::{Provider, Target}, + Error, Result, +}; /// A node in the dependency graph. #[derive(Debug)] @@ -97,6 +103,63 @@ } } + /// Reads `qg` manifest files starting from the project root into a registry + /// of available packages. + /// + /// # Errors + /// Returns an error if manifest files could not be read or are improperly + /// structured. + pub fn parse_manifests(root_manifest_file: &File) -> Result<Self> { + let root_manifest = root_manifest_file.deserialize_toml::<Manifest>()?; + let mut registry = Registry::new(); + + let project_provider = Provider::new( + &root_manifest.project.name, + root_manifest_file.path(), + false, + ); + registry.add_provider(project_provider)?; + + for (name, target) in root_manifest.targets { + registry.add_target(crate::Target::from_manifest( + &name, + &root_manifest.project.name, + target.namespace.is_global(), + target, + )?)?; + } + + for (provider, desc) in &root_manifest.providers { + if let Some(manifest) = &desc.manifest { + let provider_file = root_manifest_file.relative_file(manifest); + + let provider_data = provider_file.deserialize_toml::<manifest::ProviderFile>()?; + let global_provider = if let Some(desc) = &provider_data.provider { + desc.namespace.is_global() + } else { + false + }; + + registry.add_provider(Provider::new( + provider, + provider_file.path(), + global_provider, + ))?; + + for (name, target) in provider_data.targets { + registry.add_target(crate::Target::from_manifest( + &name, + provider, + global_provider || target.namespace.is_global(), + target, + )?)?; + } + } + } + + Ok(registry) + } + /// Returns the number of registered targets. #[must_use] pub fn target_count(&self) -> usize {