Implement stream-based ZipFile ExtractToDirectory and CreateFromDirectory method overloads #85491

carlossanlop · 2023-04-27T22:02:52Z

dotnet-issue-labeler · 2023-04-27T22:02:59Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

ghost · 2023-04-27T22:03:08Z

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

Issue Details

Fixes #1555

Author:	carlossanlop
Assignees:	carlossanlop
Labels:	`area-System.IO.Compression`
Milestone:	8.0.0

ViktorHofer · 2023-04-28T05:35:24Z

src/libraries/System.IO.Compression.ZipFile/src/Resources/Strings.resx

+<root>
+  <!-- 
+    Microsoft ResX Schema 
+
+    Version 2.0
+
+    The primary goals of this format is to allow a simple XML format 
+    that is mostly human readable. The generation and parsing of the 
+    various data types are done through the TypeConverter classes 
+    associated with the data types.
+
+    Example:
+
+    ... ado.net/XML headers & schema ...
+    <resheader name="resmimetype">text/microsoft-resx</resheader>
+    <resheader name="version">2.0</resheader>
+    <resheader name="reader">System.Resources.ResXResourceReader, System.Windows.Forms, ...</resheader>
+    <resheader name="writer">System.Resources.ResXResourceWriter, System.Windows.Forms, ...</resheader>
+    <data name="Name1"><value>this is my long string</value><comment>this is a comment</comment></data>
+    <data name="Color1" type="System.Drawing.Color, System.Drawing">Blue</data>
+    <data name="Bitmap1" mimetype="application/x-microsoft.net.object.binary.base64">
+        <value>[base64 mime encoded serialized .NET Framework object]</value>
+    </data>
+    <data name="Icon1" type="System.Drawing.Icon, System.Drawing" mimetype="application/x-microsoft.net.object.bytearray.base64">
+        <value>[base64 mime encoded string representing a byte array form of the .NET Framework object]</value>
+        <comment>This is a comment</comment>
+    </data>
+
+    There are any number of "resheader" rows that contain simple 
+    name/value pairs.
+
+    Each data row contains a name, and value. The row also contains a 
+    type or mimetype. Type corresponds to a .NET class that support 
+    text/value conversion through the TypeConverter architecture. 
+    Classes that don't support this are serialized and stored with the 
+    mimetype set.
+
+    The mimetype is used for serialized objects, and tells the 
+    ResXResourceReader how to depersist the object. This is currently not 
+    extensible. For a given mimetype the value must be set accordingly:
+
+    Note - application/x-microsoft.net.object.binary.base64 is the format 
+    that the ResXResourceWriter will generate, however the reader can 
+    read any of the formats listed below.
+
+    mimetype: application/x-microsoft.net.object.binary.base64
+    value   : The object must be serialized with 
+            : System.Runtime.Serialization.Formatters.Binary.BinaryFormatter
+            : and then encoded with base64 encoding.
+
+    mimetype: application/x-microsoft.net.object.soap.base64
+    value   : The object must be serialized with 
+            : System.Runtime.Serialization.Formatters.Soap.SoapFormatter
+            : and then encoded with base64 encoding.
+
+    mimetype: application/x-microsoft.net.object.bytearray.base64
+    value   : The object must be serialized into a byte array 
+            : using a System.ComponentModel.TypeConverter
+            : and then encoded with base64 encoding.
+    -->


Suggested change

<root>



<root>

This was automatically added by Visual Studio. Shouldn't it be preserved?

I see a lot of resx files in our repo that preserve the automatically-added info:

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Memory/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Console/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Pipes/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Http/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Formats.Tar/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.Xml/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Packaging/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Speech/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Ports/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/Microsoft.CSharp/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Text.Json/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.IO.Hashing/src/Resources/Strings.resx

I see fewer examples of resx files that do not include that info:

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.CodeDom/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Net.Mail/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.Threading/src/Resources/Strings.resx

https://github.com/dotnet/runtime/blob/main/src/libraries/System.ObjectModel/src/Resources/Strings.resx

Right, we are inconsistent here but it shows up as added in your diff. For this PR, please undo the change. Ideally, we would reach out to the team that owns the VS resx tooling and ask them to add an option to remove these auto-added comments.

Ok, I can remove it. Do you know who the owner of VS resx is?

I say -- let's do this :)

If an option is created to control this, we (dotnet/project-system) can make sure it's exposed in the Properties pane in VS for SDK-style projects, likely setting metadata on the MSBuild item. The CSPROJ team would need to do something similar for legacy projects. It's possible that other project flavours would need to be changed to support this everywhere.

I've long wanted to improve the generated comment in the .Designer.cs file too, to make it clearer what the string actually is (such as with a <c> tag) but again was concerned about pushback from zillions of diffs for something that's really just a cosmetic quality-of-life thing, and doesn't impact running code. For the .Designer.cs file, I assume doing this in a source generator would be a better option, but that doesn't help with the .resx comment/XSD.

If an option is created to control this

Do we need an option? The idea in my mind is that it's removed from all templates and when editing via ResXResourceWriter or the designer (I don't recall whether that uses ResxResourceXXX or its own code) it is only written out if it previously existed. Would that work?

I agree that the Designer.cs should become a source generator eventually, but that is orthogonal other than touching many of the same components.

@drewnoakes I would like to open an issue in the right repo to continue the conversation, I'll copy what was mentioned here. I assume dotnet/project-system is the right place?

dotnet/project-system owns the Resource Editor UI, but not the code generation. If we don't want an option for this, then I don't think there's any involvement from any project system. Eric's comment is a good summary of the situation.

The Resource Editor in VS uses the .NET Framework classes to read and write the file. I don't think the current object model would track whether such a comment/XSD existed when read in order to know whether to include it on write. That could be changed, but it'd need to be changed in .NET Framework.

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.cs

carlossanlop · 2023-05-11T21:38:56Z

/azp run runtime

azure-pipelines · 2023-05-11T21:39:23Z

Azure Pipelines successfully started running 1 pipeline(s).

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Create.Stream.cs

stephentoub

Source changes LGTM. I only quickly skimmed the tests.

carlossanlop

Suggestions to address feedback.

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Create.Stream.cs

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Extract.Stream.cs

carlossanlop · 2023-05-23T19:31:02Z

Conflict resolved.
Edit: Wait, no. I did something weird.
Edit 2: I fixed the conflict causing the build failure, but I am seeing a weird test regression in wasm. I'm investigating.

danmoseley · 2023-05-24T02:16:13Z

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Extract.cs

+        public static void ExtractToDirectory(Stream source, string destinationDirectoryName, Encoding? entryNameEncoding, bool overwriteFiles)
+        {
+            ArgumentNullException.ThrowIfNull(source);
+            if (!source.CanRead)


it will throw anyway

runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

Line 122 in 3747edb

ArgumentNullException.ThrowIfNull(stream);

runtime/src/libraries/System.IO.Compression/src/System/IO/Compression/ZipArchive.cs

Line 140 in 3747edb

throw new ArgumentException(SR.ReadModeCapabilities);

lewing · 2023-05-25T18:05:12Z

It looks like the wasm failure is just that enumerating the files ends up in a different order after extraction?

carlossanlop · 2023-05-29T02:57:54Z

It looks like the wasm failure is just that enumerating the files ends up in a different order after extraction?

Yep, that seems to be the case. I'll fix it by ordering the results. I wasn't sure that was the root cause at first, the error wasn't clear about that. It's why I pushed f1f23c4 to be able to see the enumeration comparison values in the error:

[02:11:18] info: [2023-05-24T02:11:18.029Z] [FAIL] System.IO.Compression.Tests.ZipFile_Extract_Stream.ExtractToDirectoryRoundTrip
[02:11:18] info: Showing first 10 differences
[02:11:18] info:   Position 2: Expected: notempty, Actual: binary.wmv
[02:11:18] info:   Position 3: Expected: binary.wmv, Actual: notempty
[02:11:18] info: Total number of differences: 2 out of 5
[02:11:18] info:    at System.AssertExtensions.SequenceEqual[String](ReadOnlySpan`1 expected, ReadOnlySpan`1 actual)
[02:11:18] info:    at System.AssertExtensions.SequenceEqual[String](Span`1 expected, Span`1 actual)
[02:11:18] info:    at System.AssertExtensions.SequenceEqual[String](String[] expected, String[] actual)
[02:11:18] info:    at System.IO.Compression.Tests.ZipFileTestBase.DirFileNamesEqual(String actual, String expected)
[02:11:18] info:    at System.IO.Compression.Tests.ZipFile_Extract_Stream.ExtractToDirectoryRoundTrip()
[02:11:18] info:    at System.Reflection.MethodInvoker.InterpretedInvoke(Object obj, IntPtr* args)
[02:11:18] info:    at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)

