Add StoreDriver::list for FilesystemStore

[jhpratt]

part of #995

Description

This implements and tests the list method of the StoreDriver trait for FilesystemStore.

Type of change

Please delete options that aren't relevant.

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to
  not work as expected)
• This change requires a documentation update

How Has This Been Tested?

Please also list any relevant details for your test configuration

Checklist

• Updated documentation if needed
• Tests added/amended
• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[cloudflare-pages]

Deploying nativelink with    Cloudflare Pages

Latest commit:	3f5a12f
Status:	✅  Deploy successful!
Preview URL:	https://03cd4a4a.nativelink-3e2.pages.dev
Branch Preview URL:	https://list-api-fs-store.nativelink-3e2.pages.dev

View logs

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and 5 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 1546 at r3 (raw file):

        .unwrap()
        .as_millis();
    let content_path = format!("content_path_{timestamp}");

use: make_temp_path like the other tests in this file.

---

nativelink-store/tests/filesystem_store_test.rs line 1550 at r3 (raw file):

    // Remove, but here due to previous error seen by Jacob in CI
    std::fs::create_dir_all(&content_path)

nit: remove std:: and use the async tokio functions like the rest of this file does.

---

nativelink-store/tests/filesystem_store_test.rs line 1590 at r3 (raw file):

    store
        .update_oneshot(StoreKey::Digest(expected_digest1), VALUE.into())

Why not do what list_test in memory_store_test does?

    const KEY1: StoreKey = StoreKey::new_str("key1");
    const KEY2: StoreKey = StoreKey::new_str("key2");
    const KEY3: StoreKey = StoreKey::new_str("key3");
    const VALUE: &str = "value1";
    store.update_oneshot(KEY1, VALUE.into()).await?;
    store.update_oneshot(KEY2, VALUE.into()).await?;
    store.update_oneshot(KEY3, VALUE.into()).await?;

---

