Merge 8aa33e5 into 0137271

src/python/pants/backend/codegen/protobuf/python/rules.py

@@ -10,7 +10,15 @@
 from pants.core.util_rules.source_files import SourceFilesRequest
 from pants.core.util_rules.stripped_source_files import StrippedSourceFiles
 from pants.engine.addresses import Addresses
-from pants.engine.fs import AddPrefix, Digest, MergeDigests, RemovePrefix, Snapshot
+from pants.engine.fs import (
+    AddPrefix,
+    CreateDigest,
+    Digest,
+    Directory,
+    MergeDigests,
+    RemovePrefix,
+    Snapshot,
+)
 from pants.engine.platform import Platform
 from pants.engine.process import Process, ProcessResult
 from pants.engine.rules import Get, MultiGet, collect_rules, rule
@@ -34,16 +42,7 @@ async def generate_python_from_protobuf(
     )
 
     output_dir = "_generated_files"
-    # TODO(#9650): replace this with a proper intrinsic to create empty directories.
-    create_output_dir_request = Get(
-        ProcessResult,
-        Process(
-            ("/bin/mkdir", output_dir),
-            description=f"Create the directory {output_dir}",
-            level=LogLevel.TRACE,
-            output_directories=(output_dir,),
-        ),
-    )
+    create_output_dir_request = Get(Digest, CreateDigest([Directory(output_dir)]))
 
     # Protoc needs all transitive dependencies on `protobuf_libraries` to work properly. It won't
     # actually generate those dependencies; it only needs to look at their .proto files to work
@@ -64,7 +63,7 @@ async def generate_python_from_protobuf(
 
     (
         downloaded_protoc_binary,
-        create_output_dir_result,
+        empty_output_dir,
         all_sources_stripped,
         target_sources_stripped,
     ) = await MultiGet(
@@ -80,7 +79,7 @@ async def generate_python_from_protobuf(
             (
                 all_sources_stripped.snapshot.digest,
                 downloaded_protoc_binary.digest,
-                create_output_dir_result.output_digest,
+                empty_output_dir,
             )
         ),
     )

src/python/pants/backend/python/goals/coverage_py_test.py

@@ -19,6 +19,7 @@ def run_create_coverage_config_rule(coverage_config: Optional[str]) -> str:
     def mock_handle_config(request: CreateDigest) -> Digest:
         assert len(request) == 1
         assert request[0].path == ".coveragerc"
+        assert isinstance(request[0], FileContent)
         assert request[0].is_executable is False
         resolved_config.append(request[0].content.decode())
         return Digest("jerry", 30)

src/python/pants/backend/python/util_rules/BUILD

@@ -3,4 +3,4 @@
 
 python_library()
 
-python_tests(name="tests")
+python_tests(name="tests", timeout=90)

src/python/pants/backend/python/util_rules/pex_cli.py

@@ -14,7 +14,7 @@
     ExternalTool,
     ExternalToolRequest,
 )
-from pants.engine.fs import CreateDigest, Digest, FileContent, MergeDigests
+from pants.engine.fs import CreateDigest, Digest, Directory, MergeDigests
 from pants.engine.internals.selectors import MultiGet
 from pants.engine.platform import Platform
 from pants.engine.process import Process
