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

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

Merged
merged 25 commits into from Feb 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from 8 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
2 changes: 1 addition & 1 deletion src/NuGet.Clients/NuGet.CommandLine/Commands/ProjectFactory.cs
View file
Open in desktop
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
6 changes: 4 additions & 2 deletions src/NuGet.Core/NuGet.Commands/CommandRunners/PackCommandRunner.cs
View file
Open in desktop
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
65 changes: 62 additions & 3 deletions src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs
View file
Open in desktop
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,9 @@ 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 static readonly DateTime ZipFormatMaxDate = new DateTime(2107, 12, 31, 23, 59, 58, DateTimeKind.Utc);
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
private readonly ILogger _logger = NullLogger.Instance;
private bool _zipFormatCorrected = false;
zivkan marked this conversation as resolved.
Show resolved Hide resolved

/// <summary>
/// Maximum Icon file size: 1 megabyte
Expand All @@ -47,11 +48,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 +103,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 +912,47 @@ 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;

// Please note: ZipArchive stream reader sometime changes LastWriteTime by another 1 second off than what "entry.LastWriteTime" has.
erdembayar marked this conversation as resolved.
Show resolved Hide resolved
// The FAT filesystem of DOS has a timestamp resolution of only two seconds; ZIP file records mimic this.
// As a result, the built -in timestamp resolution of files in a ZIP archive is only two seconds, though extra fields can be used to store more precise timestamps.The ZIP format has no notion of time zone, so timestamps are only meaningful if it is known what time zone they were created in.
if (timeOffset.UtcDateTime < ZipFormatMinDate)
{
if (!_zipFormatCorrected)
{
_zipFormatCorrected = true;

_logger.Log(
PackagingLogMessage.CreateMessage(
string.Format(
CultureInfo.CurrentCulture,
Strings.ZipFileTimeStampModified, entryName, timeOffset.DateTime.ToShortDateString(), ZipFormatMinDate.ToShortDateString()),
LogLevel.Information));
}

entry.LastWriteTime = ZipFormatMinDate;
}
else if (timeOffset.UtcDateTime > ZipFormatMaxDate)
{
if (!_zipFormatCorrected)
Copy link
Member

Choose a reason for hiding this comment

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

Why log only once?
We should log for all files that were adjusted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of this comment to limit spamming.

Copy link
Member

Choose a reason for hiding this comment

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

The method WriteFiles iterates the files in the package and calls CreatePart for each. If you create a collection in this method, containing all the filenames with "out of range" dates in this method, then you can create a single message with all the affected files. As an example, see this pack validation rule

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @zivkan

Limit spamming means omitting repetitive information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I imitated your pattern but end result seems still same.

Currently dotnet.exe pack --no-build output looks like below:
image

       File 'net5.0-windows/10218.dll' was last modified on '1/29/1956', which is out of range of what the zip format supports. Using '1/1/1980' instead.
       File 'netcoreapp3.1/10218.dll' was last modified on '1/29/1957', which is out of range of what the zip format supports. Using '1/1/1980' instead.

Can you elaborate more on removing repetitive information? Is below looks correct approach? But this one has some complexities. Instead of is we need to use are, so we need need to have another string resource specific for purpose. Also how about someone mix 1956 and 2200 files then what message to show (not sure if it's possible scenario)?

        File 'net5.0-windows/10218.dll' was last modified on '1/29/1956', 'netcoreapp3.1/10218.dll' was last modified on '1/29/1957', which are out of range of what the zip format supports. Using '1/1/1980 instead'.

Copy link
Member

Choose a reason for hiding this comment

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

Other warnings had a block of text, followed by a list of shorter messages for each individual instance. So, you could think about changing the warning to something like:

The zip format supports a limited date range. The following files are outside the supported range:
'file1' changed from '1979-12-31' to '1980-01-01'
'file2' changed from '2108-01-01' to '2107-12-31'

This way we can avoid repeating the "out of range for what the zip format supports" text.

Also, if you were to do this in Visual Studio, you should see only a single entry in the error list, not one per file. I'm not sure if it reduces a list of warnings in a binlong.

Having said this, I was thinking about it over the weekend, and I'm feeling much less strongly about this. Consider a c# syntax error instead. If I have the same syntax error in multiple places in a single file, Roslyn is going to raise one warning per instance, not one warning per file or one warning per project. One difference is that in Visual Studio you can click the warning and go to the line of code, whereas NuGet doesn't. Even if NuGet had the capability to determine a filename and line number, for "go to error" in general, I'm not sure it makes sense for this specific warning. It probably makes sense here to have a single message with a list of affected files.

As mentioned in another comment, we should have an NU warning code, and a docs issue to document the NU code.

{
_zipFormatCorrected = true;

_logger.Log(
PackagingLogMessage.CreateMessage(
string.Format(
CultureInfo.CurrentCulture,
Strings.ZipFileTimeStampModified, entryName, timeOffset.DateTime.ToShortDateString(), ZipFormatMinDate.ToShortDateString()),
LogLevel.Information));
}

entry.LastWriteTime = ZipFormatMaxDate;
}
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.

    NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

    Line 1126 in 49cc7eb

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

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

    NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PackageBuilder.cs

    Lines 966 to 970 in 49cc7eb

    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:

NuGet.Client/src/NuGet.Core/NuGet.Packaging/PackageCreation/Authoring/PhysicalPackageFile.cs

Lines 96 to 99 in 74781ba

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
3 changes: 3 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/PublicAPI/net472/PublicAPI.Unshipped.txt
View file
Open in desktop
@@ -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
3 changes: 3 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/PublicAPI/netcoreapp5.0/PublicAPI.Unshipped.txt
View file
Open in desktop
@@ -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
3 changes: 3 additions & 0 deletions src/NuGet.Core/NuGet.Packaging/PublicAPI/netstandard2.0/PublicAPI.Unshipped.txt
View file
Open in desktop
@@ -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
View file
Open in desktop

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
View file
Open in desktop
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="ZipFileTimeStampModified" xml:space="preserve">
<value>Last write timestamp for '{0}' changed from '{1}' to '{2}' because the zip file format does not support timestamp values before 01/01/1980 or after 12/31/2107.</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>