U4-8409 - Remove SharpZipLib dependency from Core

Created by Shannon Deminick 04 May 2016, 09:49:03 Updated by Claus Jensen 25 Oct 2017, 10:29:46

Tags: Up For Grabs Unscheduled PR Community Contrib

Subtask of: U4-7594

There is no reason to use this, .Net comes with it's own zip libraries that we should use, this also goes towards better supporting .Net Core, it's also worth noting that this lib is discontinued and simply not supported.

Comments

James South 01 Jun 2016, 01:24:03

@Shandem I have some experience with the .NET zip libraries (png encoding/decoding) so I might be able to take this on. Is there anything I should keep an eye out for?


Shannon Deminick 01 Jun 2016, 07:56:36

Nothing in particular, it should be pretty straight forward. All of the functionality we need will be available in the System.IO.Compression library, all that is being done in the core is zipping and unzipping folders on disk (IIRC)


James South 02 Jun 2016, 02:12:24

Ace... I'll get on to it asap then.


Damiaan Peeters 18 Jun 2016, 22:00:06

I have some work done. No time to test this currently, need to get up early tomorrow.

This is the pull request: https://github.com/umbraco/Umbraco-CMS/pull/1336


Shannon Deminick 20 Jun 2016, 07:58:32

Great! I think @JimbobSquarePants had also done some work on this, not sure if he submitted a PR already? If so please let me know so i can consolidate.


James South 22 Jun 2016, 07:11:04

@Shandem @dampee I hadn't finished mine and would have had to revisit the code to fix my jetlagged attempt. I've commented in the PR on a couple of things.


Mikulas Tomanka 29 Mar 2017, 16:30:44

@Shandem Is this still for grabs for a newbie in Umbraco? I checked the mentioned pull request and it seems like it wasn't accepted and won't be.

EDIT: After being finised with the PR - https://github.com/umbraco/Umbraco-CMS/pull/1845 , I realised I did the change fron dev7 and not dev 8 as "due in version" states is taht an issue?


Shannon Deminick 29 Mar 2017, 23:32:49

@pijemcolu Awesome work! I've responded on the PR, just a few unit tests to fix up


Mikulas Tomanka 30 Mar 2017, 14:27:54

@Shandem Did commit and fixed the three unit tests. A quick search for ICSharpCode.SharpZipLib still returns a couple of results though.

  • umbraco.cms.businesslogic.utilities.Zip.cs has an UnPack() method seems like it could be completely removed and Umbraco.Core.Packaging.PackageExtraction.cs class could be used instead -umbraco.cms.businesslogic.packager.Installer has a straightforward copy of the above mentioned.
  • umbraco.cms.businesslogic.package.utill.cs has a ZipPackage() method using the library
  • umbraco.presentation.translation & umbraco.preentation.ubraco.presentation has unused using statements.

Can I feel free and fix those ones as well? All the references to sharpziplib nugget package could be removed afterwards.


Shannon Deminick 30 Mar 2017, 22:22:21

Yes please do :) all code cleanup and removal (when possible) is what v8 is all about... And its pretty fun to do


Mikulas Tomanka 31 Mar 2017, 15:13:58

@Shandem As an update I am sitting on a commit dealing with all of the above menioned issues. That doesn't make it possible to remove the nugget package ICSharpCode.ZipLib because of a dependency in LuceneNet. The dependency is getting resolved in 4.0 release. (https://issues.apache.org/jira/browse/LUCENENET-555)

Now I have to test the code which I am sitting on as it would be a small miracle if it worked straight out of the box mainly due to paths issues which I foresee.

A question: Are scripts RevertToClean/EmptyInstall.bat are intended to be used for ... what they imply by the name?


Shannon Deminick 10 May 2017, 03:52:54

Ah, i didn't realize Lucene 3.x still shipped with ICSharpCode.ZipLib, damn. OH well 4.x will be out very soon as I've been following the lucene mail list :)

I haven't used RevertToClean/EmptyInstall.bat for a long time but they are mainly there for testing new installs. So once you install, perform your tests, etc... you can run these batch files to revert all local changes to be back to their original state so you can start your testing from scratch with a 'new' website


Stephan 16 Oct 2017, 17:05:23

PR https://github.com/umbraco/Umbraco-CMS/pull/1336 and PR https://github.com/umbraco/Umbraco-CMS/pull/1845 have been merged into v7 and v8 - many thanks to all.

Nothing to review for v8, for v7 need to review this new PR: https://github.com/umbraco/Umbraco-CMS/pull/2248

We are still referencing ISharpZip in a few places where Lucene still wants it, but otherwise we're clean.


Sebastiaan Janssen 18 Oct 2017, 11:38:20

We have some errors building now, left a comment on the PR.


Mikulas Tomanka 19 Oct 2017, 11:15:16

I've updated the PR branch https://github.com/umbraco/Umbraco-CMS/pull/2248 , removing last three references of SharpZipLib.


Stephan 20 Oct 2017, 15:44:43

One thing - in utill.cs, ZipPackage, it looks like you are creating the zip in an in-memory MemoryStream that's just... disposed at the end, so it's not going to create anything on-disk? Presumably you want to use a real file stream to write the zip content to disk?

(other files are ok)


Mikulas Tomanka 23 Oct 2017, 07:22:23

@zpqrtbnk updated the PR and now writing the archive to the disk.


Claus Jensen 24 Oct 2017, 13:23:30

@Mikulas I've removed a couple of left over using statements (unused) and references to the SharpZipLib package. It will automatically get added by the Lucene.net dependency while it's needed there.. so we don't need to explicitly reference this package in our files since we don't have the dependency anymore.

When Lucene is updated - it should disappear along with it :)

Confirmed that everything still builds, works (can publish/render content) and that ICSharpCode.SharpZipLib.dll is still in /bin (added by Lucene I'd assume) after removing the last dependencies on our files.

Do we want to retarget this for v8 instead of a v7 minor, since due in version says v8? (@Shandem )


Shannon Deminick 24 Oct 2017, 22:26:46

We have 2 options: we can retarget this due in version 7.8, all we are doing here is removing a dependency that we don't want to use and replacing the logic with native .NET code. Of course because of our current lucene version it will be part of the release but means that we are not actively using it and as of v8 it will just disappear

But... this isn't really an important fix that people will care about or even notice, so perhaps just re-targeting the PR to v8 is better


Claus Jensen 25 Oct 2017, 10:29:44

Have merged this to 7.8 - @zpqrtbnk has already manually merged this to v8 so we're good there.


Priority: Up for grabs

Type: Bug

State: Fixed

Assignee:

Difficulty: Normal

Category:

Backwards Compatible: True

Fix Submitted:

Affected versions:

Due in version: 8.0.0, 7.8.0

Sprint: Sprint 70

Story Points: 1

Cycle: