Added implementation on set_permissions_nofollow for all primary platforms - #158168
Conversation
|
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
…=<try> Added implementation on `set_permissions_nofollow` for all primary platforms try-job: dist-android
|
@bors r+ rollup=iffy Multitarget code that has failed us before, but hopefully shouldn't this time… |
|
💔 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: #159952 (comment) |
|
This pull request was unapproved. This PR was contained in a rollup (#159952), which was unapproved. |
There was a problem hiding this comment.
On Windows you can set attributes on symlinks (they're just normal files or directories).
There was a problem hiding this comment.
Got you. Does setting an attribute on symlinks do anything meaningful? I was reading up on MacOS and they also can set permission bits on symlinks, but they don't do anything meaningful.
There was a problem hiding this comment.
Yes, it can be meaningful but only to the extent that setting the readonly attribute is meaningful. See Permissions::set_readonly, which notes that the readonly attribute on a directory is pretty useless nowadays. Tbh the name "Permissions" is a bit of misnomer on Windows as it doesn't set actual permissions.
|
@bors try jobs=x86_64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
|
💔 Test for 4e6ca17 failed: CI. Failed job:
|
This comment has been minimized.
This comment has been minimized.
|
Okay, that confirms to me that Windows actually sets permissions on the |
…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
|
@bors try jobs=x86_64-msvc-1 |
This comment has been minimized.
This comment has been minimized.
|
Just to double-check: I think this is ready to merge again? And should work? |
All I can say is hopefully it works. It succeeded on |
|
Don't worry; this has been quite a surprising journey and I appreciate you continuing to travel down this path of madness. I'm content with just trying again, honestly. @bors try rollup=iffy Fifth(?) time's a charm? Multi-platform code that continues to find platforms that misbehave. |
|
Unknown argument "rollup". Did you mean to use |
|
🤦🏻 @bors r+ rollup=iffy Fifth(?) time's a charm? Multi-platform code that continues to find platforms that misbehave. |
| /// let mut perms = fs::symlink_metadata("foo.txt")?.permissions(); | ||
| /// perms.set_readonly(true); | ||
| /// // This should result in an error on certain platforms | ||
| /// // or succeed in modifying the permissions of a symlink |
There was a problem hiding this comment.
Or it might do nothing at all apparently, according to the comment above?
There was a problem hiding this comment.
Yea, I forgot to update this once I noticed that it could do nothing at all
| /// if the final element is a symlink. On BSD-based systems, the | ||
| /// behavior can vary from symlink permission bits changing or | ||
| /// there being no effects on symlinks | ||
| /// |
There was a problem hiding this comment.
| /// if the final element is a symlink. On BSD-based systems, the | |
| /// behavior can vary from symlink permission bits changing or | |
| /// there being no effects on symlinks | |
| /// | |
| /// if the final element is a symlink. On BSD-based systems, the | |
| /// behavior in this case can vary: the operation may have no effect at all | |
| /// or it may change the permission bits of the symlink itself. | |
| /// |
"can vary from... or ..." is not grammatical.
There was a problem hiding this comment.
Note: this has already been merged, so, any suggestions would have to be under a separate PR. Could raise this in the tracking issue though.
There was a problem hiding this comment.
I know...
@asder8215 any chance you could make a follow-up PR for doc clarifications and grammar fixes?
There was a problem hiding this comment.
(apologies for missing this, by the way; after enough CI failures I was just incredibly happy this merged successfully)
There was a problem hiding this comment.
Yeah I get it, it's hard to be on the look for docs nuances after fighting with the actual code for a while.
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