Skip to content

Commit

Permalink
Snapshots of single files store normalized paths. (pantsbuild#10707)
Browse files Browse the repository at this point in the history
Previously we were not enforcing or normalizing relative paths. There
are two callers, `DownloadedFile::load_or_download` and the
`create_digest_to_digest` intrinsic. The former happens to always pass a
normalized relative path due to the parsing behavior of `Url` but the
latter did no validation or normalization taking a path string directly
via cpython. A normalized relative path is now enforced by demanding a
`RelativePath`.
  • Loading branch information
jsirois authored Aug 30, 2020
1 parent cf483ea commit b173bcb
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 10 deletions.
6 changes: 6 additions & 0 deletions src/rust/engine/fs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,12 @@ impl AsRef<Path> for RelativePath {
}
}

impl Into<PathBuf> for RelativePath {
fn into(self) -> PathBuf {
self.0
}
}

#[derive(Clone, Debug, Eq, Hash, PartialEq)]
pub enum Stat {
Link(Link),
Expand Down
6 changes: 3 additions & 3 deletions src/rust/engine/fs/store/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,7 @@ impl Store {
/// Store a digest under a given file path, returning a Snapshot
pub async fn snapshot_of_one_file(
&self,
name: PathBuf,
name: RelativePath,
digest: hashing::Digest,
is_executable: bool,
) -> Result<Snapshot, String> {
Expand All @@ -322,9 +322,9 @@ impl Store {
self.clone(),
Digester { digest },
vec![fs::PathStat::File {
path: name.clone(),
path: name.clone().into(),
stat: fs::File {
path: name,
path: name.into(),
is_executable: is_executable,
},
}],
Expand Down
4 changes: 3 additions & 1 deletion src/rust/engine/src/intrinsics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::nodes::{lift_digest, DownloadedFile, NodeResult, Snapshot};
use crate::tasks::Intrinsic;
use crate::types::Types;

use fs::RelativePath;
use futures::compat::Future01CompatExt;
use futures::future::{self, BoxFuture, FutureExt, TryFutureExt};
use indexmap::IndexMap;
Expand Down Expand Up @@ -311,12 +312,13 @@ fn create_digest_to_digest(
.iter()
.map(|file| {
let filename = externs::project_str(&file, "path");
let path: PathBuf = filename.into();
let bytes = bytes::Bytes::from(externs::project_bytes(&file, "content"));
let is_executable = externs::project_bool(&file, "is_executable");

let store = context.core.store();
async move {
let path = RelativePath::new(PathBuf::from(filename))
.map_err(|e| format!("The file `path` must be relative: {:?}", e))?;
let digest = store.store_file_bytes(bytes, true).await?;
let snapshot = store
.snapshot_of_one_file(path, digest, is_executable)
Expand Down
14 changes: 8 additions & 6 deletions src/rust/engine/src/nodes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -629,15 +629,17 @@ impl DownloadedFile {
.and_then(Iterator::last)
.map(str::to_owned)
.ok_or_else(|| format!("Error getting the file name from the parsed URL: {}", url))?;

let path = RelativePath::new(&file_name).map_err(|e| {
format!(
"The file name derived from {} was {} which is not relative: {:?}",
&url, &file_name, e
)
})?;
let maybe_bytes = core.store().load_file_bytes_with(digest, |_| ()).await?;
if maybe_bytes.is_none() {
DownloadedFile::download(core.clone(), url, file_name.clone(), digest).await?;
DownloadedFile::download(core.clone(), url, file_name, digest).await?;
}
core
.store()
.snapshot_of_one_file(PathBuf::from(file_name), digest, true)
.await
core.store().snapshot_of_one_file(path, digest, true).await
}

async fn download(
Expand Down

0 comments on commit b173bcb

Please sign in to comment.