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.

Sign up for GitHub

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

Jump to bottom

Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix #52639

Merged
merged 24 commits into from
Nov 15, 2021
Merged

Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix #52639

merged 24 commits into from
Nov 15, 2021

Conversation

hamarb123
Copy link
Contributor

@hamarb123 hamarb123 commented May 12, 2021 •
edited
Loading

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.

@hamarb123
Implement most of the fix for #38824
270c526 
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.
@ghost
Copy link

ghost commented May 12, 2021

Tagging subscribers to this area: @carlossanlop
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #38824.
Somewhat reliant on #49555.
Reverts the changes in the seperate PR a617a01 (on the #49555 PR) to fix #38824.
Does not yet re-enable the test because that relies on #49555, will add a seperate commit to whichever is merged last to enable the SettingUpdatesPropertiesOnSymlink test.

Author: hamarb123
Assignees: -
Labels:

area-System.IO
Milestone: -

carlossanlop
carlossanlop reviewed May 12, 2021
View reviewed changes
Copy link
Member

@carlossanlop carlossanlop left a comment

Choose a reason for hiding this comment

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

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.

@carlossanlop carlossanlop mentioned this pull request May 13, 2021
Closed
4 tasks
@jeffhandley
Copy link
Member

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!

@hamarb123
Copy link
Contributor Author

@jeffhandley, unit tests are in #49555, they just need to be enabled on Windows when #49555 is merged.

@KevinCathcart KevinCathcart mentioned this pull request May 28, 2021
Closed
@jeffhandley
Copy link
Member

Ah; I'm sorry I overlooked that @hamarb123!

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@jeffhandley
Copy link
Member

@jozkee This PR is assigned to you for follow-up/decision before the RC1 snap.

@jozkee jozkee added this to the 7.0.0 milestone Aug 4, 2021
@adamsitnik adamsitnik mentioned this pull request Aug 11, 2021
Closed
10 tasks
@tmds tmds mentioned this pull request Aug 24, 2021
Merged
@hamarb123 hamarb123 mentioned this pull request Aug 30, 2021
Closed
@hamarb123 hamarb123 changed the title Fix LastWriteTime and LastAccessTime of a symlink on Windows [WIP] Fix LastWriteTime and LastAccessTime of a symlink on Windows Sep 7, 2021
@hamarb123 hamarb123 marked this pull request as draft September 7, 2021 22:35
@hamarb123
Copy link
Contributor Author

I'm separating out the symlink code into this PR for Unix from #49555 as per #49555 (comment).

@hamarb123 hamarb123 changed the title [WIP] Fix LastWriteTime and LastAccessTime of a symlink on Windows [WIP] Fix LastWriteTime and LastAccessTime of a symlink on Windows and Unix Sep 7, 2021
@hamarb123
Copy link
Contributor Author

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 ghost closed this Oct 9, 2021
@ghost
Copy link

ghost commented Oct 9, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@danmoseley
Copy link
Member

I see this is blocked on #49555. Perhaps we/you can reopen this PR when this is unblocked.

@runfoapp runfoapp bot mentioned this pull request Oct 29, 2021
Closed
@hamarb123
Changes to SettingUpdatesPropertiesOnSymlink test
d50be1b 
• 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
@hamarb123
Merge remote-tracking branch 'upstream/main' into fixfor38824
d61060b
@hamarb123
Remove unnecessary fsi.Refresh()
a894d87 
• Remove unnecessary fsi.Refresh() since atime is only updated when reading a file
tmds
tmds approved these changes Nov 1, 2021
View reviewed changes
Copy link
Member

@tmds tmds left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@runfoapp runfoapp bot mentioned this pull request Nov 1, 2021
Closed
@hamarb123 hamarb123 requested a review from jozkee November 3, 2021 21:53
@hamarb123
Updates to test and pal_time.c
d8bff21 
• Remove targetIsRelative cases
• Multi-line if statement
• Combine HAVE_LUTIMES and #else conditions to allow more code charing
@hamarb123
Merge remote-tracking branch 'upstream/main' into fixfor38824
3b37666
@hamarb123
Remove trailing space
569a24f
@jozkee jozkee merged commit f9d05bd into dotnet:main Nov 15, 2021
This was referenced Nov 29, 2021
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 16, 2021
@jozkee
Copy link
Member

jozkee commented Sep 29, 2022

Breaking change doc issue: dotnet/docs#31483

@jozkee jozkee removed the needs-breaking-change-doc-created Breaking changes need an issue opened with https://github.com/dotnet/docs/issues/new?template=dotnet label Sep 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@carlossanlop carlossanlop carlossanlop left review comments

@jkotas jkotas jkotas left review comments

@tmds tmds tmds approved these changes

@jozkee jozkee jozkee approved these changes

@adamsitnik adamsitnik Awaiting requested review from adamsitnik

Assignees

@jozkee jozkee

@hamarb123 hamarb123

Labels
area-System.IO breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.

Cannot change LastWriteTime or LastAccessTime of a symlink
8 participants
@hamarb123 @jeffhandley @danmoseley @jozkee @tmds @carlossanlop @jkotas @terrajobst