…ToDirectory methods.

…rectory methods.

…File.ExtractToDirectory.

In dotnet#85491 the stream-based version of the `DoCreateFromDirectory` override copy-pasted comments from the string-based version ```csharp // Rely on Path.GetFullPath for validation of sourceDirectoryName and destinationArchive ``` The last clause of that comment is not relevant in the stream-based version (the argument is `Stream destination`) so deleted that clause. It's possible that the other comment is also misleading as we test the `compressionLevel ` argument just above (unlike in the `String destinationArchive` version ```csharp // Checking of compressionLevel is passed down to DeflateStream and the IDeflater implementation // as it is a pluggable component that completely encapsulates the meaning of compressionLevel. ```

* Remove comment in ZipFile.Create.cs In #85491 the stream-based version of the `DoCreateFromDirectory` override copy-pasted comments from the string-based version ```csharp // Rely on Path.GetFullPath for validation of sourceDirectoryName and destinationArchive ``` The last clause of that comment is not relevant in the stream-based version (the argument is `Stream destination`) so deleted that clause. It's possible that the other comment is also misleading as we test the `compressionLevel ` argument just above (unlike in the `String destinationArchive` version ```csharp // Checking of compressionLevel is passed down to DeflateStream and the IDeflater implementation // as it is a pluggable component that completely encapsulates the meaning of compressionLevel. ``` * Remove compressionLevel validation comment Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com> --------- Co-authored-by: Carlos Sánchez López <1175054+carlossanlop@users.noreply.github.com>

carlossanlop added the area-System.IO.Compression label Apr 27, 2023

carlossanlop added this to the 8.0.0 milestone Apr 27, 2023

carlossanlop requested review from stephentoub, ViktorHofer and ericstj April 27, 2023 22:02

carlossanlop self-assigned this Apr 27, 2023

dotnet-issue-labeler bot added the new-api-needs-documentation label Apr 27, 2023

ViktorHofer reviewed Apr 28, 2023

View reviewed changes

This was referenced Apr 28, 2023

Tracking issue for CI build timeouts #76454

Closed

[mono][wasm] System.Numerics.Vectors.Tests assert equal failure #84775

Closed

Various WASM timeouts on CI #85304

Closed

stephentoub reviewed Apr 28, 2023

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.cs Outdated Show resolved Hide resolved

src/libraries/System.IO.Compression.ZipFile/src/System/IO/Compression/ZipFile.Create.cs Outdated Show resolved Hide resolved

stephentoub reviewed May 16, 2023

View reviewed changes

src/libraries/System.IO.Compression.ZipFile/tests/ZipFile.Create.Stream.cs Show resolved Hide resolved

stephentoub approved these changes May 16, 2023

View reviewed changes

carlossanlop commented May 23, 2023

View reviewed changes

