Added implementation on set_permissions_nofollow for all primary platforms#158168
Added implementation on set_permissions_nofollow for all primary platforms#158168asder8215 wants to merge 1 commit into
set_permissions_nofollow for all primary platforms#158168Conversation
|
r? @clarfonthey rustbot has assigned @clarfonthey. Use Why was this reviewer chosen?The reviewer was selected based on:
|
3411717 to
96bc9a1
Compare
This comment has been minimized.
This comment has been minimized.
| } | ||
|
|
||
| pub fn set_perm_nofollow(path: &Path, perm: FilePermissions) -> io::Result<()> { | ||
| set_perm(path, perm) |
There was a problem hiding this comment.
Similar to UEFI, I would swap the method bodies here, so that set_perm defers to set_perm_nofollow plus a comment that says symlinks aren't supported.
|
@rustbot author CI failure appears to be a simple typo but I also left some feedback on other stuff to fix. Otherwise, implementation looks good, although I would add an extra comment on the tracking issue once this is merged to have some Windows folks double-check that opening all reparse points in this method doesn't do anything we don't want, since that technically includes more than just symlinks. It should work in pretty much all normal use cases, just want to double-check the edge cases before stabilisation. |
|
Reminder, once the PR becomes ready for a review, use |
96bc9a1 to
f9c87ed
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
f9c87ed to
feafe24
Compare
This comment has been minimized.
This comment has been minimized.
feafe24 to
b286acf
Compare
This comment has been minimized.
This comment has been minimized.
b286acf to
14df6fa
Compare
This comment has been minimized.
This comment has been minimized.
14df6fa to
7029495
Compare
|
r=me minus one small mistake, plus figuring out why the tests aren't running atm. |
e62cd46 to
18fa184
Compare
This comment has been minimized.
This comment has been minimized.
c739454 to
a7222bd
Compare
This comment has been minimized.
This comment has been minimized.
|
This is the same error as #156269. I'm not sure what the CI error is for. |
…pported (windows, unix, uefi, etc.); modified the Unix implementation to use fchmodat instead of open + fchmod; clarified documentations for set_permissions_nofollow; added a test case for set_permissions_nofollow
a7222bd to
4804060
Compare
|
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. |
|
@clarfonthey Finally got this to work with CI (sorry about the many force pushes!). Let me know if you want me to remove the |
|
Force-pushes are totally fine; I can deal. Will try and take a look later today. |
|
Just kidding, had some extra time to check this out and it looks good to me. @bors r+ rollup=iffy (touches multiple targets that might not be tested in PR CI. if rollup fails, we can do a dedicated try job first.) |
…nofollow, r=clarfonthey Added implementation on `set_permissions_nofollow` for all primary platforms For context, this PR is related to the tracking issue for [`std::fs::set_permissions_nofollow`](rust-lang#141607). This PR does a few different things: * Windows support is provided for `std::fs::set_permissions_nofollow`. On Windows, it uses `OpenOptions::open` with the custom flag `FILE_FLAG_OPEN_REPARSE_POINT` enabled and then sets the permissions on the reparse point directly. All other platforms (hermit, motor, solid, uefi, etc.) defer to what `fs::set_permissions` does since symlinks aren't supported on those platforms, so they effectively do the same thing. * The implementation for Unix was modified to use [`fchmodat`](https://linux.die.net/man/2/fchmodat) instead of doing two separate calls (`open` and then `fchmod` via `OpenOptions`). * Because the previous implementations actually used `fchmod` instead of `chmod` underneath the hood (as that's what `File::set_permissions`, which is not to be confused with `fs::set_permissions` as that does use `chmod`, does underneath the hood), it actually had some incorrect documentation about the error returned by `fchmod` on symlinks. Instead of `io::ErrorKind::FilesystemLoop`, it returns `io::ErrorKind::InvalidInput`. You can read the documentation for [`fchmod`](https://pubs.opengroup.org/onlinepubs/009696699/functions/fchmod.html). `fchmodat` does effectively the same thing as the `open` + `fchmod` combo, but it does return a different error, `io::ErrorKind::Unsupported`, that makes more sense. Regardless, I corrected the documentation for `std::fs::set_permissions_nofollow` and added a lot more clarifying information. * A test case has been added to verify if `set_permissions_nofollow` is operating correctly. cc @ChrisDenton and @lolbinarycat
|
💔 I suspect this PR failed tests as part of a rollup After fixing the problem, consider running a try job for the failed job before re-approving. Link to failure: #158316 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#158316), which was unapproved. |
|
@bors try |
|
⌛ Trying commit 4804060 with merge d289242… To cancel the try build, run the command Workflow: https://github.com/rust-lang/rust/actions/runs/28049621544 |
…=<try> Added implementation on `set_permissions_nofollow` for all primary platforms
|
@bors try cancel Brain fart, yeah, will want to double-check before running since we know it fails. |
|
Try build cancelled. Cancelled workflows: |
View all comments
For context, this PR is related to the tracking issue for
std::fs::set_permissions_nofollow. This PR does a few different things:std::fs::set_permissions_nofollow. On Windows, it usesOpenOptions::openwith the custom flagFILE_FLAG_OPEN_REPARSE_POINTenabled and then sets the permissions on the reparse point directly. All other platforms (hermit, motor, solid, uefi, etc.) defer to whatfs::set_permissionsdoes since symlinks aren't supported on those platforms, so they effectively do the same thing.fchmodatinstead of doing two separate calls (openand thenfchmodviaOpenOptions).fchmodinstead ofchmodunderneath the hood (as that's whatFile::set_permissions, which is not to be confused withfs::set_permissionsas that does usechmod, does underneath the hood), it actually had some incorrect documentation about the error returned byfchmodon symlinks. Instead ofio::ErrorKind::FilesystemLoop, it returnsio::ErrorKind::InvalidInput. You can read the documentation forfchmod.fchmodatdoes effectively the same thing as theopen+fchmodcombo, but it does return a different error,io::ErrorKind::Unsupported, that makes more sense. Regardless, I corrected the documentation forstd::fs::set_permissions_nofollowand added a lot more clarifying information.set_permissions_nofollowis operating correctly.cc @ChrisDenton and @lolbinarycat