Refactor fs.rs to use call_with_permit scheme

[zbirenbaum]

Description

Refactor fs.rs to use call_with_permit scheme

Checklist

• bazel test //... passes locally
• PR is contained in a single commit, using git amend see some docs

---

This change is 

[aaronmondal]

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: 0 of 1 LGTMs obtained

---

-- commits line 2 at r1:
nit: Would be nice to have a bit more information (just like one or two sentences) in the body of the commit message.

---

nativelink-util/src/fs.rs line 285 at r1 (raw file):

        Ok(res) => res,
        Err(_) => Err(make_err!(Code::Internal, "background task failed")),
    }

Suggestion:

    tokio::task::spawn_blocking(move || f(permit))
        .await
        .unwrap_or_else(|e| Err(make_err!(Code::Internal, "background task failed: {e:?}")))

---

nativelink-util/src/fs.rs line 434 at r1 (raw file):

}

pub async fn rename(from: impl AsRef<Path>, to: impl AsRef<Path>) -> Result<(), Error> {

nit: I really appreciate these simplifications. Something I noticed is that I think we could change all the function signatures to use PathBuf like so:

pub async fn rename(from: PathBuf, to: PathBuf) -> Result<(), Error> {
    call_with_permit(move |_| std::fs::rename(from, to).map_err(Into::into)).await
}

I played around with this a bit and it seems like using PathBuf instead of all the manual Cow and OsStr also makes the filesystem store much easier on the eyes.

---

nativelink-util/src/fs.rs line 437 at r1 (raw file):

    let from = from.as_ref().to_owned();
    let to = to.as_ref().to_owned();
    call_with_permit(move |_| std::fs::rename(from, to).map_err(|e| e.into())).await

nit: You could change all the |e| e.into() to Into::into or Into::<Error>::into.

[aaronmondal]

Reviewable status: 1 of 1 LGTMs obtained

[zbirenbaum]

Reviewable status: 1 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Bazel Dev / ubuntu-22.04, Local / ubuntu-22.04, Vercel, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), pre-commit-checks, publish-image, ubuntu-20.04 / stable, ubuntu-22.04, vale

---

nativelink-util/src/fs.rs line 285 at r1 (raw file):

        Ok(res) => res,
        Err(_) => Err(make_err!(Code::Internal, "background task failed")),
    }

I like this as well

---

nativelink-util/src/fs.rs line 434 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: I really appreciate these simplifications. Something I noticed is that I think we could change all the function signatures to use PathBuf like so:

pub async fn rename(from: PathBuf, to: PathBuf) -> Result<(), Error> {
    call_with_permit(move |_| std::fs::rename(from, to).map_err(Into::into)).await
}

I played around with this a bit and it seems like using PathBuf instead of all the manual Cow and OsStr also makes the filesystem store much easier on the eyes.

Changing the signature here breaks types in a couple other places. I'm happy to make the changes but not sure if I should in this PR since this is a backport of other code that was already shipped. Let me know your thoughts

---

nativelink-util/src/fs.rs line 437 at r1 (raw file):

Previously, aaronmondal (Aaron Siddhartha Mondal) wrote…

nit: You could change all the |e| e.into() to Into::into or Into::<Error>::into.

Into::<Error>::into seems a bit clearer as to what's going on so I'll go with that

[allada]

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 2 of 1 LGTMs obtained

---

nativelink-util/src/fs.rs line 434 at r1 (raw file):

Previously, zbirenbaum (Zach Birenbaum) wrote…

Changing the signature here breaks types in a couple other places. I'm happy to make the changes but not sure if I should in this PR since this is a backport of other code that was already shipped. Let me know your thoughts

Yeah, I'd use PathBuf when all possible. Cow is just to remove the need for a copy when able, but PathBuf can suffice in many/all cases.

[aaronmondal]

Reviewed all commit messages.
Reviewable status: 2 of 1 LGTMs obtained

---

nativelink-util/src/fs.rs line 434 at r1 (raw file):

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

Yeah, I'd use PathBuf when all possible. Cow is just to remove the need for a copy when able, but PathBuf can suffice in many/all cases.

Let's defer these signature changes to another PR.

[aaronmondal]

FYI @zbirenbaum You'll need to click the "resolve" and "acknowledge buttons in Reviewable, that'll unblock this PR for merging.

Reviewable status: 2 of 1 LGTMs obtained

[adam-singer]

Reviewed all commit messages.
Reviewable status: complete! 3 of 1 LGTMs obtained

[notion-workspace]

Refactor fs.rs with call_with_permit scheme