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

[API Implementation]: SafeFileHandle overloads for SetCreationTime, SetLastAccessTime, SetLastWriteTime #60507

Merged
merged 142 commits into from
Jul 12, 2022

Conversation

deeprobin
Copy link
Contributor

@deeprobin deeprobin commented Oct 16, 2021

Proposal implementation of #20234 (closes #20234)

Proposal

namespace System.IO
{
    public static partial class File
    {
        public static DateTime GetCreationTime(SafeFileHandle fileHandle);
        public static DateTime GetCreationTimeUtc(SafeFileHandle fileHandle);
        public static DateTime GetLastAccessTime(SafeFileHandle fileHandle);
        public static DateTime GetLastAccessTimeUtc(SafeFileHandle fileHandle);
        public static DateTime GetLastWriteTime(SafeFileHandle fileHandle);
        public static DateTime GetLastWriteTimeUtc(SafeFileHandle fileHandle);

        public static void SetCreationTime(SafeFileHandle fileHandle, DateTime creationTime);
        public static void SetCreationTimeUtc(SafeFileHandle fileHandle, DateTime creatinTimeUtc);
        public static void SetLastAccessTime(SafeFileHandle fileHandle, DateTime lastAccessTime);
        public static void SetLastAccessTimeUtc(SafeFileHandle fileHandle, DateTime lastAccessTimeUtc);
        public static void SetLastWriteTime(SafeFileHandle fileHandle, DateTime lastWriteTime);
        public static void SetLastWriteTimeUtc(SafeFileHandle fileHandle, DateTime lastWriteTimeUtc);

        public static FileAttributes GetAttributes(SafeFileHandle fileHandle);
        public static void SetAttributes(SafeFileHandle fileHandle, FileAttributes fileAttributes);
    }
}

Current state of implementation

  • Proposal logic / implementation
    • Windows
    • Unix
  • Ref Assembly
  • Tests

/cc @Liryna

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 16, 2021
@ghost
Copy link

ghost commented Oct 16, 2021

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

Issue Details

Proposal implementation of #20234

⚠️ This is a draft

/cc @Liryna

Author: deeprobin
Assignees: -
Labels:

area-System.IO, community-contribution

Milestone: -

@deeprobin
Copy link
Contributor Author

@danmoseley Do the files added under Common/src/Interop/Windows/Kernel32/ (see CI failures) have to be registered somewhere so that they can be resolved?

@danmoseley
Copy link
Member

@deeprobin if you look at the build log you should see the project file on the end of the error:

2021-10-22T19:42:24.8792107Z D:\a\_work\2\s\src\libraries\Common\src\Interop\Windows\Kernel32\Interop.WIN32_FILE_ATTRIBUTE_DATA.cs(27,44): error CS0246: The type or namespace name 'BY_HANDLE_FILE_INFORMATION' could not be found (are you missing a using directive or an assembly reference?) [D:\a\_work\2\s\src\libraries\System.IO.FileSystem.AccessControl\src\System.IO.FileSystem.AccessControl.csproj]

You've introduced use of WIN32_FILE_ATTRIBUTE_DATA into a file in this project: the project file needs updating to include its source file.

@danmoseley
Copy link
Member

@deeprobin are you able to build the libraries locally successfully? An incremental build under src\libraries should be less than a minute, and ought to have thrown up these errors locally, which would be quicker for you than waiting an hour or whatever for the PR validation system.

@danmoseley
Copy link
Member

Generally folks don't push changes one at a time, because it restarts PR validation, which is extensive -- 50+ configurations -- and that uses machine capacity. What I suggest is that you wait to push until you are satisfied with your changes and have done basic build and tests locally, then push everything (squashed or not as you prefer). Another option is that I mark this PR as draft, so that pushes don't kick off more validation (I think). What are your thoughts

@deeprobin
Copy link
Contributor Author

Generally folks don't push changes one at a time, because it restarts PR validation, which is extensive -- 50+ configurations -- and that uses machine capacity. What I suggest is that you wait to push until you are satisfied with your changes and have done basic build and tests locally, then push everything (squashed or not as you prefer). Another option is that I mark this PR as draft, so that pushes don't kick off more validation (I think). What are your thoughts

Unfortunately, VS support doesn't currently work for me with .NET 7, so I've always validated that via CI.

@danmoseley
Copy link
Member

VS support doesn't currently work for me with .NET 7

You can build and run tests outside of VS. this is what I do. There is all the info you need in the workflow docs in this repo.

@danmoseley
Copy link
Member

@deeprobin it looks like there's unix specific build errors. You don't need to be on a Unix box to reproduce those. Something like this at the root of the repo:
build clr.corelib -rc release -lc debug -os linux
or to build all the libraries
build clr.corelib+libs -rc release -lc debug -os linux

There should be a way to do it with dotnet build, but I have to check how.

@deeprobin
Copy link
Contributor Author

@danmoseley Thank you. I'll fixed it (the unix-build is now passing locally)

@Jozkee Jozkee added the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 4, 2021
@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Dec 11, 2021
@danmoseley
Copy link
Member

It looks like you need to rebase on main as I made a concurrent change. It should be easy.

@danmoseley
Copy link
Member

Can you please open a new issue for your failure:


Process terminated. Assertion failed.
ClientCertificates. Expected enumerable cloned value.
   at System.Net.Security.SslClientAuthenticationOptionsExtensions.ShallowClone(SslClientAuthenticationOptions options) in /_/src/libraries/Common/src/System/Net/Security/SslClientAuthenticationOptionsExtensions.cs:line 41
   at System.Net.Http.HttpConnectionSettings.CloneAndNormalize() in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/HttpConnectionSettings.cs:line 85
   at System.Net.Http.SocketsHttpHandler.SetupHandlerChain() in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs:line 486
   at System.Net.Http.SocketsHttpHandler.SendAsync(HttpRequestMessage request, CancellationToken cancellationToken) in /_/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/SocketsHttpHandler.cs:line 576

@danmoseley
Copy link
Member

Cc @wfurt

@deeprobin
Copy link
Contributor Author

@Jozkee @danmoseley
I think this PR is ready-for-merge or do you find any possible improvements?

@deeprobin
Copy link
Contributor Author

@Jozkee Can you give another review (or approve)?

deeprobin and others added 4 commits July 11, 2022 22:37
@deeprobin deeprobin requested a review from Jozkee July 11, 2022 21:09
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
src/libraries/System.Private.CoreLib/src/System/IO/File.cs Outdated Show resolved Hide resolved
@Jozkee
Copy link
Member

Jozkee commented Jul 12, 2022

@deeprobin if you are not planning on addressing the documentation feedback, could you please file an issue for it?

@deeprobin
Copy link
Contributor Author

@deeprobin if you are not planning on addressing the documentation feedback, could you please file an issue for it?

I'll address it. Give me one hour ;)

Co-Authored-By: David Cantú <jozkyy@gmail.com>
@deeprobin
Copy link
Contributor Author

@Jozkee Doc changes applied

@Jozkee
Copy link
Member

Jozkee commented Jul 12, 2022

CI error is #66625

@Jozkee Jozkee merged commit 15cb373 into dotnet:main Jul 12, 2022
@Jozkee
Copy link
Member

Jozkee commented Jul 12, 2022

Thanks, @deeprobin.

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.

SetCreationTime, SetLastAccessTime, SetLastWriteTime Should not open a new stream to obtain a SafeFileHandle
8 participants