Skip to content

Conversation

@zone117x
Copy link
Contributor

@zone117x zone117x commented Sep 11, 2016

I certify that I own, and have sufficient rights to contribute, all source code and related material intended to be compiled or integrated with the source code for the SharpZipLib open source product (the "Contribution"). My Contribution is licensed under the MIT License.

Creating a pull request for review purposes. These changes need reviewed and tested before merging.

No features were removed during this refactoring other than some exception serialization related stuff.

  • The ZIP AES changes need tested
  • The streams were improperly overriding Stream.Close() according to https://msdn.microsoft.com/en-us/library/ms143450(v=vs.110).aspx ... these were changed to Stream.Dispose(bool) but there may be problems around garbage collection - needs reviewed and tested
  • Unsupported exception serialization and constructors were removed
  • DefaultCodePage changes need reviewed
  • Some commenting may need updated

* The ZIP AES changes need tested
* The streams were improperly overriding Stream.Close() according to https://msdn.microsoft.com/en-us/library/ms143450(v=vs.110).aspx ... these were changed to Stream.Dispose(bool) but there may be problems around garbage collection - needs reviewed and tested
* Unsupported exception serialization and constructors were removed
* DefaultCodePage changes need reviewed
@xied75
Copy link
Contributor

xied75 commented Sep 14, 2016

Agree on the change to remove using Stream.Close(), it's no longer exist in the coreclr/corefx.

It's better to separate into multiple smaller PRs so it get reviewed faster.

@zone117x
Copy link
Contributor Author

zone117x commented Sep 14, 2016

Separating them is a bit hard because the above changes were the minimum required to get the .netstandard lib to compile at all.

We could make those individual changes only for the existing lib without netstandard and PR those one at a time, but I don't have time to do that. I got my fork working for my purposes and mostly made this PR to give some head start to the main contributors.

@xied75
Copy link
Contributor

xied75 commented Sep 15, 2016

If @McNeight is willing to consider merge it, I can volunteer to help the review.

@jahmai-ca
Copy link

Missed specifying code signing key on the netstandard project.
Also, nuspec not updated.

Otherwise, I have built and modified the existing nuget package by hand and our unit tests that use this still pass, so well done 👍

@McNeight McNeight added this to the 1.0 milestone Oct 10, 2016
@McNeight McNeight self-assigned this Oct 10, 2016
@McNeight
Copy link
Contributor

This is great work! My lingering concern, and the reason why I took so long to finally commit this, is that it breaks some of the automated build and test. I've been hoping to get it cleaned up before now, but I've realized that I need to get the change in and then work on cleaning up.

Any help with getting the automated build and test setup back to normal again would be much appreciated.

-Neil

@McNeight McNeight merged commit 155ca22 into icsharpcode:master Oct 31, 2016
@xied75
Copy link
Contributor

xied75 commented Nov 2, 2016

I did a clean clone and trying to build with VS2015up3, somehow I got this error:

1>C:\Program Files (x86)\MSBuild\Microsoft\NuGet\Microsoft.NuGet.targets(140,5): error : Your project is not referencing the ".NETFramework,Version=v4.5" framework. Add a reference to ".NETFramework,Version=v4.5" in the "frameworks" section of your project.json, and then re-run NuGet restore.

In fact I could right click and just build the portable project, but not the main 4.5 project.

Just wondering, netstandard1.3 will support from 4.6, are we going to keep 4.5 project?

@zone117x
Copy link
Contributor Author

zone117x commented Nov 2, 2016

This is fixed in my last pull request #150

@McNeight McNeight added enhancement Feature request or other improvements of existing functionality xplat Cross Platform, related to running SharpZipLib in non-windows environments labels Nov 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Feature request or other improvements of existing functionality xplat Cross Platform, related to running SharpZipLib in non-windows environments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants