From e6338371c836b1d0cde351c111312d96abb221f2 Mon Sep 17 00:00:00 2001 From: Stu Hood Date: Fri, 12 May 2023 10:05:13 -0700 Subject: [PATCH] `Snapshot`s can now calculate their own digests without a `Scheduler`, so replace `_unsafe_create` with `create_for_testing`. --- .../pants/backend/project_info/peek_test.py | 12 ++--- src/python/pants/core/goals/fix_test.py | 32 +++--------- src/python/pants/engine/fs_test.py | 51 ++++--------------- .../pants/engine/internals/native_engine.pyi | 4 +- src/rust/engine/fs/store/src/snapshot.rs | 13 ++--- src/rust/engine/src/externs/fs.rs | 13 ++--- 6 files changed, 33 insertions(+), 92 deletions(-) diff --git a/src/python/pants/backend/project_info/peek_test.py b/src/python/pants/backend/project_info/peek_test.py index 60443a5b346..01546599315 100644 --- a/src/python/pants/backend/project_info/peek_test.py +++ b/src/python/pants/backend/project_info/peek_test.py @@ -15,14 +15,14 @@ from pants.base.specs import RawSpecs, RecursiveGlobSpec from pants.core.target_types import ArchiveTarget, FilesGeneratorTarget, FileTarget, GenericTarget from pants.engine.addresses import Address -from pants.engine.fs import Digest, Snapshot +from pants.engine.fs import Snapshot from pants.engine.internals.dep_rules import DependencyRuleAction, DependencyRuleApplication from pants.engine.rules import QueryRule from pants.testutil.rule_runner import RuleRunner def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot: - return Snapshot._unsafe_create(Digest(fingerprint.ljust(64, "0"), 1), files, ()) + return Snapshot.create_for_testing(files, ()) @pytest.mark.parametrize( @@ -74,7 +74,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot: "bar.txt", "foo.txt" ], - "sources_fingerprint": "2000000000000000000000000000000000000000000000000000000000000000", + "sources_fingerprint": "d3dd0a1f72aaa1fb2623e7024d3ea460b798f6324805cfad5c2b751e2dfb756b", "sources_raw": [ "*.txt" ] @@ -111,7 +111,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot: "sources": [ "foo.txt" ], - "sources_fingerprint": "1000000000000000000000000000000000000000000000000000000000000000", + "sources_fingerprint": "b5e73bb1d7a3f8c2e7f8c43f38ab4d198e3512f082c670706df89f5abe319edf", "sources_raw": [ "foo.txt" ], @@ -158,7 +158,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot: "target_type": "files", "dependencies": [], "sources": [], - "sources_fingerprint": "0000000000000000000000000000000000000000000000000000000000000000", + "sources_fingerprint": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", "sources_raw": [ "*.txt" ], @@ -239,7 +239,7 @@ def _snapshot(fingerprint: str, files: tuple[str, ...]) -> Snapshot: "sources": [ "foo/a.txt" ], - "sources_fingerprint": "0000000000000000000000000000000000000000000000000000000000000000", + "sources_fingerprint": "72ceef751c940b5797530e298f4d9f66daf3c51f7d075bfb802295ffb01d5de3", "sources_raw": [ "*.txt" ] diff --git a/src/python/pants/core/goals/fix_test.py b/src/python/pants/core/goals/fix_test.py index 5109780687c..ebd44129d32 100644 --- a/src/python/pants/core/goals/fix_test.py +++ b/src/python/pants/core/goals/fix_test.py @@ -28,7 +28,6 @@ from pants.core.util_rules import source_files from pants.core.util_rules.partitions import PartitionerType from pants.engine.fs import ( - EMPTY_DIGEST, EMPTY_SNAPSHOT, CreateDigest, Digest, @@ -410,12 +409,8 @@ def test_no_targets() -> None: def test_message_lists_added_files() -> None: - input_snapshot = Snapshot._unsafe_create( - Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"] - ) - output_snapshot = Snapshot._unsafe_create( - Digest("b" * 64, 1000), ["f.ext", "added.ext", "dir/f.ext"], ["dir"] - ) + input_snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"]) + output_snapshot = Snapshot.create_for_testing(["f.ext", "added.ext", "dir/f.ext"], ["dir"]) result = FixResult( input=input_snapshot, output=output_snapshot, @@ -427,12 +422,8 @@ def test_message_lists_added_files() -> None: def test_message_lists_removed_files() -> None: - input_snapshot = Snapshot._unsafe_create( - Digest("a" * 64, 1000), ["f.ext", "removed.ext", "dir/f.ext"], ["dir"] - ) - output_snapshot = Snapshot._unsafe_create( - Digest("b" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"] - ) + input_snapshot = Snapshot.create_for_testing(["f.ext", "removed.ext", "dir/f.ext"], ["dir"]) + output_snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"]) result = FixResult( input=input_snapshot, output=output_snapshot, @@ -444,14 +435,8 @@ def test_message_lists_removed_files() -> None: def test_message_lists_files() -> None: - # _unsafe_create() cannot be used to simulate changed files, - # so just make sure added and removed work together. - input_snapshot = Snapshot._unsafe_create( - Digest("a" * 64, 1000), ["f.ext", "removed.ext", "dir/f.ext"], ["dir"] - ) - output_snapshot = Snapshot._unsafe_create( - Digest("b" * 64, 1000), ["f.ext", "added.ext", "dir/f.ext"], ["dir"] - ) + input_snapshot = Snapshot.create_for_testing(["f.ext", "removed.ext", "dir/f.ext"], ["dir"]) + output_snapshot = Snapshot.create_for_testing(["f.ext", "added.ext", "dir/f.ext"], ["dir"]) result = FixResult( input=input_snapshot, output=output_snapshot, @@ -530,8 +515,7 @@ class FixKitchenRequest(FixTargetsRequest): def test_streaming_output_changed(caplog) -> None: caplog.set_level(logging.DEBUG) - changed_digest = Digest(EMPTY_DIGEST.fingerprint, 2) - changed_snapshot = Snapshot._unsafe_create(changed_digest, [], []) + changed_snapshot = Snapshot.create_for_testing(["other_file.txt"], []) result = FixResult( input=EMPTY_SNAPSHOT, output=changed_snapshot, @@ -540,7 +524,7 @@ def test_streaming_output_changed(caplog) -> None: tool_name="fixer", ) assert result.level() == LogLevel.WARN - assert result.message() == "fixer made changes." + assert result.message() == "fixer made changes.\n other_file.txt" assert ["Output from fixer\nstdout\nstderr"] == [ rec.message for rec in caplog.records if rec.levelno == logging.DEBUG ] diff --git a/src/python/pants/engine/fs_test.py b/src/python/pants/engine/fs_test.py index f598b82fd2a..bc2830b6070 100644 --- a/src/python/pants/engine/fs_test.py +++ b/src/python/pants/engine/fs_test.py @@ -15,7 +15,7 @@ from http.server import BaseHTTPRequestHandler from io import BytesIO from pathlib import Path -from typing import Callable, Dict, Iterable, List, Optional, Set, Union +from typing import Callable, Dict, Iterable, Optional, Set, Union import pytest @@ -1399,49 +1399,20 @@ def test_digest_is_not_file_digest() -> None: def test_snapshot_properties() -> None: - digest = Digest("691638f4d58abaa8cfdc9af2e00682f13f07f96ad1d177f146216a7341ca4982", 154) - snapshot = Snapshot._unsafe_create(digest, ["f.ext", "dir/f.ext"], ["dir"]) - assert snapshot.digest == digest + snapshot = Snapshot.create_for_testing(["f.ext", "dir/f.ext"], ["dir"]) + assert snapshot.digest is not None assert snapshot.files == ("dir/f.ext", "f.ext") assert snapshot.dirs == ("dir",) -def test_snapshot_hash() -> None: - def assert_hash( - expected: int, - *, - digest_char: str = "a", - files: Optional[List[str]] = None, - dirs: Optional[List[str]] = None, - ) -> None: - digest = Digest(digest_char * 64, 1000) - snapshot = Snapshot._unsafe_create(digest, files or ["f.ext", "dir/f.ext"], dirs or ["dir"]) - assert hash(snapshot) == expected - - # The digest's fingerprint is used for the hash, so all other properties are irrelevant. - assert_hash(-6148914691236517206) - assert_hash(-6148914691236517206, files=["f.ext"]) - assert_hash(-6148914691236517206, dirs=["foo"]) - assert_hash(-6148914691236517206, dirs=["foo"]) - assert_hash(-4919131752989213765, digest_char="b") - - -def test_snapshot_equality() -> None: - # Only the digest is used for equality. - snapshot = Snapshot._unsafe_create(Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"]) - assert snapshot == Snapshot._unsafe_create( - Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"] - ) - assert snapshot == Snapshot._unsafe_create( - Digest("a" * 64, 1000), ["f.ext", "dir/f.ext"], ["foo"] - ) - assert snapshot == Snapshot._unsafe_create(Digest("a" * 64, 1000), ["f.ext"], ["dir"]) - assert snapshot != Snapshot._unsafe_create(Digest("a" * 64, 0), ["f.ext", "dir/f.ext"], ["dir"]) - assert snapshot != Snapshot._unsafe_create( - Digest("b" * 64, 1000), ["f.ext", "dir/f.ext"], ["dir"] - ) - with pytest.raises(TypeError): - snapshot < snapshot # type: ignore[operator] +def test_snapshot_hash_and_eq() -> None: + one = Snapshot.create_for_testing(["f.ext"], ["dir"]) + two = Snapshot.create_for_testing(["f.ext"], ["dir"]) + assert hash(one) == hash(two) + assert one == two + three = Snapshot.create_for_testing(["f.ext"], []) + assert hash(two) != hash(three) + assert two != three @pytest.mark.parametrize( diff --git a/src/python/pants/engine/internals/native_engine.pyi b/src/python/pants/engine/internals/native_engine.pyi index ba60b4fe7df..c3a4172402d 100644 --- a/src/python/pants/engine/internals/native_engine.pyi +++ b/src/python/pants/engine/internals/native_engine.pyi @@ -87,9 +87,7 @@ class Snapshot: """ @classmethod - def _unsafe_create( - cls, digest: Digest, files: Sequence[str], dirs: Sequence[str] - ) -> Snapshot: ... + def create_for_testing(cls, files: Sequence[str], dirs: Sequence[str]) -> Snapshot: ... @property def digest(self) -> Digest: ... @property diff --git a/src/rust/engine/fs/store/src/snapshot.rs b/src/rust/engine/fs/store/src/snapshot.rs index 0f69484c691..cd48b614d93 100644 --- a/src/rust/engine/fs/store/src/snapshot.rs +++ b/src/rust/engine/fs/store/src/snapshot.rs @@ -177,14 +177,8 @@ impl Snapshot { } } - /// # Safety - /// - /// This should only be used for testing, as this will always create an invalid Snapshot. - pub unsafe fn create_for_testing_ffi( - digest: Digest, - files: Vec, - dirs: Vec, - ) -> Result { + /// Creates a snapshot containing empty Files for testing purposes. + pub fn create_for_testing(files: Vec, dirs: Vec) -> Result { // NB: All files receive the EMPTY_DIGEST. let file_digests = files .iter() @@ -216,8 +210,7 @@ impl Snapshot { &file_digests, )?; Ok(Self { - // NB: The DigestTrie's computed digest is ignored in favor of the given Digest. - digest, + digest: tree.compute_root_digest(), tree, }) } diff --git a/src/rust/engine/src/externs/fs.rs b/src/rust/engine/src/externs/fs.rs index bf0a9295e7f..fb5f8dc3441 100644 --- a/src/rust/engine/src/externs/fs.rs +++ b/src/rust/engine/src/externs/fs.rs @@ -157,15 +157,10 @@ pub struct PySnapshot(pub Snapshot); #[pymethods] impl PySnapshot { #[classmethod] - fn _unsafe_create( - _cls: &PyType, - py_digest: PyDigest, - files: Vec, - dirs: Vec, - ) -> PyResult { - let snapshot = - unsafe { Snapshot::create_for_testing_ffi(py_digest.0.as_digest(), files, dirs) }; - Ok(Self(snapshot.map_err(PyException::new_err)?)) + fn create_for_testing(_cls: &PyType, files: Vec, dirs: Vec) -> PyResult { + Ok(Self( + Snapshot::create_for_testing(files, dirs).map_err(PyException::new_err)?, + )) } fn __hash__(&self) -> u64 {