nativelink-store/tests/filesystem_store_test.rs line 1606 at r3 (raw file):

        store
            .list(range, |key| {
                let key_owned = key.borrow().into_owned();

nit: Inline this, like it is in the other test.

---

nativelink-store/tests/filesystem_store_test.rs line 1618 at r3 (raw file):

        // Test listing all keys.
        let keys = get_list(&store, ..).await;
        assert_eq!(

nit:

        let keys = get_list(&store, ..).await;
        assert_eq!(keys, vec![KEY1, KEY2, KEY3]);

Like the other test?

[allada]

Reviewable status: 0 of 1 LGTMs obtained, and 6 discussions need to be resolved

---

nativelink-store/Cargo.toml line 42 at r3 (raw file):

shellexpand = "3.1.0"
tempfile = "3.10.1"
tokio = { version = "1.37.0", features = ["rt-multi-thread"] }

nit: Why bring in real time support into this module?

[jhpratt]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Cloudflare Pages, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, vale, windows-2022 / stable, and 3 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 1546 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

use: make_temp_path like the other tests in this file.

done

---

nativelink-store/tests/filesystem_store_test.rs line 1550 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: remove std:: and use the async tokio functions like the rest of this file does.

done

---

nativelink-store/tests/filesystem_store_test.rs line 1590 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Why not do what list_test in memory_store_test does?

    const KEY1: StoreKey = StoreKey::new_str("key1");
    const KEY2: StoreKey = StoreKey::new_str("key2");
    const KEY3: StoreKey = StoreKey::new_str("key3");
    const VALUE: &str = "value1";
    store.update_oneshot(KEY1, VALUE.into()).await?;
    store.update_oneshot(KEY2, VALUE.into()).await?;
    store.update_oneshot(KEY3, VALUE.into()).await?;

The StoreKey goes through SHA256 in hash_key, as is the case in memory_store_test. I could use the pre-existing method (namely StoreKey::into_digest) to auto-create it, but that uses the Blake3 hash. Presumably that would cause a compatibility issue?

---

nativelink-store/tests/filesystem_store_test.rs line 1606 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Inline this, like it is in the other test.

done

---

nativelink-store/tests/filesystem_store_test.rs line 1618 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit:

        let keys = get_list(&store, ..).await;
        assert_eq!(keys, vec![KEY1, KEY2, KEY3]);

Like the other test?

KEY1 & others are now &strs, not StoreKeys, so this wouldn't work. PartialEq isn't implemented between the two types (nor should they be imo). This is related to the concern on line 1590 above, so they'll likely be resolved together.

---

nativelink-store/Cargo.toml line 42 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

nit: Why bring in real time support into this module?

done

[jhpratt]

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Bazel Dev / ubuntu-22.04, Cargo Dev / macos-13, Cargo Dev / ubuntu-22.04, Installation / macos-13, Installation / macos-14, Installation / ubuntu-22.04, Local / ubuntu-22.04, Publish image, Publish nativelink-worker-init, Publish nativelink-worker-lre-cc, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), macos-13, pre-commit-checks, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable, and 4 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 200 at r5 (raw file):

    let separator = std::path::MAIN_SEPARATOR_STR;
    format!(
        "{}{separator}{}{separator}{}",

make_temp_path elsewhere in the repository apparently does not touch the OS file system, so the path separator does not matter. That is not the case here, so we need to use \ for Windows and / for everything else. The standard library provides a constant (whose value is OS-dependent) for this purpose, which is what's used here.

One alternative, if it's desired, is to go through a proper PathBuf.

[allada]

Reviewed 1 of 2 files at r6.
Reviewable status: 0 of 1 LGTMs obtained, and 4 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 1590 at r3 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

The StoreKey goes through SHA256 in hash_key, as is the case in memory_store_test. I could use the pre-existing method (namely StoreKey::into_digest) to auto-create it, but that uses the Blake3 hash. Presumably that would cause a compatibility issue?

Hmmm, this is an oversite in the FilesystemStore. Can we fix FilesystemStore first then land this PR?

I know this PR is really close, but it will make the test much cleaner.

Here's some code I started:
https://gist.github.com/allada/dab6535799d044224c439f2ff97d4deb

I created a ticket here:
#1108

---

nativelink-store/tests/filesystem_store_test.rs line 200 at r5 (raw file):

Previously, jhpratt (Jacob Pratt) wrote…

make_temp_path elsewhere in the repository apparently does not touch the OS file system, so the path separator does not matter. That is not the case here, so we need to use \ for Windows and / for everything else. The standard library provides a constant (whose value is OS-dependent) for this purpose, which is what's used here.

One alternative, if it's desired, is to go through a proper PathBuf.

Modern versions of windows worth with either since ~WindowsXP.

---

nativelink-store/tests/filesystem_store_test.rs line 205 at r6 (raw file):

    )

    // env::var_os("TEST_TMPDIR")

remove.

[jhpratt]

Reviewable status: 0 of 1 LGTMs obtained, and 4 discussions need to be resolved

---

nativelink-store/tests/filesystem_store_test.rs line 1590 at r3 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Hmmm, this is an oversite in the FilesystemStore. Can we fix FilesystemStore first then land this PR?

I know this PR is really close, but it will make the test much cleaner.

Here's some code I started:
https://gist.github.com/allada/dab6535799d044224c439f2ff97d4deb

I created a ticket here:
#1108

Sure, I'll take a look into that.

---

nativelink-store/tests/filesystem_store_test.rs line 200 at r5 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

Modern versions of windows worth with either since ~WindowsXP.

Is it capable of handling mixing and matching? CI was failing on Windows before this change, but it passes afterwards.

---

nativelink-store/tests/filesystem_store_test.rs line 205 at r6 (raw file):

Previously, allada (Nathan (Blaise) Bruer) wrote…

remove.

Done. That was me experimenting with PathBuf stuff and I forgot to remove it outright.

[adam-singer]

Reviewed 1 of 2 files at r6, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained, and 4 discussions need to be resolved