Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1051,7 +1051,7 @@ private SortedSet<string> WriteFiles(ZipArchive package, SortedSet<string> files
var warningMessage = new StringBuilder();

// Add files that might not come from expanding files on disk
foreach (IPackageFile file in new HashSet<IPackageFile>(Files))
foreach (IPackageFile file in new SortedSet<IPackageFile>(Files, Comparer<IPackageFile>.Create((a, b) => String.CompareOrdinal(a.Path, b.Path))))
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Anyone know if this provides the same sorting order across platforms? The unit tests have many comments like this:

// Linux sorts the first two in different order than Window

And I would love to make sure this ordering is the same no matter the platform.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's a culture/locale issue, not an OS issue. I found 3 instances of the comment, and all 3 were doing stringList.OrderBy(s => s), so it didn't specify a string comparison method, and therefore defaults to culture specific sort.

Using this C# program:

string[] values = [ "[Content_Types].xml", "_rel/.rels" ];

List<string> contentTypesFirst = new List<string>();
List<string> relsFirst = new List<string>();

foreach (var culture in System.Globalization.CultureInfo.GetCultures(System.Globalization.CultureTypes.SpecificCultures))
{
    System.Threading.Thread.CurrentThread.CurrentCulture = culture;
    System.Threading.Thread.CurrentThread.CurrentUICulture = culture;
    if (values.OrderBy(v => v).First() == "[Content_Types].xml")
    {
        contentTypesFirst.Add(culture.Name);
    }
    else
    {
        relsFirst.Add(culture.Name);
    }
}

Console.WriteLine("Cultures where [Content_Types].xml comes first: " + string.Join(", ", contentTypesFirst));
Console.WriteLine("Cultures where _rels/.rels comes first: " + string.Join(", ", relsFirst));

Running on both Windows and Ubuntu (on WSL2), there's only 2 CultureInfo's that sort content types first: th-TH and en-US-POSIX

My guess is that the people at the time who wrote that comment probably had a linux machine using the C/POSIX locale and didn't know better.

I didn't test this PR's change, but since it's explicitly using String.CompareOrdinal, I expect it to work. I think new SortedSet<IPackageFile>(Files, StringComparer.Ordinal) is a shorter way to write the equivalent outcome.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for digging into this. C/POSIX makes a lot of sense.

I can't get new SortedSet<IPackageFile>(Files, StringComparer.Ordinal) to work. I think that tries to compare the the IPackageFile objects directly instead of using the nested Path (a string) property:

error CS1503: Argument 2: cannot convert from 'System.StringComparer' to 'System.Collections.Generic.IComparer<IPackageFile>?'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oops, I missed that it was comparing a property, rather than the object itself. Yes, the anonymous function is needed.

{
using (Stream stream = file.GetStream())
{
Expand Down
45 changes: 19 additions & 26 deletions test/NuGet.Core.Tests/NuGet.Packaging.Test/PackageBuilderTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ public void CreatePackageWithEmptyFoldersForV3Folders()
var files = archive.Entries
.Where(file => file.Name == "_._")
.Select(file => file.FullName)
.OrderBy(s => s)
.ToArray();

// Assert
Expand Down Expand Up @@ -148,7 +147,6 @@ public void CreatePackageWithDifferentFileKinds()
var files = archive.Entries
.Where(file => file.Name.StartsWith("foo"))
.Select(file => file.FullName)
.OrderBy(s => s)
.ToArray();

// Assert
Expand Down Expand Up @@ -2788,21 +2786,20 @@ public void PackageBuilderWorksWithFileNamesContainingSpecialCharacters()
using (var archive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: true))
{
// Get raw filenames without un-escaping.
var files = archive.Entries.Select(e => e.FullName).OrderBy(s => s).ToArray();
var files = archive.Entries.Select(e => e.FullName).ToArray();

// Linux sorts the first two in different order than Windows
Assert.Contains<string>(@"[Content_Types].xml", files);
Assert.Contains<string>(@"_rels/.rels", files);
Assert.Equal(@"_rels/.rels", files[0]);
Assert.Equal(@"test.nuspec", files[1]);
Assert.Equal(@"content/images/bread&butter.jpg", files[2]);
Assert.Equal(@"content/images/logo123?#78.png", files[3]);
Assert.Equal(@"lib/C#/test.dll", files[4]);
Assert.Equal(@"lib/name with spaces.dll", files[5]);
Assert.Equal(@"lib/regular.file.dll", files[6]);

Assert.StartsWith(@"package/services/metadata/core-properties/", files[7]);
Assert.EndsWith(@".psmdcp", files[7]);
Assert.Equal(@"[Content_Types].xml", files[7]);
Assert.StartsWith(@"package/services/metadata/core-properties/", files[8]);
Assert.EndsWith(@".psmdcp", files[8]);

Assert.Equal(@"test.nuspec", files[8]);
}
}
}
Expand Down Expand Up @@ -2830,17 +2827,14 @@ public void PackageBuilderWorksWithFileNameWithoutAnExtension()

using (var archive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: true))
{
var files = archive.GetFiles().OrderBy(s => s).ToArray();
var files = archive.GetFiles().ToArray();

// Linux sorts the first two in different order than Windows
Assert.Contains<string>(@"[Content_Types].xml", files);
Assert.Contains<string>(@"_rels/.rels", files);
Assert.Equal(@"_rels/.rels", files[0]);
Assert.Equal(@"test.nuspec", files[1]);
Assert.Equal(@"myfile", files[2]);

Assert.StartsWith(@"package/services/metadata/core-properties/", files[3]);
Assert.EndsWith(@".psmdcp", files[3]);

Assert.Equal(@"test.nuspec", files[4]);
Assert.Equal(@"[Content_Types].xml", files[3]);
Assert.StartsWith(@"package/services/metadata/core-properties/", files[4]);
Assert.EndsWith(@".psmdcp", files[4]);

using (var contentTypesReader = new StreamReader(archive.Entries.Single(file => file.FullName == @"[Content_Types].xml").Open()))
{
Expand Down Expand Up @@ -3040,14 +3034,13 @@ public void PackageBuilderWorksWithFilesHavingCurrentDirectoryAsTarget(string in

using (var archive = new ZipArchive(stream, ZipArchiveMode.Read, leaveOpen: true))
{
var files = archive.GetFiles().OrderBy(s => s).ToArray();

// Linux sorts the first two in different order than Windows
Assert.Contains<string>(@"[Content_Types].xml", files);
Assert.Contains<string>(@"_rels/.rels", files);
Assert.StartsWith(@"package/services/metadata/core-properties/", files[2]);
Assert.Equal(@"test.nuspec", files[3]);
Assert.Equal(outputFile, files[4]);
var files = archive.GetFiles().ToArray();

Assert.Equal(@"_rels/.rels", files[0]);
Assert.Equal(@"test.nuspec", files[1]);
Assert.Equal(outputFile, files[2]);
Assert.Equal(@"[Content_Types].xml", files[3]);
Assert.StartsWith(@"package/services/metadata/core-properties/", files[4]);
}
}
}
Expand Down
Loading