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

[asder8215]

I screwed up, I have to redo this again 🤦

[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?

[asder8215]

Not really a big fan of this (though this only affects UEFI). As it is right now, I'm sort of forced to use a clone here with UEFI converting &Path -> PathBuf. Unsure if there's a way I can avoid the clone here on a dyn callback.

I think what's possible is that we can convert with_native_path to use an impl Fn instead of &dyn Fn. The thing with doing that is it'll require a bit more refactoring since when I attempted to do that the compiler asked me to give concrete types on the captured variable within the closure block. However, I'm also aware that using an impl Fn will make this a compile time cost (generate multiple with_native_path per filesystem functions that calls on with_native_path) and potentially increase binary size. Not sure if that is okay or ideal to do.

View changes since the review

[asder8215]

Forgot to do this
@rustbot ready

[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 hope you don't mind me reassigning this PR to you @joboet. If/Once CI turns green, could you also run a try job on test-various to see if this compiles/runs that fine?

r? joboet

[rustbot]

joboet is currently at their maximum review capacity.
They may take a while to respond.

[asder8215]

Decided to put the re-export here instead of in unix_like.rs because I realized that with_native_path implementation would conflict with motor.rs' and unsupported.rs' with_native_path implementation as they both re-export unix_like (unsupported platforms had this behavior before, if you want me to remove re-exporting of unix_like from unsupported platforms, I could do that).

View changes since the review