Implemented different with_native_path conversion methods, refactoring fs functions to utilize with_native_path, and move exists to use with_native_path

[asder8215]

View all comments

This PR attempts to complete the FIXME comment on trying to introduce the different conversion functions of with_native_path on all supported platforms and also moves the exists filesystem functions to utilize with_native_path. It should be easier to make readdir and remove_dir_all utilize with_native_path, which I'm down to support that transition in a separate PR.

I took note on what each platform fs functions does underneath the hood to decide the argument it should take:

• On Hermit and Vexos they use run_path_with_cstr just like Unix, so it was clear for their fs functions to take a &CStr
• Motor always converts the &Path to &str, so I just changed the arguments to &str
• Solid uses an internal cstr function that converts a &Path to io::Result<CString>, so I made its fs functions take in a &CStr anyways
• On uefi, as far as I'm aware every of its fs function ends up calling crate::path::absolute(returns a Result<PathBuf>) or uefi_fs::File::from_path, which calls on crate::path::absolute anyways, so it made sense for me to make its fs functions take a PathBuf.

I did some other refactoring such as putting with_native_path functions within std/src/sys/path (except for unsupported.rs which has with_native_path implemented within its file). I thought it made sense to centralize the different with_native_path (aside from run_path_with_cstr can just use the alias with_native_path) functions there considering that's done for Windows. I also renamed the unsupported_backslash.rs file to solid.rs because it seems like solid is the only one that utilizes that file; the unix.rs file was renamed to common.rs because that's used by unix platforms, motor, and other miscellaneous platforms (motor platform has a different with_native_path implementation however).

If there's anything here that I should revert or change from platform to platform, just let me know.

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: @ChrisDenton, libs
• @ChrisDenton, libs expanded to 13 candidates
• Random selection from 6 candidates

[asder8215]

To give a heads up, this PR might be better to wait on #158168 to merge in first, so that I can have with_native_path supported easily on set_perms_nofollow on all platforms.

[asder8215]

This should be ready now, just resolved conflicts on set_permissions_nofollow.

[rustbot]

The run-make-support library was changed

cc @jieyouxu

Some changes occurred in src/tools/cargo

cc @weihanglo

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[asder8215]

I screwed up, I have to redo this again 🤦

[Mark-Simulacrum]

r=me with nit fixed, please squash the commits

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[asder8215]

I've been wondering this for some time, but what does the "r=me" thing mean? I know you said this before in a previous PR I made and I've also noticed other reviewers say "r=me" in comments, but I'm not sure what it means.

[Mark-Simulacrum]

r=me means that approval is given conditional (typically) on some minor change. It's a signal to future me or other reviewers that if that change is made it's safe to not re-review the rest of the PR.

See more here: https://rustc-dev-guide.rust-lang.org/walkthrough.html?highlight=r%3Dme#implementation

@bors r+

[rust-bors]

📌 Commit f632c9a has been approved by Mark-Simulacrum

It is now in the queue for this repository.

[JonathanBrouwer]

💔 I suspect this PR failed tests as part of a rollup
@bors r-

After fixing the problem, consider running a try job for the failed job before re-approving.

Link to failure: #160810 (comment)

[rust-bors]

This pull request was unapproved.

This PR was contained in a rollup (#160810), which was unapproved.

View changes since this unapproval

[asder8215]

I think I need a cfg gate on this for uefi to do something different with PathBuf?

View changes since the review

[asder8215]

I'm a bit confused. The first old argument is a &Path, which is converted to a PathBuf within with_native_path (second &|old| should be a &|PathBuf|), which I would have thought that this old would have been moved as an argument for imp::rename?

[asder8215]

Should this be a move closure?