@@ -101,9 +101,7 @@ async def setup_pex_cli_process(
     tmpdir = ".tmp"
     downloaded_pex_bin, tmp_dir_digest = await MultiGet(
         Get(DownloadedExternalTool, ExternalToolRequest, pex_binary.get_request(Platform.current)),
-        # TODO(John Sirois): Use a Directory instead of this FileContent hack when a fix for
-        #  https://github.com/pantsbuild/pants/issues/9650 lands.
-        Get(Digest, CreateDigest([FileContent(f"{tmpdir}/.reserve", b"")])),
+        Get(Digest, CreateDigest([Directory(f"{tmpdir}/.reserve")])),
     )
 
     digests_to_merge = [downloaded_pex_bin.digest, tmp_dir_digest]

src/python/pants/engine/fs.py

@@ -3,7 +3,7 @@
 
 from dataclasses import dataclass
 from enum import Enum
-from typing import TYPE_CHECKING, Iterable, Optional, Tuple
+from typing import TYPE_CHECKING, Iterable, Optional, Tuple, Union
 
 from pants.engine.collection import Collection
 from pants.engine.rules import QueryRule, side_effecting
@@ -53,7 +53,11 @@ class Paths:
 
 @dataclass(frozen=True)
 class FileContent:
-    """The content of a file."""
+    """The content of a file.
+
+    This can be used to create a new Digest with `Get(Digest, CreateDigest)`. You can also get back
+    a list of `FileContent` objects by using `Get(DigestContents, Digest)`.
+    """
 
     path: str
     content: bytes
@@ -66,12 +70,29 @@ def __repr__(self) -> str:
         )
 
 
+@dataclass(frozen=True)
+class Directory:
+    """The path to a directory.
+
+    This can be used to create empty directories with `Get(Digest, CreateDigest)`.
+    """
+
+    path: str
+
+    def __repr__(self) -> str:
+        return f"Directory({repr(self.path)})"
+
+
 class DigestContents(Collection[FileContent]):
     """The file contents of a Digest."""
 
 
-class CreateDigest(Collection[FileContent]):
-    """A request to create a Digest with the input FileContent values.
+class CreateDigest(Collection[Union[FileContent, Directory]]):
+    """A request to create a Digest with the input FileContent and/or Directory values.
+
+    The engine will create any parent directories necessary, e.g. `FileContent('a/b/c.txt')` will
+    result in `a/`, `a/b`, and `a/b/c.txt` being created. You only need to use `Directory` to
+    create an empty directory.
 
     This does _not_ actually materialize the digest to the build root. You must use
     `engine.fs.Workspace` in a `@goal_rule` to save the resulting digest to disk.

src/python/pants/engine/fs_test.py

@@ -26,6 +26,7 @@
     Digest,
     DigestContents,
     DigestSubset,
+    Directory,
     DownloadFile,
     FileContent,
     GlobMatchErrorBehavior,
@@ -72,6 +73,7 @@ class FSTest(FSTestBase):
     def rules(cls):
         return (
             *super().rules(),
+            QueryRule(Snapshot, (CreateDigest,)),
             QueryRule(Snapshot, (DigestSubset,)),
         )
 
@@ -421,9 +423,7 @@ def test_add_prefix(self) -> None:
         assert digest == output_digest
 
         # Illegal.
-        with self.assertRaisesRegex(
-            Exception, r"Cannot add component .*ParentDir.* of path prefix `../something`."
-        ):
+        with self.assertRaisesRegex(Exception, r"The `prefix` must be relative."):
             self.request(Digest, [AddPrefix(digest, "../something")])
 
     def test_remove_prefix(self) -> None:
@@ -502,6 +502,19 @@ def test_remove_prefix(self) -> None:
                 "directory contained non-matching directory named: books and file named: index"
             ) in str(exc.value)
 
+    def test_create_empty_directory(self) -> None:
+        res = self.request(Snapshot, [CreateDigest([Directory("a/")])])
+        assert res.dirs == ("a",)
+        assert not res.files
+        assert res.digest != EMPTY_DIGEST
+
+        res = self.request(
+            Snapshot, [CreateDigest([Directory("x/y/z"), Directory("m"), Directory("m/n")])]
+        )
+        assert res.dirs == ("m", "m/n", "x", "x/y", "x/y/z")
+        assert not res.files
+        assert res.digest != EMPTY_DIGEST
+
     def test_lift_digest_to_snapshot(self) -> None:
         digest = self.prime_store_with_roland_digest()
         snapshot = self.request(Snapshot, [digest])

src/rust/engine/fs/fs_util/src/main.rs

@@ -26,7 +26,7 @@
 #![allow(clippy::new_without_default, clippy::new_ret_no_self)]
 // Arc<Mutex> can be more clear than needing to grok Orderings:
 #![allow(clippy::mutex_atomic)]
