Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

net45 support for nuget.packaging.extraction.dll and transitive closure #3554

Merged
merged 23 commits into from
Aug 11, 2020

Conversation

rrelyea
Copy link
Contributor

@rrelyea rrelyea commented Aug 3, 2020

Fixes: NuGet/Home#9883 - covers motivation, etc..
Regression: No

Last working version: N/A
How are we preventing it in future: N/A

Fix
Details: see at bottom of this comment.

Testing/Validation
Tests Added: No
Reason for not adding tests: Deadline...same code base. Tracking issue to ensure proper coverage: NuGet/Home#9884

Validation: Successful CI run, VS Setup team running bits and verifying rejection of tampered packages, etc...

Details of fixes:

strategy for fixes.

  • use #if !net45 and #else as necessary to provide alternate techniques that work on net45.
  • built ireadonlycollectionutility to abstract HashSet not supporting IReadOnlyCollection in net4.5
  • built marshalutility to abstract generic way, vs earlier way
  • ifdef for empty array creation. new way has perf optimizations to share instances.
  • compiler made me fix a few things: -> nameof(foo), count() -> any()
  • signing functionality -- use alternate techique to get RSAPublicKey - @dtivel please review
  • dispose wasn't supported in 4.5 on a few types (x509 related) - skipping dispose best solution - @dtivel please review

new dll - nuget.packaging.extraction.csproj - mostly linked to the subset of code files from nuget.packaging that needed to pull in for compilation. pulled over copies of resx files. didn't subset those resx files.

@rrelyea rrelyea changed the title Prototype of net45 support for nuget.packaging.extraction.dll and nuget.protocol (and transitive closure) net45 support for nuget.packaging.extraction.dll and transitive closure Aug 6, 2020
@rrelyea rrelyea marked this pull request as ready for review August 7, 2020 17:16
Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

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

Some source-build comments.

@rrelyea rrelyea requested a review from dominoFire August 7, 2020 20:39
Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

I think we can minimize the impact by reducing the number of projects that build net5 assemblies.

Might be easier to compile everything into 1 assembly.

Ideally we don't publish this on NuGet.org, that will require changing our release definition.

@rrelyea rrelyea requested a review from nkolev92 August 10, 2020 17:05
@nkolev92
Copy link
Member

@rrelyea Given that my feedback is summarized in https://github.com/NuGet/Client.Engineering/issues/461, I see no reason to wait on me here.

Copy link
Contributor

@dtivel dtivel left a comment

Choose a reason for hiding this comment

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

Optimistically approving. Request a rereview from me if there are non-trivial changes.

@rrelyea, since X509Certificate didn't implement IDisposable until .NET 4.6, your changes look appropriate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants