Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix #52639
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tagging subscribers to this area: @carlossanlop Issue DetailsFixes #38824.
|
There was a problem hiding this comment.
The change looks good so far, @hamarb123, thanks for the fix.
As mentioned in your description, unit tests should be included, so please ping me when you add them.
4 tasks
|
Hi @hamarb123 -- it looks like the we were waiting for some unit tests to be added. Let us know if you need any help or guidance on that. Thanks! |
|
@jeffhandley, unit tests are in #49555, they just need to be enabled on Windows when #49555 is merged. |
|
Ah; I'm sorry I overlooked that @hamarb123! |
terrajobst
added
the
community-contribution
|
@jozkee This PR is assigned to you for follow-up/decision before the RC1 snap. |
10 tasks
hamarb123
changed the title
|
I'm separating out the symlink code into this PR for Unix from #49555 as per #49555 (comment). |
hamarb123
changed the title
|
I'm not sure how to make this PR include the changes of the other PR without having it as seperate commits that need to also be merged, so for now I'll just remove the relevant code from #49555 and add it here once it's merged. If it is possible, please let me know how I can do it because this PR needs to change files added in #49555. |
ghost
closed this
|
Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it. |
|
I see this is blocked on #49555. Perhaps we/you can reopen this PR when this is unblocked. |
|
Yes, that's fine. |
jozkee
added a commit
that referenced
this pull request
Oct 20, 2021
* Fix setting creation date for OSX Fix for #39132 * Fix setting creation date for OSX - fix for setattrlist 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 * Fix setting creation date for OSX - fix for caching when set time & possible 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 * Fix setting creation date for OSX - changes for PR 1 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. * Fix setting creation date for OSX - changes for PR 2 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. * Fix setting creation date for OSX - changes for PR 3 Revert change in last commit: Make access time be set using setattrlist on OSX to fix following symlinks issue that would've failed the new test. It is now using the other API as otherwise some file watcher tests fail. Set AT_SYMLINK_NOFOLLOW in pal_time.c to not follow symlinks for access time on OSX, or any times on other unix systems. * Fix setting creation date for OSX - fix test for Windows and browser + update comments Fix the SettingUpdatesPropertiesOnSymlink test for Windows by setting FILE_FLAG_OPEN_REPARSE_POINT in the CreateFile used for setting times. Disable the SettingUpdatesPropertiesOnSymlink test for Browser as it can't use symlinks with the current API in the test project. Update comments. Add FILE_FLAG_OPEN_REPARSE_POINT. Fix for #39132 and issues in #49555. * Fix setting creation date for OSX - make modification time use normal unix APIs Only implement creation time setting with setattrlist. Here is why: eg. the old code doesn't account for setting creation time to 2002, then setting modification time to 2001, and then setting access time (since access time didn't have the creation date check). I've split up SetAccessOrWriteTime into 3 parts - SetAccessOrWriteTime, SetAccessOrWriteTime_StandardUnixImpl, and SetAccessOrWriteTimeImpl (this last one contains the logic of the method without the things like refreshing the times so it's not called twice on OSX). In this process I replaced the _fileStatusInitialized = -1 calls with Invalidate() calls. And to make sure that if the creation time is needed to be set by SetAccessOrWriteTime, then we don't get in an infinite loop in case setattrlist fails, so we call SetAccessOrWriteTime_StandardUnixImpl. * Fix setting creation date for OSX - hotfix: Remove ATTR_CMN_MODTIME Adding this commit now to avoid 2x CI. It was meant to be in the last one, but I forgot to save the file. Fix for #39132 and issues in #49555. * Fix setting creation date for OSX - revert windows changes & simplify code Revert the changes for Windows & disable the SettingUpdatesPropertiesOnSymlink test on Windows. Remove unnecessary setting to 0 on the attrList variable. Remove the unnecessary MarshalAs attributes in the AttrList type. Fix for #39132 and issues in #49555. * Fix setting creation date for OSX - hotfix: change to CULong for marshalling with setattrlist Use CULong as per #49555 (comment) Fix for #39132 and issues in #49555. * Fix setting creation date for OSX - hotfix: change to CULong for marshalling with setattrlist 2 Use CULong as per #49555 (comment) Forgot to save the other file's changes Fix for #39132 and issues in #49555. * Fix setting creation date for OSX - consolidate unix nanosecond calculation Consolidate unix nanosecond calculation into UnixTimeSecondsToNanoseconds as per #49555 (comment). Fix for #39132 and issues in #49555. * Use InvalidateCaches instead of Invalidate and EnsureCachesInitialized instead of EnsureStatInitialized * Fixes for reviews in PR #49555 Add an identifying field to the TimeFunction tuple and a function to get a TimeFunction from the name. This function is then used so that SettingUpdatesPropertiesAfterAnother can be written as a Theory, and is a lot more readable. Fix some comments that were incorrect. * Hotfix for missing parameter * Fix naming * Revert changes re SettingUpdatesPropertiesAfterAnother (mainly) and revert using a Theory Reversion as per #49555 (comment) This change can be unreverted if that's what's decided. * Fix errors of #49555 due to missing variables * Remove the parameters that were meant to be removed * Finish merge * Remove duplicate file * Add custom error handling for setattrlist Add custom error handling for setattrlist as per #49555 (comment). ENOTDIR means it is not a directory, so directory not found seems logical to me, I think the other errors can be dealt with by an IOException, happy to change this. * Remove symlink specific code from this PR Removes the symlink specific code as per #49555 (comment). This will be reverted (and modified) as per #49555 (comment) into #52639. * Remove custom ENOTDIR handling, enable tests for other OSX-like platforms + move items in project file • Removed ENOTDIR handling as per #49555 (comment) • Enabled tests for other OSX-like platforms since the code was included on them (IsOSXLike is presumably defined at src/libraries/Common/tests/TestUtilities/System/PlatformDetection.Unix.cs). If it fails on any of these platforms, they'll be fully explicitly excluded everywhere for this PR or fixed. • Move item in project file (System.Private.CoreLib.Shared.projitems) as per #49555 (comment) * Rename files according to review Rename files according to #49555 (comment) (and nearby comments). * Change names of functions to improve readability and refactor code This commit changes and refactors the core functions of this PR to have clearer names and to create a more logical flow of the code. It also updates some comments and fixes some oversights. Changes are based on the discussion #49555 (comment) (read whole conversation). * Fix the wrongly named variable and add the missing 'unsafe' keyword Fix the wrongly named variable and add the missing 'unsafe' keyword * Better comments and minor improvements Changes according to #49555 (review) • Use explicit type `IEnumerable<TimeFunction>` instead of var • Use PlatformDetection rather than OperatingSystem • Move the InvalidateCaches code for creation time setting to avoid unnecessary checks • Update explanatory comments to be more useful * Add additional tests and improvements - 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) * Fix typos * Revert to <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-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. * Remove SettingOneTimeMaintainsOtherDefaultTimes and AllDefaultTimesAreSame 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. * Fix: setattrlist can only return 0 or -1 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> * Update code comment for SettingUpdatesPropertiesAfterAnother test The comment is now clearer and grammatically correct :) * Update code comment for SettingUpdatesPropertiesAfterAnother test Describe how utimensat change more dates than would be desired * Update comment for SettingUpdatesPropertiesAfterAnother for consistency Update for consistency with the comment in FileStatus.Unix.cs * Fixes for compilation and update to comment • 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 Use the Error property * Move comments and add explicit types to SettingUpdatesPropertiesAfterAnother test Move comments and add explicit types to SettingUpdatesPropertiesAfterAnother test Co-authored-by: David Cantú <dacantu@microsoft.com>
tmds
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Outdated
Show resolved
Hide resolved
tmds
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Outdated
Show resolved
Hide resolved
tmds
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Outdated
Show resolved
Hide resolved
tmds
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Directory/GetSetTimes.cs
Outdated
Show resolved
Hide resolved
tmds
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Outdated
Show resolved
Hide resolved
jozkee
reviewed
Oct 29, 2021
src/libraries/System.IO.FileSystem/tests/Base/BaseGetSetTimes.cs
Outdated
Show resolved
Hide resolved
• 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() since atime is only updated when reading a file
jozkee
reviewed
Nov 5, 2021
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
• Remove targetIsRelative cases • Multi-line if statement • Combine HAVE_LUTIMES and #else conditions to allow more code charing
jozkee
approved these changes
Nov 6, 2021
This was referenced Nov 29, 2021
ghost
locked as resolved and limited conversation to collaborators
|
Breaking change doc issue: dotnet/docs#31483 |
jozkee
removed
the
needs-breaking-change-doc-created
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #38824.
Fixes setting the LastWriteTime and the LastAccessTime on a symlink on Windows and Unix.
Somewhat reliant on #49555.
Reverts the Windows changes of the commit a617a01 (on the #49555 PR) to fix #38824 and for Unix: hamarb123@f842935.