Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UnixFileMode: address remaining feedback. #71188

Merged
merged 1 commit into from
Jun 23, 2022

Conversation

tmds
Copy link
Member

@tmds tmds commented Jun 23, 2022

This addresses the open comments of #69980.

cc @eerhardt @dotnet/area-system-io

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jun 23, 2022
@ghost
Copy link

ghost commented Jun 23, 2022

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

This addresses the open comments of #69980.

cc @eerhardt @dotnet/area-system-io

Author: tmds
Assignees: -
Labels:

area-System.IO, community-contribution

Milestone: -

@tmds
Copy link
Member Author

tmds commented Jun 23, 2022

Looks like SystemNative_FChMod entrypoint is now unused and can be deleted.

It's used here:

int rv = handle is not null ? Interop.Sys.FChMod(handle, (int)mode)
: Interop.Sys.ChMod(path!, (int)mode);

And it's available publicly now via the new File.SetUnixFileMode(SafeFileHandle fileHandle, UnixFileMode mode).

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you @tmds !

@adamsitnik adamsitnik merged commit 20698b4 into dotnet:main Jun 23, 2022
adamsitnik pushed a commit to adamsitnik/runtime that referenced this pull request Jun 23, 2022
Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @tmds!

carlossanlop pushed a commit that referenced this pull request Jun 23, 2022
* Implement UnixFileMode APIs (#69980)

* Implement UnixFileMode APIs on Unix.

* Throw PNSE on Windows, add UnsupportedOSPlatform.

* Fix API compat issue.

* Borrow a few things from SafeFileHandle API PR to this compiles.

* Fix System.IO.FileSystem.AccessControl compilation.

* Add xml docs.

* Replace Interop.Sys.Permissions to System.IO.UnixFileMode.

* Throw PNSE immediately on Windows.

* Add ODE to xml docs of methods that accept a handle.

* Don't throw (PNSE) from FileSystemInfo.UnixFileMode getter on Windows.

* Minor style fix.

* Get rid of some casts.

Co-authored-by: Eric Erhardt <[email protected]>

* Add tests for creating a file/directory with UnixFileMode.

* Some CI envs don't have a umask exe, try retrieving via a shell builtin.

* Update expected test mode values.

* Fix OSX

* Fix Windows build.

* Add ArgumentException tests.

* Fix Windows build.

* Add get/set tests.

* Update test for Windows.

* Make setters target link instead of link target.

* Linux: fix SetUnixFileMode

* Fix OSX compilation.

* Try make all tests pass in CI.

* For link, operate on target permissions.

* Skip tests on Browser.

* Add tests for 'Get' that doesn't use a 'Set' first.

* Don't perform exist check for handles.

* Fix Get test for wasm.

* Review xml comments.

* Add comment to test.

* GetUnixFileMode for handle won't throw UnauthorizedAccessException.

* Apply suggestions from code review

Co-authored-by: Eric Erhardt <[email protected]>

* PR feedback.

* Update enum doc to say 'owner' instead of 'user'.

* Use UnixFileMode in library.

* Use UnixFileMode in library tests.

* Fix Windows build.

* Fix missing FileAccess when changing to FileStreamOptions API.

* PR feedback.

* Fix Argument_InvalidUnixCreateMode message.

Co-authored-by: Adam Sitnik <[email protected]>

Co-authored-by: Eric Erhardt <[email protected]>
Co-authored-by: Adam Sitnik <[email protected]>

* UnixFileMode: address remaining feedback. (#71188)

Co-authored-by: Tom Deseyn <[email protected]>
Co-authored-by: Eric Erhardt <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.IO community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants