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

NuGet pack "The DateTimeOffset specified cannot be converted into a Zip file timestamp" #3793

Merged
merged 25 commits into from Feb 3, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
1e0437f
Fix NuGet pack "The DateTimeOffset specified cannot be converted into…
erdembayar Dec 10, 2020
57bb207
Add unit test for Zip file date before 1980 case
erdembayar Dec 10, 2020
e45b889
Address PR review comment by Damon and Nikolche
erdembayar Dec 11, 2020
d7bdb3c
Log that zip file modified dates are set to 1/4/1980 if it's before y…
erdembayar Dec 14, 2020
18e3bf0
Fix failing unit test
erdembayar Dec 14, 2020
3f11fc4
Make modified datetime passed to ZipArchive UTC so any file modified …
erdembayar Dec 16, 2020
5c9d833
Fix typos.
erdembayar Dec 16, 2020
ede2569
Added Zip file 2 second precision limitation comment.
erdembayar Dec 16, 2020
d89fcfb
Merge branch 'dev' into dev-eryondon-CannotBeConverted2ZipfileTimestamp
erdembayar Jan 12, 2021
b212126
Address code review comments by Andy for deterministic packing.
erdembayar Jan 13, 2021
146d4e5
Change to local Datetime for timestamping.
erdembayar Jan 15, 2021
7442f7b
Clean up
erdembayar Jan 29, 2021
dea0eef
Revert accidental changes.
erdembayar Jan 29, 2021
bd1c76a
Cleanup comments.
erdembayar Jan 29, 2021
d9e5479
Address Nikolche's code review.
erdembayar Jan 30, 2021
f24aff3
Address PR comment by Andy.
erdembayar Feb 1, 2021
af38361
Merge branch 'dev' into dev-eryondon-CannotBeConverted2ZipfileTimestamp
erdembayar Feb 1, 2021
69161ae
Address comment about string.Fomatting.
erdembayar Feb 1, 2021
a7f53aa
Fix typo.
erdembayar Feb 1, 2021
c5c3ebf
Fix typo.
erdembayar Feb 1, 2021
49cc7eb
Address more comments for string resouces.
erdembayar Feb 1, 2021
5d812dc
Address additional review comments.
erdembayar Feb 2, 2021
b7737cb
Address string.Format with string.Concat(+) for better performance.
erdembayar Feb 2, 2021
fa51fa0
Address comment by Nikolche.
erdembayar Feb 3, 2021
9bad83e
Check if warningMessage has something before trimming new line.
erdembayar Feb 3, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -228,7 +228,7 @@ public Packaging.PackageBuilder CreateBuilder(string basePath, NuGetVersion vers
Path.GetFullPath(Path.GetDirectoryName(TargetPath))), LogLevel.Minimal));
}

builder = new Packaging.PackageBuilder();
builder = new Packaging.PackageBuilder(Logger);

try
{
Expand Down
Expand Up @@ -720,15 +720,17 @@ private PackageBuilder CreatePackageBuilderFromNuspec(string path)
path,
_packArgs.GetPropertyValue,
!_packArgs.ExcludeEmptyDirectories,
_packArgs.Deterministic);
_packArgs.Deterministic,
_packArgs.Logger);
}

return new PackageBuilder(
path,
_packArgs.BasePath,
_packArgs.GetPropertyValue,
!_packArgs.ExcludeEmptyDirectories,
_packArgs.Deterministic);
_packArgs.Deterministic,
_packArgs.Logger);
}

private bool BuildFromProjectFile(string path)
Expand Down
Expand Up @@ -10,15 +10,13 @@
using System.IO.Compression;
using System.Linq;
using System.Reflection;
using System.Reflection.Emit;
using System.Xml.Linq;
using NuGet.Client;
using NuGet.Common;
using NuGet.ContentModel;
using NuGet.Frameworks;
using NuGet.Packaging.Core;
using NuGet.Packaging.PackageCreation.Resources;
using NuGet.Packaging.Rules;
using NuGet.RuntimeModel;
using NuGet.Versioning;

Expand All @@ -31,6 +29,7 @@ public class PackageBuilder : IPackageMetadata
private readonly bool _includeEmptyDirectories;
private readonly bool _deterministic;
private static readonly DateTime ZipFormatMinDate = new DateTime(1980, 1, 1, 0, 0, 0, DateTimeKind.Utc);
private readonly ILogger _logger = NullLogger.Instance;

/// <summary>
/// Maximum Icon file size: 1 megabyte
Expand All @@ -47,11 +46,23 @@ public PackageBuilder(string path, Func<string, string> propertyProvider, bool i
{
}

public PackageBuilder(string path, Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, ILogger logger)
: this(path, Path.GetDirectoryName(path), propertyProvider, includeEmptyDirectories, deterministic)
{
_logger = logger;
zivkan marked this conversation as resolved.
Show resolved Hide resolved
}

public PackageBuilder(string path, string basePath, Func<string, string> propertyProvider, bool includeEmptyDirectories)
: this(path, basePath, propertyProvider, includeEmptyDirectories, deterministic: false)
{
}

public PackageBuilder(string path, string basePath, Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, ILogger logger)
: this(path, basePath, propertyProvider, includeEmptyDirectories, deterministic)
{
_logger = logger;
}

public PackageBuilder(string path, string basePath, Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic)
: this(includeEmptyDirectories, deterministic)
{
Expand Down Expand Up @@ -90,6 +101,12 @@ public PackageBuilder()
{
}

public PackageBuilder(ILogger logger)
: this(includeEmptyDirectories: false, deterministic: false)
{
_logger = logger;
}

private PackageBuilder(bool includeEmptyDirectories, bool deterministic)
{
_includeEmptyDirectories = includeEmptyDirectories;
Expand Down Expand Up @@ -893,7 +910,24 @@ private ZipArchiveEntry CreateEntry(ZipArchive package, string entryName, Compre
private ZipArchiveEntry CreatePackageFileEntry(ZipArchive package, string entryName, DateTimeOffset timeOffset, CompressionLevel compressionLevel)
{
var entry = package.CreateEntry(entryName, compressionLevel);
entry.LastWriteTime = timeOffset;

// Here added 3 days, just in case to avoid any possible TimeZone issue, otherwise we hit this when repackaging the files.
if (timeOffset < ZipFormatMinDate.AddDays(3))
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
{
_logger.Log(
PackagingLogMessage.CreateMessage(
string.Format(
CultureInfo.CurrentCulture,
Strings.ZipFileTimeStampeModified, entryName, timeOffset.DateTime.ToShortDateString(), ZipFormatMinDate.AddDays(3).ToShortDateString()),
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
LogLevel.Information));

entry.LastWriteTime = ZipFormatMinDate.AddDays(3);
}
else
{
entry.LastWriteTime = timeOffset;
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

In my previous round of reviews, I asked to make sure timeOffset was in the UTC timezone before setting entry.LastWriteTime, but I don't see that it's checked or converted anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in case I added .UtcDateTime. But for actual physical files it's already in UTC timezone and it comes from below call stack.

  1. This method calls our above method and passing lastWriteTime.

    var entry = CreatePackageFileEntry(package, entryName, lastWriteTime, CompressionLevel.Optimal, warningMessage);

  2. And this one calls above 1 and passing lastWriteTime.

    CreatePart(
    package,
    file.Path,
    stream,
    lastWriteTime: _deterministic ? ZipFormatMinDate : file.LastWriteTime,

  3. In non-deterministic mode lastWriteTime is file.LastWriteTime which actually reads UTC datetime here:

else if (SourcePath != null)
{
_lastWriteTime = File.GetLastWriteTimeUtc(SourcePath);
return File.OpenRead(SourcePath);

Copy link
Member

Choose a reason for hiding this comment

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

But for actual physical files it's already in UTC timezone and it comes from below call stack.

Yes, but NuGet.Packaging is a package that customers can reference in their own projects, and by implementing their own IPackageFile that does not return UTC time, they could encounter problems. You've fixed it by using .UtcDateTime, but my point is that since NuGet is not only a tool, but also a package/SDK, we need to consider all the different possible usage.

}

return entry;
}

Expand Down
@@ -1,5 +1,8 @@
NuGet.Packaging.NupkgMetadataFile.Source.get -> string
NuGet.Packaging.NupkgMetadataFile.Source.set -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, string basePath, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInDependencyGroupsWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFilesWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFrameworkAssemblyGroupsWarning.get -> string
Expand Down
@@ -1,5 +1,8 @@
NuGet.Packaging.NupkgMetadataFile.Source.get -> string
NuGet.Packaging.NupkgMetadataFile.Source.set -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, string basePath, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInDependencyGroupsWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFilesWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFrameworkAssemblyGroupsWarning.get -> string
Expand Down
@@ -1,5 +1,8 @@
NuGet.Packaging.NupkgMetadataFile.Source.get -> string
NuGet.Packaging.NupkgMetadataFile.Source.set -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
NuGet.Packaging.PackageBuilder.PackageBuilder(string path, string basePath, System.Func<string, string> propertyProvider, bool includeEmptyDirectories, bool deterministic, NuGet.Common.ILogger logger) -> void
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInDependencyGroupsWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFilesWarning.get -> string
static NuGet.Packaging.Rules.AnalysisResources.InvalidUndottedFrameworkInFrameworkAssemblyGroupsWarning.get -> string
Expand Down
9 changes: 9 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/Strings.Designer.cs

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/Strings.resx
Expand Up @@ -859,4 +859,10 @@ Valid from:</comment>
<value>Some framework assembly reference TFMs are missing a platform version: {0}</value>
<comment>0 - comma-separated list of reference group TFMs</comment>
</data>
<data name="ZipFileTimeStampeModified" xml:space="preserve">
<value>{0} file lastwritetime(modified) timestamp changed from {1} to {2} due to zip file doesn't support timestamp before 01/01/1980 limitation.</value>
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
<comment>0 - Entry name
1 - original modified timestamp
2 - new modified timestamp</comment>
</data>
</root>
Expand Up @@ -11,8 +11,11 @@
using System.Xml;
using System.Xml.Linq;
using Moq;
using NuGet.Commands;
using NuGet.Common;
using NuGet.Frameworks;
using NuGet.Packaging.Core;
using NuGet.ProjectModel;
using NuGet.Test.Utility;
using NuGet.Versioning;
using Xunit;
Expand Down Expand Up @@ -2857,6 +2860,74 @@ public void PackageBuilderPreserveFileLastWriteTime()
}
}

[Fact]
public void PackageBuilderCorrectLastWriteTimeForZipfileBefore1980()
{
// https://github.com/NuGet/Home/issues/7001
zivkan marked this conversation as resolved.
Show resolved Hide resolved
// Act
DateTime ZipFormatMinDate = new DateTime(1980, 1, 1, 0, 0, 0, DateTimeKind.Utc);
DateTime Year2017Date = new DateTime(2017, 1, 15, 23, 59, 0, DateTimeKind.Utc);
var lastWriteTime = new DateTimeOffset(1979, 11, 15, 23, 59, 0, TimeSpan.Zero);
int numberOfDateCorrectedFiles = 0;
int numberOfDateNotCorrectedFiles = 0;
TestLogger innerLogger = new TestLogger();
ILogger logger = new PackCollectorLogger(innerLogger, new WarningProperties());

using (var directory = new TestLastWriteTimeDirectory(lastWriteTime))
{
var builder = new PackageBuilder(logger) { Id = "test", Version = NuGetVersion.Parse("1.0"), Description = "test" };
builder.Authors.Add("test");

// Create a file that is modified after 1980 and it shouldn't be modified.
string after1980File1 = Path.Combine(directory.Path, "After1980.txt");
string after1980File2 = Path.Combine(directory.Path, "After1980_2.txt");
File.WriteAllText(after1980File1, string.Empty);
File.WriteAllText(after1980File2, string.Empty);
File.SetLastWriteTime(after1980File1, ZipFormatMinDate.AddDays(3).AddHours(1));
File.SetLastWriteTime(after1980File2, Year2017Date);

builder.AddFiles(directory.Path, "**", "Content");

using (var stream = new MemoryStream())
{
builder.Save(stream);

// Assert
using (var archive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: true))
{
foreach (ZipArchiveEntry entry in archive.Entries)
{
string path = Path.Combine(directory.Path, entry.Name);
// Only checks the entries that originated from files in test directory
if (File.Exists(path))
{
if (path == after1980File1)
{
Assert.Equal(entry.LastWriteTime.DateTime, ZipFormatMinDate.AddDays(3).AddHours(1));
numberOfDateNotCorrectedFiles++;
}
else if (path == after1980File2)
{
Assert.Equal(entry.LastWriteTime.DateTime, Year2017Date);
numberOfDateNotCorrectedFiles++;
}
else
{
Assert.NotEqual(entry.LastWriteTime.DateTime, File.GetLastWriteTimeUtc(path));
Assert.Equal(entry.LastWriteTime.DateTime, ZipFormatMinDate.AddDays(3));
numberOfDateCorrectedFiles++;
}
}
}
}
}

Assert.True(numberOfDateNotCorrectedFiles == 2);
Assert.True(numberOfDateCorrectedFiles > 0);
Assert.Equal(innerLogger.LogMessages.Count(l => l.Message.Contains("file lastwritetime(modified) timestamp changed from")), innerLogger.LogMessages.Count());
}
}

private static IPackageFile CreatePackageFile(string name)
{
var file = new Mock<IPackageFile>();
Expand Down