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

[DONE; update documentation] publish Polly v6.0.0 as strong-named only #442

Closed
reisenberger opened this issue Apr 30, 2018 · 16 comments
Closed

Comments

@reisenberger
Copy link
Member

reisenberger commented Apr 30, 2018

CONSIDER publishing Polly v6.0.0 as strong-named only

Background

ASP.NET Core 2.1 is moving to consume Polly for HttpClient instances created through HttpClientFactory. This will allow the easy application of Polly resilience strategies to outbound calls through HttpClient.

This will significantly increase consumption of Polly's strong-named package (currently labelled Polly-Signed). That however will create conflicts for packages or projects that currently consume non-strong-named Polly: example; longer discussion.

There is no easy answer to this problem, as attested by discussion on numerous projects: octokit and the many projects that reference that issue.

Options:

  • Be purely non-strong-named: project cannot then be consumed in environments or by upstream packages mandating strong-naming; not an option for Polly due to ASP.NET Core.
  • Offer both strong-named and not (as Polly currently): but creates conflicts where a project ends up transiently referencing both.

Or:

Publish only strong-named

Some consensus among high-consumption projects now is around this, the NewtonSoft.Json strategy, as the least of all evils. @JeremySkinner of FluentValidation sums it up well here as does @sharwell elsewhere. I summarise also here.

TL;DR Switching to release Polly as strong-named only would be a one-time hit breaking change for consumers of Polly. But thereafter, having only the strong-named version of Polly in the market would prevent/reduce those conflicts (strong-named versus not) going forward.

(edited: for typos)


This is a proposal on which we would appreciate evidence-driven feedback. Particularly any consumption scenarios we may not be aware of. The proposed change is driven by the HttpClientFactory ASP.NET Core integration, which will drive many more Polly users to the strong-named version. ASP.NET Core has prior decision to be strong-named and thus consume strong-named assemblies.

@joelhulen
Copy link
Member

I am leaning toward publishing only strong-named, starting with version 6.0. This would result in us ceasing the Polly-Signed lineage, and we'd need to make it very clear in several places that the assembly in the Polly package is strong-named.

I had initially thought of having a new namespace called something like PollyCore that is strongly signed, and maintain that separately like we do with Polly and Polly-Signed, in order to reduce conflicts. Then thought better of it as adding more confusion and then dealing with ambiguous method names, etc. when you bring in HttpClientFactory to an existing project that uses Polly already. The other downside is that you'd have two identical DLLs, but with different names. This is not ideal for various reasons.

@JamesNK
Copy link

JamesNK commented May 1, 2018

I am leaning toward publishing only strong-named, starting with version 6.0. This would result in us ceasing the Polly-Signed lineage, and we'd need to make it very clear in several places that the assembly in the Polly package is strong-named.

.NET Framework treats changing the signing key on the assembly, including adding and removing one, as a different assembly. That means all libraries that depends on the package will break when someone upgrades to your new package until the library is recompiled.

That is fine for apps - people will recompile after upgrading packages - but other NuGet packages they use that depend your package will break until they are upgraded.

Read up on what happened when log4net changed its key.

This situation might have improved on .NET Core.

@iancooper
Copy link

I commented over here that consistency in the ecosystem is probably the best outcome we can hope for. The push to signed policy created by asp.net HttpClientFactory is essentially a fait accompli here, we are all going to have to take the signed version. So it would be easier to create a pit of success.

Strong naming is problematic, but I won't solve that problem...

@reisenberger
Copy link
Member Author

reisenberger commented May 1, 2018

Briefly to add some stats around the dependent packages issue highlighted ^^ by @JamesNK (thanks!), I don't know if the following stats are accurate/complete, but libraries.io helpfully gives us some idea of the packages currently dependent on Polly unsigned:

EDIT: To estimate the ripple out effect more: The number of dependent packages is as above, but the number of dependent packages that in turn have widespread consumption is small (ie projects with more than a handful of stars per libraries.io; some ad-hoc checks suggest this has correlation with high downloads).

@holytshirt
Copy link

Brighter project will depend on this change
BrighterCommand/Brighter#287

@sharwell
Copy link

sharwell commented May 1, 2018

💭 Is adding a strong name a binary breaking change for applications that are not strong-named? It's been so long since I did anything with an assembly that wasn't strong named that I'm not sure the answer.

@reisenberger
Copy link
Member Author

x-ref input from the Dot Net team

@iancooper
Copy link

@sharwell Yes, see this Brighter issue where we are tracking: BrighterCommand/Brighter#286

@iancooper
Copy link

But, we are happy to go this route. It seems to be the best path forward. We'll have to release as a 'breaking change' but we can live with that issue.

@reisenberger
Copy link
Member Author

Thanks @iancooper and @rvanmaanen for highlighting this important issue.

x-ref aspnet/HttpClientFactory#108 where I am querying the ASPNET Core team on the timing for this.

@sharwell
Copy link

sharwell commented May 2, 2018

Yes, see this Brighter issue where we are tracking: BrighterCommand/Brighter#286

This a different issue.¹ The situation I refer to would be the following:

  1. Create a library MyLibrary which does not have a strong name
  2. Create a command line application MyApp which does not have a strong name
  3. Recompile MyLibrary with a strong name
  4. Copy the binary from step (3) to the output directory from step (2), overwriting the binary in that directory originally from step (1)
  5. Attempt to run MyApp without recompiling it

If the above steps are successful, it would indicate that replacing a non-strong-name assembly with a strong-name assembly would not be a binary breaking change. The key insight leading to this conclusion is step (2) - specifically it's not possible to create MyApp with a strong name at this point.

Edit: I ran this test locally and found the following output in step (5):

Unhandled Exception: System.IO.FileLoadException: Could not load file or assembly 'StrongNameBreakingTest, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference. (Exception from HRESULT: 0x80131040)

¹ The linked issue differs from the case I am describing because the application in question references both the non-strong-name and the strong-name forms of the assembly at the same time.

@reisenberger
Copy link
Member Author

reisenberger commented May 3, 2018

@pardahlman (for RawRabbit); @TomPallister (for Ocelot); @fanliang11 (for surging): @RamjotSingh (for Microsoft.Bot.Connector.Teams)

libraries.io suggests you maintain libraries which have high downloads on nuget or high github stars, and consume Polly as a dependency. As a courtesy, please read this thread: Polly intends, from Polly v6.0.0, to switch to publish only as a strong-named version.

Polly v6.0.0 will be a release functionally identical to Polly v5.9.0, but a major-version bump (v6.0.0) as we switch to purely strong-naming.

Current (up to Polly v5.9.0)

nuget package kind
Polly non strong-named
Polly-Signed strong-named

Polly v6.0.0 onwards

nuget package kind
Polly strong-named

Impact

Both non-strong-named DLLs and strong-named DLLs can reference strong-named DLLs.

Whether the DLLs in your package are themselves strong-named or not, you will be able to reference the new Polly v6.0.0 strong-named package.

Why is Polly doing this?

ASPNET Core 2.1 is moving to consume Polly for HttpClient instances created through HttpClientFactory. This will allow the easy application of Polly resilience strategies to outbound calls through HttpClient.

This is expected to significantly increase consumption of Polly's strong-named package (currently labelled Polly-Signed). That however will create conflicts which apparently have no solution for packages or projects that currently consume non-strong-named Polly (example) (good summary) and wish to use the new ASPNET Core 2.1 features.

If we leave both non-strong-named and strong-named Polly in the market, we prolong the possibility of these conflicts into future months and years. We prolong the possibility that we (and you) will have users who complain that products are unusable, because some product (perhaps consuming yours) requires strong-named Polly as a transitive dependency via one axis, and non-strong-named Polly as a transitive dependency via another axis.

If we switch to a single, strong-named Polly release, there is initial impact while products switch to strong-named Polly, but the duration of pain is limited by there being only a single version in the market going forward.

Deeper discussion: 1; 2.

This strategy is the strategy used by many high-download nuget packages such as NewtonSoft.Json, FluentValidation, and Serilog, and is the strategy recommended by the Microsoft ASP.NET team.

Reducing onward impact

We are also adopting the so-called Newtonsoft.Json strategy described here to minimise onward impact. In this strategy:

  • nuget package numbers follow full semver (Major.Minor.Patch increments) to differentiate bug-fixes, new-functionality-without-breaking-changes, and breaking-changes.
  • Dlls partially follow Major.0.0 version numbering only (even if the true version is Major.Minor.Patch). This reduces the burden on projects which consume Polly of introducing excessive assembly binding redirects.

Edited: to fix links


Following community discussion, we believe this to be the best way forward (but comments and questions are welcome!).

reisenberger added a commit to reisenberger/Polly that referenced this issue May 3, 2018
Adopt new package and DLL versioning strategy,  See App-vNext#442.  Nuget packages will be full semver-numbered.  AssemblyFileVersion and AssemblyInformationVersion attributes will be full semver-numbered.  AssemblyVersion attribute will by Major.0.0.0 numbered.
@joelhulen
Copy link
Member

joelhulen commented May 3, 2018

We've just released Polly version 6.0.0-alpha to nuget.org. It has the following characteristics:

- Publish as strong-named package only (discontinue non-strong-named versions)
- Add .NetStandard 2.0 tfm
- Provide .NET4.5 support via .NetStandard 1.1 tfm
- Discontinue .NET4.0 support
- Remove methods marked as deprecated in v5.9.0

The first item is the one that addresses the primary issue. Because this is related to Polly and its relationship to ASP.NET Core 2.1,we're working on releasing Polly.Extensions.Http 2.0 alpha tomorrow or the next day, which follows the same single-strong-name package method and will reference this new Polly v6.0 package.

UPDATE: Polly.Extensions.Http 2.0 alpha has been published. This follows the single-strong-named package approach and references Polly v6.0-alpha.

@TomPallister
Copy link

Thanks for the heads up, Ocelot uses Polly so I will do whatever I need to fit in with you :)

Thanks for the amazing work and the Polly project!

@reisenberger
Copy link
Member Author

Polly v6.0.1 is published, delivering the changes discussed in this thread.

Leaving the issue open as I intend to transition the discussion from this thread and aspnet/HttpClientFactory#108, aspnet/HttpClientFactory#105 into the Polly wiki, as documentation.

@reisenberger reisenberger changed the title CONSIDER publishing Polly v6.0.0 as strong-named only [DONE; update documentation] publish Polly v6.0.0 as strong-named only May 26, 2018
@reisenberger
Copy link
Member Author

Documentation published to wiki

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

No branches or pull requests

7 participants