Fix setting creation date for OSX-like platforms #49555

hamarb123 · 2021-03-12T21:16:09Z

Fixes #39132
This is the replacement of #39263.

This commit adds a fallback for setattrlist failing. It can sometimes fail on specific volume types, so this is a workaround to use the old behaviour on unsupported filesystems. Fix for #39132

…ossible stack overflow This commit adds _fileStatusInitialized = -1; which resets the cache for the file status after setting a time value to the file using the setattrlist api version so that it will not simply return the last cached time. It also fixes SetCreationTime_OtherUnix method so that it calls SetLastWriteTime_OtherUnix directly instead of SetLastWriteTime because otherwise you'll get stuck in an infinite loop on OSX when the fallback is theoretically needed. Fix for #39132

hamarb123 · 2021-03-17T09:25:30Z

@carlossanlop the PR should be ready assuming all the tests passes (they did locally), but I think we should add 2 more tests:

A test for setting modification date before the creation date as this was a possible issue with the old version of my api (SetCreationTime on Mac #39132 (comment)) and possibly the other possible orders of date file/folder metadata (I think that a good way to test this would be, for example, 1. set mod date to 2002, 2. set creation date to 2001, 3. set mod date to 2000 and then 4. check mod and creation dates)
And a test for symlinks / other OS equivalents because the setattrlist API, by default, will follow symlinks which is not standard behaviour with the Windows version and also doesn't make sense to me (my code sets FSOPT_NOFOLLOW to avoid this behaviour), we should add this test to protect future changes to these APIs & to check other OSes like Linux.

What are your thoughts on these tests?

hamarb123 · 2021-03-17T18:47:52Z

@carlossanlop I've discovered that the issue with #39132 (comment) also applies to this code, so I'm going to push a fix soon, I'll probably wait until we decide whether to add a test or not as to reduce spinning up testing machines.
I think we should also definitely add the tests because of this.
(the order of creation time 2002, mod time 2001, creation time 2000 was the only one that failed on macOS with my changes)

carlossanlop

Thanks for the PR, @hamarb123! I left a couple of comments.

A test for setting modification date before the creation date as this was a possible issue with the old version of my api and possibly the other possible orders of date file/folder metadata (I think that a good way to test this would be, for example, 1. set mod date to 2002, 2. set creation date to 2001, 3. set mod date to 2000 and then 4. check mod and creation dates)

Sounds good. Add the test and we can take a look.

And a test for symlinks

Makes sense to keep it consistent with the Windows behavior with symlinks. A good thing to keep in mind is that some of the symlinks behavior has some open issues, both in Windows and in Unix (I have a stale draft PR to address the Unix one, will eventually get back to it). If/when the time comes, we can come back to the Mac code and determine if we need to adjust it to address one of those issues.

src/libraries/System.IO.FileSystem/src/System.IO.FileSystem.csproj

src/libraries/Common/src/Interop/OSX/Interop.libc.cs

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.OSX.cs

hamarb123 · 2021-03-18T07:02:38Z

Makes sense to keep it consistent with the Windows behavior with symlinks. A good thing to keep in mind is that some of the symlinks behavior has some open issues, both in Windows and in Unix (I have a stale draft PR to address the Unix one, will eventually get back to it). If/when the time comes, we can come back to the Mac code and determine if we need to adjust it to address one of those issues.

In that case, I'll skip that test for now and someone can add it when the those other issues are fixed.

Fix the behaviour of SetLastWriteTime (OSX) when setting lastwritetime before creationtime. Use Invalidate() instead of _fileStatusInitialized = -1, and only when appropriate as per PR #49555. Add SettingUpdatesPropertiesAfterAnother test to test things such as setting lastwritetime to before creationtime. Rename the added _OtherUnix methods to _StandardUnixImpl, a more logical name given their purpose (#49555) Fix for #39132 and issues in #49555.

hamarb123 · 2021-03-18T07:58:05Z

I've made changes to the code, @carlossanlop feel free to re-review when the checks pass (they theoretically might fail on linux for example since I don't have a linux machine to test it on). Feel free to read my responses to your comments in the meantime and ask any more questions / request other changes.

hamarb123 · 2021-03-18T13:17:38Z

I'll disable the test for browser in the next commit, not sure what android arm64 is doing, but the other androids worked fine, so I'll ignore it.

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs

carlossanlop · 2021-03-18T16:15:26Z

In that case, I'll skip that test for now and someone can add it when the those other issues are fixed.

I think you can add your test for symlinks, and add a comment explaining that this test is currently consistent with the Windows behavior. If you hit a problem, and we have an issue open, you can top the test with the [ActiveIssue] attribute.

Added a new test to test the behaviour of the file time functions on symlinks as per standard Windows behaviour, and also what I use in my code today. It makes sure that the times are set on the symlink itself, not the target. It is likely to fail on many unix platforms, so these will be given the [ActiveIssue] attribute when we discover which ones. Make access time be set using setattrlist on OSX to fix following symlinks issue that would've failed the new test. Fix and update wording of comment in SetAccessOrWriteTime. Add T CreateSymlinkToItem(T item) to BaseGetSetTimes<T> (and implementation in inheritors) for the new test. Made the SettingUpdatesPropertiesAfterAnother test skip on browser since it only, in effect, has one date attribute. Fix for #39132 and issues in #49555.

hamarb123 · 2021-03-18T18:54:44Z

Do I resolve the comments on files when I've fixed them in a commit?
The new code should be ready to review when the build is finished. It will probably fail my new test (4 variants, all named SettingUpdatesPropertiesOnSymlink) on unix platforms other than OSX, so just ignore those, I'll add the [ActiveIssue] attribute in the next, and hopefully final commit

danmoseley · 2021-03-18T19:20:20Z

Do I resolve the comments on files when I've fixed them in a commit?

We aren't really consistent. I typically see people resolve them before they pushed up the change, unless it's something where discussion might need to continue.

As you say, each push resets CI, which isn't zero cost, so we usually batch pushes some. But it's not a big deal.

hamarb123 · 2021-03-19T00:10:23Z

@carlossanlop it looks like I misremembered the behaviour of Windows links.
(Current behaviour) It gets the time on the link file, but sets the time onto the target file (this seems inconsistent to me).
What should the expected behaviour be? I personally feel like it should be both get and set on the link itself (which is what I've implemented for Mac), which would make more sense with the eventual completion of https://github.com/dotnet/runtime/projects/41 to me, and also how I'd like to use it in my code.
In my code it looks like I just disabled setting time on links on Windows because it could crash if the target doesn't exist.
I didn't/haven't yet invested time into seeing how to set the time on links using low-level Windows APIs.
Also, it should be noted that my test would pass if the set and get both did either the target or the link itself.
How should we continue @carlossanlop, should I just add Windows to the ActiveIssue attribute as well as the other failing platforms, or do something else? I'm happy for discussion to be had about what the correct behaviour should be, but personally I need some way to set it on the link itself: whether it's using a different api or not doesn't bother me.
Explanation of my logic with links:
Functions that work with the target (windows afaik): Set times, Reads and writes (it makes sense that it would be this to me)
Functions that work with the link itself (windows afaik): Delete, Exists, Get times, GetAttributes
I'm not sure where other APIs are.
To me it would make sense for all the APIs to work with the link itself, except for reading/writing the content (since link files have no visible content and this is the point of links). With the link API proposed in #24271 one could simply get the target and set attributes (time, attributes, etc.) on the target if they so desired.
Obviously if we're setting it on a file in a symlink folder then it makes sense to do it on the real file.
Also of note: I currently emulate a large portion of the logic in #24271 in one way or another as I wanted to be able to manipulate links, but I couldn't find a different way to do it other than writing interop code myself for Windows, and on Mac I just called the ln executable.
Edit: also my test seems to be failing on Mac despite testing it locally, I must have done something wrong locally, I'll fix the test in the next commit.
Edit 2: I don't seem to be able to repro the issues with the test locally yet (specifically SettingUpdatesPropertiesOnSymlink), which is odd. I've confirmed that the dates are set on the link files by pausing it just before the test finishes, not sure what's happening here.

hamarb123 · 2021-03-19T06:16:44Z

@danmoseley do you know what's going on with the CI?
On the last commit, some of the tests failed (expected), but SettingUpdatesPropertiesOnSymlink at least shouldn't have failed for OSX.
Also runtime (Build OSX x64 Debug AllSubsets_Mono_LLVMAOT) seems to be stuck running.
Out of OSX Libraries Test Run, only some failed the relevant test, and some didn't;
Failed SettingUpdatesPropertiesOnSymlink:
Libraries Test Run checked coreclr OSX x64 Debug
Didn't fail SettingUpdatesPropertiesOnSymlink (may have failed other tests):
Libraries Test Run release coreclr OSX x64 Debug
Libraries Test Run release mono OSX x64 Debug

To me, it looks like they should all have the same result, and also all pass since the test passed locally.

danmoseley · 2021-03-19T15:27:37Z

It seems there are some infra issues with OSX, linked above. Beyond that, not sure. If you are seeing different results between the mac machines, it may be due to OS versions, possibly different filesystems, ...

hamarb123 · 2021-09-15T07:51:53Z

@jozkee, let me know if those replies make sense, or if I need to change anything else :)

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj

- Use tuple destruction to swap variables - Add SettingOneTimeMaintainsOtherDefaultTimes and AllDefaultTimesAreSame test as per #49555 (comment) - Change TFM(/s) as per #49555 (comment) - Avoid unnecessary call to `UnixTimeToDateTimeOffset` as per #49555 (comment) - Use InvalidOperationException as per #49555 (comment)

…Current)-Unix;$(NetCoreAppCurrent)-Browser</TargetFrameworks> Revert to using <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-Unix;$(NetCoreAppCurrent)-Browser</TargetFrameworks> since <TargetFramework>$(NetCoreAppCurrent)</TargetFramework> failed; I wouldn't be surprised if this failed too.

…eSame tests Remove SettingOneTimeMaintainsOtherDefaultTimes and AllDefaultTimesAreSame tests since they don't work on windows because it is apparently an invalid time. Additionally, the function SettingOneTimeMaintainsOtherDefaultTimes wasn't working and needed to be fixed.

hamarb123 · 2021-10-18T23:19:18Z

The test Azure Pipelines / runtime (Mono llvmfullaot Pri0 Runtime Tests Run Linux arm64 release) has been running for almost 3 hours. Not sure what's happening here, don't think it's related to this code, but maybe it should be cancelled so it doesn't waste CI time.

As per #49555 (comment), use GetLastErrorInfo to get the error when setattrlist indicates that there is one. Co-authored-by: David Cantú <dacantu@microsoft.com>

src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs

The comment is now clearer and grammatically correct :)

Describe how utimensat change more dates than would be desired

Update for consistency with the comment in FileStatus.Unix.cs

• Fix trailing spaces and incorrect capitalisation in FileStatus.SetTimes.OSX.cs • Add more info to the comment in the SettingUpdatesPropertiesAfterAnother test

Use the Error property

jozkee

Left some last comments, feel free to ignore them.
Otherwise; LGTM, thanks for working on this!

src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs

…Another test Move comments and add explicit types to SettingUpdatesPropertiesAfterAnother test

hamarb123 · 2021-10-20T02:56:03Z

Thanks @jozkee! I've added those changes.

jeffhandley · 2021-10-20T18:27:50Z

🎉 Hooray! @hamarb123 -- Thanks so much for this contribution and your determination!

…#52639) * Implement most of the fix for #38824 Reverts the changes in the seperate PR a617a01 to fix #38824. Does not re-enable the test because that relies on #49555, will add a seperate commit to whichever is merged last to enable the SettingUpdatesPropertiesOnSymlink test. * Most of the code for PR #52639 to fix #38824 • Deal with merge conflicts • Add FSOPT_NOFOLLOW for macOS and use it in setattrlist • Use AT_SYMLINK_NOFOLLOW with utimensat (note: there doesn't seem to be an equivalent for utimes) • Add SettingUpdatesPropertiesOnSymlink test to test if it actually sets it on the symlink itself (to the best that we can anyway ie. write + read the times) • Specify FILE_FLAG_OPEN_REPARSE_POINT for CreateFile in FileSystem.Windows.cs (is there any other CreateFile calls that need this?) * Remove additional FILE_FLAG_OPEN_REPARSE_POINT I added FILE_FLAG_OPEN_REPARSE_POINT before and it was then added in another PR, this removes the duplicate entry. * Add missing override keywords Add missing override keywords for abstract members in the tests. * Fix access modifiers Fix access modifiers for tests - were meant to be protected rather than public * Add more symlink tests, rearrange files • Move symlink creation api to better spot in non-base files • Add IsDirectory property for some of the new tests • Change abstract symlink api to CreateSymlink that accepts path and target • Create a CreateSymlinkToItem method that creates a symlink to an item that may be relative that uses the new/modified abstract CreateSymlink method • Add SettingUpdatesPropertiesCore to avoid code duplication • Add tests for the following variants: normal/symlink, target exists/doesn't exist, absolute/relative target • Exclude nonexistent symlink target tests on Unix for Directories since they are counted as files * Fix return type of CreateSymlink in File/GetSetTimes.cs * Remove browser from new symlink tests as it doesn't support creation of symlinks Remove browser from new symlink tests as it doesn't support creation of symlinks * Use lutimes, improve code readability, simplify tests • Use lutimes when it's available • Extract dwFlagsAndAttributes to a variable • Use same year for all tests • Checking to delete old symlink is unnecessary, so don't • Replace var with explicit type * Change year in test to 2014 to reduce diff * Rename symlink tests, use 1 core symlink times function, and check that target times don't change Rename symlink tests, use 1 core symlink times function, and check that target times don't change * Inline RunSymlinkTestPart 'function' Inline RunSymlinkTestPart 'function' so that the code can be reordered so the access time test can be valid. * Share CreateSymlinkToItem call in tests and update comment for clarity * Update symlink time tests • Make SettingUpdatesPropertiesOnSymlink a theory • Remove special case for Unix due to #52639 (comment) (will revert if fails) • Rename isRelative to targetIsRelative for clarity * Remove unnecessary Assert.All * Changes to SettingUpdatesPropertiesOnSymlink test • Rename item field to link field • Don't use if one-liner • Use all time functions since only using UTC isn't necessary • Remove the now-defunct IsDirectory property since we aren't checking it anymore * Remove unnecessary fsi.Refresh() • Remove unnecessary fsi.Refresh() since atime is only updated when reading a file * Updates to test and pal_time.c • Remove targetIsRelative cases • Multi-line if statement • Combine HAVE_LUTIMES and #else conditions to allow more code charing * Remove trailing space

dotnet-issue-labeler bot added the area-System.IO label Mar 12, 2021

hamarb123 mentioned this pull request Mar 13, 2021

SetCreationTime on Mac #39132

Closed

hamarb123 added 2 commits March 16, 2021 20:22

Fix setting creation date for OSX

10379ce

Fix for #39132

Fix setting creation date for OSX - fix for setattrlist

2ade343

This commit adds a fallback for setattrlist failing. It can sometimes fail on specific volume types, so this is a workaround to use the old behaviour on unsupported filesystems. Fix for #39132

runfoapp bot mentioned this pull request Mar 16, 2021

Mono System.Text.Json.Tests on Windows timing out #42677

Open

hamarb123 changed the title ~~[WIP] Fix setting creation date for OSX~~ Fix setting creation date for OSX Mar 17, 2021

carlossanlop assigned hamarb123 Mar 18, 2021

carlossanlop requested review from adamsitnik, jozkee and carlossanlop March 18, 2021 04:26

carlossanlop reviewed Mar 18, 2021

View reviewed changes

hamarb123 requested a review from carlossanlop March 18, 2021 07:56

danmoseley reviewed Mar 18, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/src/System/IO/FileStatus.Unix.cs Outdated Show resolved Hide resolved

hamarb123 requested a review from danmoseley March 18, 2021 19:05

This was referenced Mar 19, 2021

OSX deprovision jaredpar/runfo#41

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

hamarb123 marked this pull request as ready for review September 15, 2021 07:50

jozkee reviewed Sep 17, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.SetTimes.OSX.cs Outdated Show resolved Hide resolved

jozkee reviewed Sep 21, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs Outdated Show resolved Hide resolved

hamarb123 mentioned this pull request Sep 30, 2021

Manage .NET's platform dependencies dotnet/core#5651

Open

8 tasks

hamarb123 commented Oct 9, 2021

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStatus.Unix.cs Show resolved Hide resolved

src/libraries/System.IO.FileSystem/tests/System.IO.FileSystem.Tests.csproj Outdated Show resolved Hide resolved

hamarb123 added 5 commits October 9, 2021 19:15

Fix typos

1ac43cc

Merge remote-tracking branch 'upstream/main'

11b26d8

hamarb123 and others added 2 commits October 19, 2021 13:56

Fix: setattrlist can only return 0 or -1

d8ec4fb

As per #49555 (comment), use GetLastErrorInfo to get the error when setattrlist indicates that there is one. Co-authored-by: David Cantú <dacantu@microsoft.com>

Merge remote-tracking branch 'upstream/main'

6931bd0

jozkee reviewed Oct 19, 2021

View reviewed changes

hamarb123 added 5 commits October 19, 2021 14:10

Update code comment for SettingUpdatesPropertiesAfterAnother test

57a1229

The comment is now clearer and grammatically correct :)

Update code comment for SettingUpdatesPropertiesAfterAnother test

9c6ab2b

Describe how utimensat change more dates than would be desired

Update comment for SettingUpdatesPropertiesAfterAnother for consistency

1fdc325

Update for consistency with the comment in FileStatus.Unix.cs

Fixes for compilation and update to comment

6aa1852

• Fix trailing spaces and incorrect capitalisation in FileStatus.SetTimes.OSX.cs • Add more info to the comment in the SettingUpdatesPropertiesAfterAnother test

Update FileStatus.SetTimes.OSX.cs

31db20b

Use the Error property

runfoapp bot mentioned this pull request Oct 19, 2021

Error in CI Mono llvmfullaot Pri0 Runtime Tests Run Linux arm64 release #60234

Closed

jozkee approved these changes Oct 20, 2021

View reviewed changes

src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs Outdated Show resolved Hide resolved

Move comments and add explicit types to SettingUpdatesPropertiesAfter…

c7c11d2

…Another test Move comments and add explicit types to SettingUpdatesPropertiesAfterAnother test

jozkee merged commit 7cd8f19 into dotnet:main Oct 20, 2021

ghost locked as resolved and limited conversation to collaborators Nov 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix setting creation date for OSX-like platforms #49555

Fix setting creation date for OSX-like platforms #49555

hamarb123 commented Mar 12, 2021 •

edited

Loading

hamarb123 commented Mar 17, 2021 •

edited

Loading

hamarb123 commented Mar 17, 2021

carlossanlop left a comment •

edited

Loading

hamarb123 commented Mar 18, 2021

hamarb123 commented Mar 18, 2021 •

edited

Loading

hamarb123 commented Mar 18, 2021

carlossanlop commented Mar 18, 2021

hamarb123 commented Mar 18, 2021 •

edited

Loading

danmoseley commented Mar 18, 2021

hamarb123 commented Mar 19, 2021 •

edited

Loading

hamarb123 commented Mar 19, 2021 •

edited

Loading

danmoseley commented Mar 19, 2021

hamarb123 commented Sep 15, 2021

hamarb123 commented Oct 18, 2021

jozkee left a comment

hamarb123 commented Oct 20, 2021

jeffhandley commented Oct 20, 2021

Fix setting creation date for OSX-like platforms #49555

Fix setting creation date for OSX-like platforms #49555

Conversation

hamarb123 commented Mar 12, 2021 • edited Loading

hamarb123 commented Mar 17, 2021 • edited Loading

hamarb123 commented Mar 17, 2021

carlossanlop left a comment • edited Loading

Choose a reason for hiding this comment

hamarb123 commented Mar 18, 2021

hamarb123 commented Mar 18, 2021 • edited Loading

hamarb123 commented Mar 18, 2021

carlossanlop commented Mar 18, 2021

hamarb123 commented Mar 18, 2021 • edited Loading

danmoseley commented Mar 18, 2021

hamarb123 commented Mar 19, 2021 • edited Loading

hamarb123 commented Mar 19, 2021 • edited Loading

danmoseley commented Mar 19, 2021

hamarb123 commented Sep 15, 2021

hamarb123 commented Oct 18, 2021

jozkee left a comment

Choose a reason for hiding this comment

hamarb123 commented Oct 20, 2021

jeffhandley commented Oct 20, 2021

hamarb123 commented Mar 12, 2021 •

edited

Loading

hamarb123 commented Mar 17, 2021 •

edited

Loading

carlossanlop left a comment •

edited

Loading

hamarb123 commented Mar 18, 2021 •

edited

Loading

hamarb123 commented Mar 18, 2021 •

edited

Loading

hamarb123 commented Mar 19, 2021 •

edited

Loading

hamarb123 commented Mar 19, 2021 •

edited

Loading