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

Tar: set directory modification times while extracting. #88231

Merged
merged 10 commits into from
Jul 5, 2023

Conversation

tmds
Copy link
Member

@tmds tmds commented Jun 30, 2023

Fixes #74398.

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

ghost commented Jun 30, 2023

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

Issue Details

Fixes #74398.

Author: tmds
Assignees: -
Labels:

community-contribution, area-System.Formats.Tar

Milestone: -

@tmds
Copy link
Member Author

tmds commented Jun 30, 2023

The test I've included fails on Windows.
Something causes the last write time on the extracted directory to be the current time.

It can be a bug in the implementation, though I expect that would also happen on Linux.
It can also be something that is specific to Windows which causes the time to change.
Does someone have an idea on what it might be?

@danmoseley
Copy link
Member

Something causes the last write time on the extracted directory to be the current time

I didn't look carefully, but I don't see you setting the directory times (Directory.SetLastWriteTime(..)). Without that, the directories are going to have the same time of the last write to a file within them, I think. (Although I thought that was true on Linux, so not sure why it would be only on Windows)

tmds and others added 2 commits June 30, 2023 23:24
@danmoseley
Copy link
Member

danmoseley commented Jul 1, 2023

Looks like Directory.SetXx must be used to set a directory timestamp. File.SetXx will not work because Windows requires one extra flag:

dwFlagsAndAttributes |= Interop.Kernel32.FileOperations.FILE_FLAG_BACKUP_SEMANTICS;

While on Unix, it looks like either work, they end up calling utimensat the same way.

@tmds
Copy link
Member Author

tmds commented Jul 1, 2023

Looks like Directory.SetXx must be used to set a directory timestamp. File.SetXx will not work because Windows requires one extra flag:

Yes, that makes the difference. The tests are passing now on Windows too.

=> c == Path.DirectorySeparatorChar;
}

private static void AttemptDirectorySetLastWriteTime(string fullPath, DateTimeOffset lastWriteTime)
Copy link
Member Author

Choose a reason for hiding this comment

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

This is copied and changed for directories from:

private static void AttemptSetLastWriteTime(string destinationFileName, DateTimeOffset lastWriteTime)
{
try
{
File.SetLastWriteTime(destinationFileName, lastWriteTime.LocalDateTime); // SetLastWriteTime expects local time
}
catch
{
// Some OSes like Android might not support setting the last write time, the extraction should not fail because of that
}
}

@danmoseley
Copy link
Member

When we close this issue we probably should open an issue for our zip implementation as well as that's also mentioned in that issue

@@ -285,42 +285,44 @@ public Task ExtractToFileAsync(string destinationFileName, bool overwrite, Cance
internal abstract bool IsDataStreamSetterSupported();

// Extracts the current entry to a location relative to the specified directory.
internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool overwrite, SortedDictionary<string, UnixFileMode>? pendingModes)
internal void ExtractRelativeToDirectory(string destinationDirectoryPath, bool overwrite, SortedDictionary<string, UnixFileMode>? pendingModes, Stack<(string, DateTimeOffset)> directoryModificationTimes)
Copy link
Member

Choose a reason for hiding this comment

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

I might be reading this wrong but isn't directoryModificationTimes always empty here with the current implementation? If it's always empty, why even pass it in?

Copy link
Member Author

@tmds tmds Jul 5, 2023

Choose a reason for hiding this comment

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

It gets new-ed up by the caller of this method and the same instance is passed to all entries of the archive.
Items get added and removed by the call to TarHelpers.UpdatePendingModificationTimes in this method.

@ViktorHofer ViktorHofer merged commit 5e1608d into dotnet:main Jul 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Formats.Tar 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.

Directories created by extracting from a tar file do not have timestamps set
3 participants