From da90ee59b96a136822ce86b378f57188ef7f9ab0 Mon Sep 17 00:00:00 2001 From: m-hilgendorf Date: Tue, 19 Nov 2024 14:36:54 -0600 Subject: [PATCH] fix: detect presence of correct artifacts directory when collecting input, remove dead code --- packages/server/src/artifact/checkin/input.rs | 98 +++++++------------ .../server/src/artifact/checkin/output.rs | 35 +++++-- packages/server/src/artifact/checkin/tests.rs | 46 ++++++++- packages/server/src/artifact/checkout.rs | 7 +- 4 files changed, 109 insertions(+), 77 deletions(-) diff --git a/packages/server/src/artifact/checkin/input.rs b/packages/server/src/artifact/checkin/input.rs index 6166aad1b..645590e00 100644 --- a/packages/server/src/artifact/checkin/input.rs +++ b/packages/server/src/artifact/checkin/input.rs @@ -44,36 +44,29 @@ struct State { impl Server { pub(super) async fn create_input_graph( &self, - arg: tg::artifact::checkin::Arg, + mut arg: tg::artifact::checkin::Arg, progress: Option<&crate::progress::Handle>, ) -> tg::Result { - // Make a best guess for the input. - let path = self.find_root(&arg.path).await.map_err( + // Find the root path. + arg.path = self.find_root(&arg.path).await.map_err( |source| tg::error!(!source, %path = arg.path.display(), "failed to find root path for checkin"), )?; - // Create a graph. - let mut graph = 'a: { - let arg_ = tg::artifact::checkin::Arg { - path, - ..arg.clone() - }; - let graph = self.collect_input(arg_, progress).await?; + // Create the ignore tree. + let ignore = self.ignore_for_checkin().await?; - // Check if the graph contains the path. - if graph.contains_path(&arg.path).await { - break 'a graph; - } + // Initialize state. + let state = RwLock::new(State { + ignore, + roots: Vec::new(), + visited: BTreeMap::new(), + graph: Graph { nodes: Vec::new() }, + }); - // Otherwise use the parent. - let path = arg - .path - .parent() - .ok_or_else(|| tg::error!("cannot check in root"))? - .to_owned(); - let arg_ = tg::artifact::checkin::Arg { path, ..arg }; - self.collect_input(arg_, progress).await? - }; + // Create the graph. + self.create_input_graph_inner(None, arg.path.as_ref(), &arg, &state, progress) + .await?; + let State { mut graph, .. } = state.into_inner(); // Validate the input graph. graph.validate().await?; @@ -109,31 +102,7 @@ impl Server { Ok(path.to_owned()) } - async fn collect_input( - &self, - arg: tg::artifact::checkin::Arg, - progress: Option<&crate::progress::Handle>, - ) -> tg::Result { - // Create the ignore tree. - let ignore = self.ignore_for_checkin().await?; - - // Initialize state. - let state = RwLock::new(State { - ignore, - roots: Vec::new(), - visited: BTreeMap::new(), - graph: Graph { nodes: Vec::new() }, - }); - - // Collect input. - self.collect_input_inner(None, arg.path.as_ref(), &arg, &state, progress) - .await?; - - let State { graph, .. } = state.into_inner(); - Ok(graph) - } - - async fn collect_input_inner( + async fn create_input_graph_inner( &self, referrer: Option, path: &Path, @@ -211,7 +180,7 @@ impl Server { // Collect any dangling directories. for dangling in dangling { - Box::pin(self.collect_input_inner(None, &dangling, arg, state, progress)) + Box::pin(self.create_input_graph_inner(None, &dangling, arg, state, progress)) .await .map_err( |source| tg::error!(!source, %path = dangling.display(), "failed to collect dangling directory"), @@ -221,7 +190,7 @@ impl Server { // If this is a module file, ensure the parent is collected. if metadata.is_file() && tg::package::is_module_path(&absolute_path) { let parent = absolute_path.parent().unwrap().to_owned(); - let parent = Box::pin(self.collect_input_inner(None, &parent, arg, state, progress)) + let parent = Box::pin(self.create_input_graph_inner(None, &parent, arg, state, progress)) .await .map_err( |source| tg::error!(!source, %path = absolute_path.display(), "failed to collect input of parent"), @@ -317,7 +286,7 @@ impl Server { let path = path.join(&name); // Follow the edge. - let node = Box::pin(self.collect_input_inner( + let node = Box::pin(self.create_input_graph_inner( Some(referrer), name.as_ref(), arg, @@ -506,7 +475,7 @@ impl Server { // Get the input of the referent. let child = if is_external { // If this is an external import, treat it as a root using the absolute path. - self.collect_input_inner(Some(referrer), &absolute_path, &arg, state, progress).await.map_err(|source| tg::error!(!source, "failed to collect child input"))? + self.create_input_graph_inner(Some(referrer), &absolute_path, &arg, state, progress).await.map_err(|source| tg::error!(!source, "failed to collect child input"))? } else { // Otherwise, treat it as a relative path. @@ -514,7 +483,7 @@ impl Server { let parent = state.read().await.graph.nodes[referrer].parent.ok_or_else(|| tg::error!("expected a parent"))?; // Recurse. - self.collect_input_inner(Some(parent), import_path.as_ref(), &arg, state, progress).await.map_err(|source| tg::error!(!source, "failed to collect child input"))? + self.create_input_graph_inner(Some(parent), import_path.as_ref(), &arg, state, progress).await.map_err(|source| tg::error!(!source, "failed to collect child input"))? }; @@ -610,8 +579,20 @@ impl Server { )?; // Check if this is a checkin of an artifact. + let artifacts_path = { + let _permit = self.file_descriptor_semaphore.acquire().await.unwrap(); + let mut artifacts_path = None; + for ancestor in path.ancestors().skip(1) { + let path = ancestor.join(".tangram/artifacts"); + if matches!(tokio::fs::try_exists(&path).await, Ok(true)) { + artifacts_path.replace(path); + break; + } + } + artifacts_path.unwrap_or(self.artifacts_path()) + }; let path = crate::util::path::diff(&arg.path, &target_absolute_path)?; - let edge = if let Ok(subpath) = target_absolute_path.strip_prefix(self.artifacts_path()) { + let edge = if let Ok(subpath) = target_absolute_path.strip_prefix(artifacts_path) { let mut components = subpath.components(); let object = components .next() @@ -701,15 +682,6 @@ impl Server { } impl Graph { - async fn contains_path(&self, path: &Path) -> bool { - for node in &self.nodes { - if node.arg.path == path { - return true; - } - } - false - } - #[allow(dead_code)] pub(super) async fn print(&self) { let mut visited = BTreeSet::new(); diff --git a/packages/server/src/artifact/checkin/output.rs b/packages/server/src/artifact/checkin/output.rs index 13136e407..69fef4a89 100644 --- a/packages/server/src/artifact/checkin/output.rs +++ b/packages/server/src/artifact/checkin/output.rs @@ -715,8 +715,22 @@ impl Server { output: &Graph, symlink: usize, ) -> tg::Result { - // Get the outgoing edge. - if let Some(output_edge) = output.nodes[symlink].edges.first() { + // Get the artifact and subpath, if they exist.. + let artifact_and_subpath = output.nodes[symlink] + .edges + .first() + .map(|edge| (output.nodes[edge.node].id.clone(), edge.subpath.clone())) + .or_else(|| { + let tg::artifact::Data::Symlink(tg::symlink::Data::Normal { + artifact: Some(artifact), + subpath, + }) = output.nodes[symlink].data.clone() + else { + return None; + }; + Some((artifact, subpath)) + }); + if let Some((artifact, subpath)) = artifact_and_subpath { // Find how deep this node is in the output tree. let mut input_index = output.nodes[symlink].input; let mut depth = 0; @@ -729,8 +743,8 @@ impl Server { } // Create the target. - let id = output.nodes[output_edge.node].id.to_string(); - let subpath = output_edge.subpath.clone().unwrap_or_default(); + let id = artifact.to_string(); + let subpath = subpath.unwrap_or_default(); let components = (0..depth) .map(|_| std::path::Component::ParentDir) @@ -739,16 +753,19 @@ impl Server { )))) .chain(subpath.components()); - Ok(components.collect()) - } else if let tg::artifact::Data::Symlink(tg::symlink::Data::Normal { + return Ok(components.collect()); + } + + // Otherwise write the target directly. + if let tg::artifact::Data::Symlink(tg::symlink::Data::Normal { artifact: None, subpath: Some(target), }) = &output.nodes[symlink].data { - Ok(target.clone()) - } else { - return Err(tg::error!("invalid symlink")); + return Ok(target.clone()); } + + Err(tg::error!(?symlink = &output.nodes[symlink], "invalid symlink")) } } diff --git a/packages/server/src/artifact/checkin/tests.rs b/packages/server/src/artifact/checkin/tests.rs index 68fb5c059..7cf2e6e93 100644 --- a/packages/server/src/artifact/checkin/tests.rs +++ b/packages/server/src/artifact/checkin/tests.rs @@ -1,7 +1,7 @@ use crate::{util::fs::cleanup, Config, Server}; use futures::{Future, FutureExt as _}; use insta::assert_snapshot; -use std::{panic::AssertUnwindSafe, pin::pin}; +use std::{panic::AssertUnwindSafe, path::PathBuf, pin::pin}; use tangram_client as tg; use tangram_futures::stream::TryStreamExt as _; use tangram_temp::{self as temp, Temp}; @@ -633,6 +633,50 @@ async fn destructive_package() -> tg::Result<()> { .await } +#[tokio::test] +async fn external_symlink_roundtrip() -> tg::Result<()> { + let temp = Temp::new(); + let config = Config::with_path(temp.path().to_owned()); + let server = Server::start(config).await?; + let result = AssertUnwindSafe(async { + let directory = tg::directory! { + "file" => "contents", + } + .into(); + let symlink = tg::symlink!(Some(directory), Some(PathBuf::from("file"))); + + // Check out the artifact. + let temp = Temp::new(); + let arg = tg::artifact::checkout::Arg { + dependencies: true, + force: false, + path: Some(temp.path().to_owned()), + }; + let path = tg::Artifact::from(symlink.clone()) + .check_out(&server, arg) + .await?; + + // Check the artifact back in. + let arg = tg::artifact::checkin::Arg { + deterministic: true, + destructive: false, + ignore: true, + locked: true, + path, + }; + let artifact = tg::Artifact::check_in(&server, arg) + .await? + .try_unwrap_symlink() + .map_err(|_| tg::error!("expected a symlink"))?; + assert_eq!(artifact.id(&server).await?, symlink.id(&server).await?); + Ok::<_, tg::Error>(()) + }) + .catch_unwind() + .await; + cleanup(temp, server).await; + result.unwrap() +} + async fn test( artifact: temp::Artifact, path: &str, diff --git a/packages/server/src/artifact/checkout.rs b/packages/server/src/artifact/checkout.rs index 007fa651d..a3ec0e5e7 100644 --- a/packages/server/src/artifact/checkout.rs +++ b/packages/server/src/artifact/checkout.rs @@ -218,10 +218,9 @@ impl Server { let artifact_path = self.artifacts_path().join(id.to_string()); // If there is already a file system object at the path, then return. - if tokio::fs::try_exists(&path) - .await - .map_err(|source| tg::error!(!source, %path = path.display(), "failed to stat the path"))? - { + if tokio::fs::try_exists(&path).await.map_err( + |source| tg::error!(!source, %path = path.display(), "failed to stat the path"), + )? { let output = tg::artifact::checkout::Output { path: artifact_path, };