-#![type_length_limit = "1881004"]
+#![type_length_limit = "1881024"]
 
 use std::convert::TryInto;
 use std::io::{self, Write};
@@ -38,7 +38,7 @@ use std::time::Duration;
 use boxfuture::{BoxFuture, Boxable};
 use bytes::Bytes;
 use clap::{value_t, App, Arg, SubCommand};
-use fs::GlobMatching;
+use fs::{GlobMatching, RelativePath};
 use futures::compat::Future01CompatExt;
 use futures::future::TryFutureExt;
 use futures01::{future, Future};
@@ -491,7 +491,7 @@ async fn execute(top_match: &clap::ArgMatches<'_>) -> Result<(), ExitError> {
 
         if let Some(prefix_to_strip) = args.value_of("child-dir") {
           digest = store
-            .strip_prefix(digest, PathBuf::from(prefix_to_strip))
+            .strip_prefix(digest, RelativePath::new(PathBuf::from(prefix_to_strip))?)
             .await
             .map_err(|err| match err {
               SnapshotOpsError::String(string)

src/rust/engine/fs/store/src/lib.rs

@@ -374,7 +374,7 @@ impl Store {
   ///
   pub async fn record_directory(
     &self,
-    directory: &bazel_protos::remote_execution::Directory,
+    directory: &remexec::Directory,
     initial_lease: bool,
   ) -> Result<Digest, String> {
     let local = self.local.clone();
@@ -396,15 +396,15 @@ impl Store {
   pub async fn load_directory(
     &self,
     digest: Digest,
-  ) -> Result<Option<(bazel_protos::remote_execution::Directory, LoadMetadata)>, String> {
+  ) -> Result<Option<(remexec::Directory, LoadMetadata)>, String> {
     self
       .load_bytes_with(
         EntryType::Directory,
         digest,
         // Trust that locally stored values were canonical when they were written into the CAS,
         // don't bother to check this, as it's slightly expensive.
         move |bytes: &[u8]| {
-          let mut directory = bazel_protos::remote_execution::Directory::new();
+          let mut directory = remexec::Directory::new();
           directory.merge_from_bytes(&bytes).map_err(|e| {
             format!(
               "LMDB corruption: Directory bytes for {:?} were not valid: {:?}",
@@ -416,7 +416,7 @@ impl Store {
         // Eagerly verify that CAS-returned Directories are canonical, so that we don't write them
         // into our local store.
         move |bytes: Bytes| {
-          let mut directory = bazel_protos::remote_execution::Directory::new();
+          let mut directory = remexec::Directory::new();
           directory.merge_from_bytes(&bytes).map_err(|e| {
             format!(
               "CAS returned Directory proto for {:?} which was not valid: {:?}",
@@ -1004,12 +1004,7 @@ impl Store {
   ///
   pub fn walk<
     T: Send + 'static,
-    F: Fn(
-        &Store,
-        &PathBuf,
-        Digest,
-        &bazel_protos::remote_execution::Directory,
-      ) -> BoxFuture<T, String>
+    F: Fn(&Store, &PathBuf, Digest, &remexec::Directory) -> BoxFuture<T, String>
       + Send
       + Sync
       + 'static,
@@ -1032,12 +1027,7 @@ impl Store {
 
   fn walk_helper<
     T: Send + 'static,
-    F: Fn(
-        &Store,
-        &PathBuf,
-        Digest,
-        &bazel_protos::remote_execution::Directory,
-      ) -> BoxFuture<T, String>
+    F: Fn(&Store, &PathBuf, Digest, &remexec::Directory) -> BoxFuture<T, String>
       + Send
       + Sync
       + 'static,

src/rust/engine/fs/store/src/snapshot_ops.rs

@@ -7,8 +7,8 @@ use async_trait::async_trait;
 use bazel_protos::remote_execution as remexec;
 use bytes::Bytes;
 use fs::{
-  ExpandablePathGlobs, GitignoreStyleExcludes, PathGlob, PreparedPathGlobs, DOUBLE_STAR_GLOB,
-  SINGLE_STAR_GLOB,
+  ExpandablePathGlobs, GitignoreStyleExcludes, PathGlob, PreparedPathGlobs, RelativePath,
+  DOUBLE_STAR_GLOB, SINGLE_STAR_GLOB,
 };
 use futures::future::{self as future03, FutureExt, TryFutureExt};
 use glob::Pattern;
@@ -20,7 +20,7 @@ use log::log_enabled;
 use std::collections::HashSet;
 use std::convert::From;
 use std::iter::Iterator;
-use std::path::{Component, Path, PathBuf};
+use std::path::{Path, PathBuf};
 use std::sync::Arc;
 
 #[derive(Clone, Eq, PartialEq, Hash, Debug)]
@@ -662,23 +662,11 @@ pub trait SnapshotOps: StoreWrapper + 'static {
   async fn add_prefix(
     &self,
     mut digest: Digest,
-    prefix: PathBuf,
+    prefix: RelativePath,
   ) -> Result<Digest, SnapshotOpsError> {
-    let mut components = prefix.components();
-    while let Some(parent) = components.next_back() {
-      let parent = match &parent {
-        Component::Normal(p) => p,
-        x => {
-          return Err(
-            format!(
-              "Cannot add component \"{:?}\" of path prefix `{}`.",
-              x,
-              prefix.display(),
-            )
-            .into(),
-          )
-        }
-      };
+    let prefix: PathBuf = prefix.into();
+    let mut prefix_iter = prefix.iter();
+    while let Some(parent) = prefix_iter.next_back() {
       let mut dir_node = remexec::DirectoryNode::new();
       dir_node.set_name(osstring_as_utf8(parent.to_os_string())?);
       dir_node.set_digest((&digest).into());
@@ -695,11 +683,11 @@ pub trait SnapshotOps: StoreWrapper + 'static {
   async fn strip_prefix(
     &self,
     root_digest: Digest,
-    prefix: PathBuf,
+    prefix: RelativePath,
   ) -> Result<Digest, SnapshotOpsError> {
     let mut dir = self.load_directory_or_err(root_digest).await?;
     let mut already_stripped = PathBuf::new();
-    let mut prefix = prefix;
+    let mut prefix: PathBuf = prefix.into();
     loop {
       let has_already_stripped_any = already_stripped.components().next().is_some();
 
@@ -779,7 +767,12 @@ pub trait SnapshotOps: StoreWrapper + 'static {
     let SubsetParams { globs } = params;
     snapshot_glob_match(self.clone(), digest, globs).await
   }
+
+  async fn create_empty_dir(&self, path: RelativePath) -> Result<Digest, SnapshotOpsError> {
+    self.add_prefix(EMPTY_DIGEST, path).await
+  }
 }
+
 impl<T: StoreWrapper + 'static> SnapshotOps for T {}
 
 struct PartiallyExpandedDirectoryContext {

src/rust/engine/fs/store/src/snapshot_ops_tests.rs

@@ -5,7 +5,7 @@ use testutil::make_file;
 use crate::{
   snapshot_ops::StoreWrapper,
   snapshot_tests::{expand_all_sorted, setup, STR, STR2},
-  OneOffStoreFileByDigest, Snapshot, SnapshotOps, Store, SubsetParams,
+  OneOffStoreFileByDigest, RelativePath, Snapshot, SnapshotOps, Store, SubsetParams,
 };
 use bazel_protos::remote_execution as remexec;
 use fs::{GlobExpansionConjunction, PosixFS, PreparedPathGlobs, StrictGlobMatching};
@@ -187,8 +187,9 @@ async fn subset_tracking_load_counts() {
   )
   .await;
 
+  let prefix = RelativePath::new(PathBuf::from("subdir")).unwrap();
   let subdir_digest = load_tracking_store
-    .strip_prefix(merged_digest, PathBuf::from("subdir"))
+    .strip_prefix(merged_digest, prefix)
     .await
     .unwrap();