Add ZipDirectory and Unzip tasks by jeffkl · Pull Request #3291 · dotnet/msbuild

jeffkl · 2018-05-09T20:43:40Z

ZipDirectory task zips up a whole directory. It will fail if the destination file already exists. Zipping individual files is a lot harder because you have to calculated a base path for all files and make zip entries relative to that. The code in the CLR that does this is very efficient by reusing string buffers and I didn't want to have to replicate the code. So for now there's only ZipDirectory instead of a more generic Zip.

Unzip unzips files from an archive to a directory.

Defaults to skip unzipping files that are already up-to-date
Logs every file that was unzipped
Supports cancellation
Supports overwriting read-only files
Supports unzipping multiple files to the same directory

Fixes #1781

AndyGerlicher · 2018-05-10T20:35:28Z

Do we always want this? Seems like it could be a giant list in some cases and use up a lot of memory and may or may not be needed? The output folder might be enough?

Its what <Copy /> does so I though I'd ... 😑 ... 😎 copy it.

Technically you could copy a ton of files as well

<ItemGroup> <Files Include="Folder\**" /> </ItemGroup> <Copy SourceFiles="@(Files)" DestinationFolder="Foo" />

And we'll waste all of the memory. What should I do?

@jeffkl Would it save memory to make _unzippedFiles and _destinationFiles a collection of tuples of sourceTaskItem and destinationPath.FullName? That way if you access the output properties DestinationFiles or UnzippedFiles you could build the full list of TaskItems on demand?

We decided to just remove it. If necessary, we can add the functionality later.

rainersigwald · 2018-05-14T15:38:44Z

I'm a bit worried about this interface. It's pretty much impossible to get correct incremental behavior out of this. You'd have to:

Enumerate the contents of the folder just before the target that invokes ZipDirectory.

Additionally compute a hash of those items to avoid the removed-an-input problem.

Use the new stuff in the target that calls this.

Accepting Item[] would be more complex in implementation but easier to make correct.

I discussed this with @AndyGerlicher and we're concerned more about the complexity required to make it incremental out-of-the-box. So we're going to leave it as-is.

I think if we wrote a target that used this we would need to make it incremental. But given that it zips a directory, it doesn't seem worth the complexity to implement at the task level. It might be worth writing a sample in the docs page that is a target that zips your output folder that would be incremental as an example.

rainersigwald · 2018-05-14T15:42:14Z

Does this not deserve an Overwrite param?

rainersigwald · 2018-05-14T15:42:50Z

Why not just create? Didn't we make something create instead of erroring in this situation fairly recently?

This is the directory to zip up, so it must exist

Yes, that . . . is definitely true 😬

rainersigwald · 2018-05-14T15:44:40Z

Do you think this is sufficiently long-running/IO intensive that we should Yield() here?

Yeah the zip file could contain a large file, I'll add it

rainersigwald · 2018-05-14T15:46:53Z

Is this used?

I was at one point, good catch

rainersigwald · 2018-05-14T15:49:39Z

Worth using the CopyToAsync overload that takes a CancellationToken to speed up canceling if we're stuck in a huge file? Possibly not . . .

rainersigwald · 2018-05-14T15:52:46Z

Make this a CancellationTokenSource instead and pass around a CancellationToken?

jeffkl · 2018-05-15T14:01:08Z

@Microsoft/msbuild-maintainers this is ready for final review

rainersigwald · 2018-05-15T14:37:06Z

Don't forget to manually port these to the internal VS repo after this merges.

rainersigwald · 2018-05-15T14:39:34Z

Use

[PlatformSpecific(TestPlatforms.Windows)]

instead.

cdmihai · 2018-05-16T22:15:59Z

Why not eagerly exit to reduce the time it takes to finish the build on error?

Ah, to show all errors at once rather than one at a time. The alternative is to complicate things and check everything first, and do the extraction second.

In reply to: 188788554 [](ancestors = 188788554)

cdmihai · 2018-05-16T22:18:56Z

Shouldn't Reacquire be in a finally block? Or does the engine handle this?

cdmihai · 2018-05-16T22:28:33Z

What happens if there's files in multiple archives with the same path? Should there be an option to warn or error about this?

I think its okay to just ignore it, do you want me to log something?

I guess people can use the diagnostic log to see the task inputs, so I guess they already have insight into the sources.

cdmihai · 2018-05-16T22:31:17Z

Should it be made readonly after the write to preserve user data? :)

I'm not sure. I'd assume that if the file is readonly in the archive it should be readonly after extraction but I don't see attributes like that in the ZipArchiveEntry class... It seems a little weird to be setting the readonly attribute based on what the file used to have. Thoughts?

cdmihai · 2018-05-16T22:32:36Z

nit: named constant for the buffer size.

cdmihai · 2018-05-16T22:37:36Z

The other task wraps each IO in a try catch. Should this one be wrapped too?

cdmihai · 2018-05-16T22:55:13Z

Shouldn't the returned task be awaited before the entire task returns?

cdmihai · 2018-05-16T22:58:20Z