carlossanlop force-pushed the ZipFileStreamMethods branch from 95de910 to 322ca3f Compare May 23, 2023 19:30

danmoseley reviewed May 24, 2023

View reviewed changes

carlossanlop added 6 commits May 28, 2023 20:06

ref: Add stream-based ZipFile.CreateFromDirectory and ZipFile.Extract…

483f05e

…ToDirectory methods.

src: Add stream-based ZipFile.CreateFromDirectory ZipFile.ExtractToDi…

1ab248d

…rectory methods.

tests: Move wrongly placed tests to the correct class.

e2d2285

tests: Add stream-based tests for ZipFile.CreateFromDirectory and Zip…

67994d7

…File.ExtractToDirectory.

src: Documentation and resource strings.

fddb858

tests: More tests for unseekable/unreadable/unwritable streams.

33181a2

carlossanlop added 5 commits May 28, 2023 20:06

Address suggestions and include more exception validation tests.

bf4d933

Fix braces after resolving conflict.

6a546cb

Use AssertExtensions.SequenceEqual for clearer error message.

c02d3c5

Reset the resx file again.

660cf87

Order results of dir enumeration tests

42097ce

carlossanlop force-pushed the ZipFileStreamMethods branch from e402b38 to 42097ce Compare May 29, 2023 04:22

This was referenced May 30, 2023

Failed USB connection via port 54050, error 61, in tvOS arm64 Release AllSubsets_Mono #82637

Open

Consider removing all the resx schema unnecessary comments auto added by Visual Studio #86906

Open

carlossanlop merged commit cfa28e3 into dotnet:main May 30, 2023

carlossanlop deleted the ZipFileStreamMethods branch May 30, 2023 16:48

IDisposable mentioned this pull request May 30, 2023

Remove comment in ZipFile.Create.cs #86927

Merged

ghost locked as resolved and limited conversation to collaborators Jun 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement stream-based ZipFile ExtractToDirectory and CreateFromDirectory method overloads #85491

Implement stream-based ZipFile ExtractToDirectory and CreateFromDirectory method overloads #85491

carlossanlop commented Apr 27, 2023

dotnet-issue-labeler bot commented Apr 27, 2023

ghost commented Apr 27, 2023

ViktorHofer Apr 28, 2023

carlossanlop Apr 28, 2023

carlossanlop Apr 28, 2023

ViktorHofer May 19, 2023 •

edited

Loading

carlossanlop May 23, 2023

danmoseley May 23, 2023

drewnoakes May 24, 2023

danmoseley May 24, 2023

carlossanlop May 24, 2023

drewnoakes May 25, 2023 •

edited

Loading

carlossanlop commented May 11, 2023

azure-pipelines bot commented May 11, 2023

stephentoub left a comment

carlossanlop left a comment

carlossanlop commented May 23, 2023 •

edited

Loading

danmoseley May 24, 2023

lewing commented May 25, 2023

carlossanlop commented May 29, 2023

Implement stream-based ZipFile ExtractToDirectory and CreateFromDirectory method overloads #85491

Implement stream-based ZipFile ExtractToDirectory and CreateFromDirectory method overloads #85491

Conversation

carlossanlop commented Apr 27, 2023

dotnet-issue-labeler bot commented Apr 27, 2023

ghost commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drewnoakes May 25, 2023 • edited Loading

Choose a reason for hiding this comment

carlossanlop commented May 11, 2023

azure-pipelines bot commented May 11, 2023

stephentoub left a comment

Choose a reason for hiding this comment

carlossanlop left a comment

Choose a reason for hiding this comment

carlossanlop commented May 23, 2023 • edited Loading

Choose a reason for hiding this comment

lewing commented May 25, 2023

carlossanlop commented May 29, 2023

ViktorHofer May 19, 2023 •

edited

Loading

drewnoakes May 25, 2023 •

edited

Loading

carlossanlop commented May 23, 2023 •

edited

Loading