Curious what happens if another file exists in the destination and is longer than the archive entry. Does this override or does it just copy over the source stream, leaving some trailing bytes from the destination? The CopyTo documentation is a bit unclear on this.

Oh, did not notice the FileMode.Create. Nevermind.

In reply to: 188796551 [](ancestors = 188796551)

Add unit tests for Unzip

rainersigwald · 2018-05-21T15:03:12Z

`Copy` doesn’t restore the RO bit, so I’d say don’t do it here either.

jeffkl · 2018-05-22T15:25:53Z

@cdmihai can you please review my changes to address your comments? I think this is ready to merge but wanted to make sure you were happy.

cdmihai · 2018-05-22T16:33:05Z

+        [Required]
+        public ITaskItem SourceDirectory { get; set; }
+
+        public override bool Execute()


Can you please make this yieldable as well?

natemcmaster · 2018-05-22T20:48:21Z

+        {
+            foreach (ZipArchiveEntry zipArchiveEntry in sourceArchive.Entries.TakeWhile(i => !_cancellationToken.IsCancellationRequested))
+            {
+                FileInfo destinationPath = new FileInfo(Path.Combine(destinationDirectory.FullName, zipArchiveEntry.FullName));


Sorry to comment after you've just merged, but would you consider normalizing directory separators? One of the most common errors I've encountered is that zips created on Windows using \ as a directory separator will not extract correctly on Linux/macOS. Instead \ becomes part of the filename.

This common error is something we worked around in the unzip task used in the .NET Core build system: see https://github.com/dotnet/arcade/blob/0d117c0b3649565a6d9aacf9f435e29ab67c3c0d/src/Microsoft.DotNet.Build.Tasks.IO/src/UnzipArchive.cs#L64-L76

jeffkl · 2018-05-22T20:51:06Z

@natemcmaster I'll double check but I think my unit tests cover that scenario.

Flithor · 2023-11-04T07:34:45Z

+                try
+                {
+                    Log.LogMessageFromResources(MessageImportance.High, "ZipDirectory.Comment", sourceDirectory.FullName, destinationFile.FullName);
+                    ZipFile.CreateFromDirectory(sourceDirectory.FullName, destinationFile.FullName);


I think need make the "includeBaseDirectory" argment as an option

AndyGerlicher reviewed May 10, 2018

View reviewed changes

jeffkl changed the title ~~WIP: Add ZipDirectory and Unzip tasks~~ Add ZipDirectory and Unzip tasks May 11, 2018

jeffkl force-pushed the zip branch 2 times, most recently from 8a91c23 to 401c24d Compare May 14, 2018 14:33

rainersigwald reviewed May 14, 2018

View reviewed changes

jeffkl force-pushed the zip branch 3 times, most recently from 2abd49c to d7e7f23 Compare May 14, 2018 16:07

rainersigwald reviewed May 14, 2018

View reviewed changes

jeffkl force-pushed the zip branch from d7e7f23 to 9990a49 Compare May 14, 2018 16:11

rainersigwald approved these changes May 15, 2018

View reviewed changes

cdmihai reviewed May 16, 2018

View reviewed changes

jeffkl added 10 commits May 21, 2018 07:47

Add Unzip task

3f45618

ZipDirectory and strings

1301481

Update xsd

1580946

Update public API

611beb9

Fix copy/paste error

6427cc8

Remove Output properties of Unzip

e178b68

Add unit tests for Unzip

Fix runtime error on net46

9f066d0

Update xlf after rebase

2966a71

Add ZipDirectory unit tests

a0880f0

Fix broken tests

1cc0927

jeffkl added 5 commits May 21, 2018 07:47

Use cancellation token and Yield while extracting

4501ce6

Remove unused parameter

47662af

Add Overwrite property to ZipDirectory

6fe2b56

Use [PlatformSpecific(TestPlatforms.Windows)]

e028944

Address comments

8583157

jeffkl force-pushed the zip branch from b65ccd7 to 8583157 Compare May 21, 2018 15:12

cdmihai reviewed May 22, 2018

View reviewed changes

cdmihai approved these changes May 22, 2018

View reviewed changes

Yield execution in ZipDirectory

88d02fe

jeffkl merged commit f90c73c into dotnet:master May 22, 2018

jeffkl deleted the zip branch May 22, 2018 20:36

natemcmaster reviewed May 22, 2018

View reviewed changes

jeffkl mentioned this pull request Jun 5, 2018

Document some new tasks in MSBuild 15.8 MicrosoftDocs/visualstudio-docs#1080

Closed

natemcmaster mentioned this pull request Jun 8, 2018

Remove obsolete MSBuild tasks: DownloadFile, ZipArchive, UnzipArchive dotnet/arcade#244

Merged

Flithor reviewed Nov 4, 2023

View reviewed changes

Conversation

jeffkl commented May 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffkl commented May 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdmihai May 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rainersigwald commented May 21, 2018 via email

Uh oh!

jeffkl commented May 22, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffkl commented May 9, 2018 •

edited

Loading

cdmihai May 16, 2018 •

edited

Loading