Skip to content
This repository has been archived by the owner on Nov 17, 2018. It is now read-only.

Remove strong named assembly version of Polly for non-SN version #105

Closed
iancooper opened this issue Apr 30, 2018 · 30 comments
Closed

Remove strong named assembly version of Polly for non-SN version #105

iancooper opened this issue Apr 30, 2018 · 30 comments

Comments

@iancooper
Copy link

iancooper commented Apr 30, 2018

It is great that this project has decided to use Polly over writing it's own Retry and Circuit Breaker implementations. But, it should be remembered that a lot of other projects have been using Polly for some time, and ship with it as a dependency.

Those projects tend not to use signed assemblies, which are viral and unnecessary in a post-GAC age HttpClientFactory ships with a signed version of Polly and thus breaks all those projects. It is a destructive bullet fired right into the existing ecosystem as a result.

See: BrighterCommand/Brighter#286

At its heart this exposes the very issue with signed assemblies, their virality. They are an infection that spreads throughout the ecosystem.

Please remove the dependency of this project on a signed version of Polly so that you can continue to be compatible with all the other projects in the ecosystem that already use Polly.

@martincostello
Copy link
Contributor

ASP.NET Core needs to ship with strong-naming, so they'd have to remove it entirely 😞

@iancooper
Copy link
Author

But why are they shipping Polly, not just referencing the existing package?

@martincostello
Copy link
Contributor

Signed packages can't reference unsigned packages.

@iancooper
Copy link
Author

Let's just break the ecosystem then. Because everyone who is currently using Polly is now broken. That is a catastrophic change, not a positive step forward for the community

@martincostello
Copy link
Contributor

I guess they were trying to let people get the benefits of Polly for free out-of-the-box without having to know the implementation details.

AFAIK there's only one way to resolve this issue, which is to remove this functionality completely. Polly comes in two flavours - using the signed version breaks the non-signed consumers (who can maybe fix it by switching to signed instead, where it's just a preference/dislike of strong-naming, rather than a technical constraint), using the unsigned version breaks the signed consumers with no fix path as signed packages can't reference unsigned packages.

@rvanmaanen
Copy link

I guess they were trying to let people get the benefits of Polly for free out-of-the-box without having to know the implementation details.

That's great and I thinkt the integration with Polly is awesome, but it kinda sucks for everyone not using signed packages. So even if Ian fixes this, then I have to head over to yet someone else to ask them to use his signed version and go through this again.. Don't know of any other solution either though..

@iancooper
Copy link
Author

I think the issue could be that Polly-Signed is actually strong named. Do MS require strong named?

@martincostello
Copy link
Contributor

ASP.NET Core needs to ship with strong-naming...

Otherwise anyone who needs to strong-name their code couldn't consume .NET Core at all.

@iancooper
Copy link
Author

But this is just painful as it is a breaking change for all existing users of Polly. It's destructive to the downstream ecosystem

@martincostello
Copy link
Contributor

Like I said, the only resolution to this issue that isn't doing nothing is to remove the Polly-related code completely.

@iancooper
Copy link
Author

OK, but this is damaging to the existing ecosystem as is, because it breaks everyone else

@iancooper
Copy link
Author

iancooper commented Apr 30, 2018

BTW, the switch to strong naming is not simply preference, as it's viral. In order to strong name, I have to use only strong-named packages and any consumer has to now use strong naming. If those packages do not provide a strong named version, then you can no longer use them. And the chain is the problem, it only takes one weak link and you are done. It's not a trivial change and it ripples through the ecosystem. So it is a constraint.

The issue here is the problematic need of aspnet to strong-name, which is placing burdens on the ecosystem.

@martincostello
Copy link
Contributor

The root of the problem is that strong-naming ever existed, but as that's a fait accompli, library developers are stuck between a rock and a hard-place of whether to use unsigned packages and completely preclude their use from developers who need strong-naming (for whatever reason, I'm making no assertions here about why someone would choose/need to do that and whether they should or not), or providing a strong-named package that requires other packages to do the same to preserve the dependency chain (or providing one of each).

The former is a hard-block, signed can't use unsigned, where as unsigned can use signed. In a world of compromises, providing a signed package that (dependency chains aside) works for both use cases is the choice that makes the most sense.

Personally, I have no strong opinions of strong-naming in of itself, but it's a lot less headache as a library developer (when you have no dependencies of your own beyond the Framework/SDK) to just provide one package and sign the assemblies in it.

I'm sure a similar debate would have happened for Newtonsoft.Json at some point in time (unless it's always been strong-named), given that that's a dependency you can use anywhere as well as being a dependency of ASP.NET Core itself.

@iancooper iancooper changed the title Remove signed assembly version of Polly for unsigned version Remove strong named assembly version of Polly for non-SN version Apr 30, 2018
@iancooper
Copy link
Author

It's not as simple as that. Strong naming becomes viral because now, all my dependencies must be strong-named. So even if it is only my package that depends on Polly, all the packages in my chain must now support strong naming. If they don't then I have two choices (a) stop using Polly (b) fork and rename Polly to use my own version instead. The chain of dependencies is the issue.

This is the heart of the failure of strong-naming, that you couple everyone into the decision, and coupling of that kind makes it hard to release. Strong naming also introduces harsher versioning constraints to support side-by-side loading. which forces assembly binding redirection (and woe betide platforms that don't support that). In fact lack of support for assembly binding redirection is why most people stopped using strong naming. Most developers don't understand binding redirection, or why they would need it.

Indeed if Polly intends to version rapidly, then strong naming is highly problematic.

Jeremy Miller has a reasonable account of experiences here: https://jeremydmiller.com/2014/04/28/fubumvc-lessons-learned-strong-naming-woes-and-workarounds/

And for what? The GAC? In a post container and 12 factor app world, no one should be installing in the GAC because every container should hold one app and one alone. For security? Most snk files in GitHub repos are not password protected so as to make it easy to build local versions.

Strong naming is a tax on the OSS ecosystem with no benefit.

Our choices are (1) Fork Polly to 'Parrot' or the like so that we can avoid a clash (2) Break Polly library support out of Brighter, and into a plug in assembly that abstracts our dependency on Polly and just sign that [Note that MS could do this if they shipped their extensions without badging them as MS] (3) ILMerge Polly into Brighter to avoid dependencies.

But anyone who has used Polly in their library will now have to review their entire dependency chain, ensure that they can obtain a strong named version, and sign there version. In turn, anyone who uses that library will have to make the same choices.

This is dropping a large rock into the pond - there will be a lot of outward ripples

@martincostello
Copy link
Contributor

The whole thing is summed up nicely by this 2.5 year old comment:

There is no easy solution. SN is dumb but that's the world we live in.

@iancooper
Copy link
Author

Polly may be forced into the Newtonsoft strategy, and never again increment its assembly version.

@martincostello
Copy link
Contributor

AFAIK Newtonsoft.Json increments the assembly version for each major version.

@iancooper
Copy link
Author

Sure, but then you have breaking changes anyway

@khellang
Copy link

khellang commented Apr 30, 2018

I'll just subscribe to this thread by posting a link to the Epic Strong-Naming Thread of 2014; octokit/octokit.net#405 (which serves as the ultimate strong-naming resource on the web) 😉

@reisenberger
Copy link
Contributor

reisenberger commented Apr 30, 2018

AFAICS, the Newtonsoft.Json strategy is:

  1. only a single nuget package, DLLs within are strong-named
  2. semver the nuget packages
  3. assemblies within the package have major version only for "Strong name" and "Full Name"
  4. assemblies within the package have major.minor.patch for "AssemblyFileVersion" and "AssemblyInformationalVersion"

image

(EDITED: to paste above image)

We note a disadvantage of (3) major-version-number-only (ie X.0.0) on the assemblies within the package: if product A references Polly v6.1.0, while product B references Polly v6.3.0, and A were to install Polly in the GAC for whatever reason, it seems possible (per discussion elsewhere) to end up in a situation where A installs 6.1, B ends up referencing 6.1 from the GAC instead of 6.3, and product B blows up at runtime on the lack of some 6.3.0-specific feature. GAC, I know - less relevant now? - just highlighting for completeness.

Other high-volume packages (nUnit; AutoMapper) do not do (3); they number assemblies within the nupkg with full semver for "Strong name" and "Full name".


Closing thoughts:

  • Polly has no downstream dependencies. But we produce related packages (eg Polly.Caching.MemoryCache) which reference Polly; we would ripple similar changes through these.

  • Decisions and implementation might ideally be before ASP.NET Core 2.1 RTM ( @rynowak @glennc ? ) . If Polly adopted this strategy, we would switch to that as a breaking change, v6.0.0. It could make sense for the ASP.NET Core 2.1 packages to reference Polly v6.0.0 to highlight that breaking impact.

@reisenberger
Copy link
Contributor

@iancooper If Polly adopted the Newtonsoft.Json strategy, would you still need to produce a strong-named version of Brighter? Could the existing, non-strong-named version of Brighter not consume the strong-named version of Polly? Is that the way Brighter is successfully referencing NewtonSoft.Json at the moment?

Noted: If Polly adopts that strategy, projects currently consuming non-strong-named Polly do take the hit of the conflict with the strong-named version of Polly referenced by HttpClientFactory, until they switch to strong-named Polly. However, by publishing only a strong-named version of Polly going forward, not both versions, we would at least reduce these conflicts over the long term.

@joelhulen
Copy link

To echo @reisenberger's comments, we really do want to come to a quick consensus to the best way forward. There are some things we'll have to figure out, like if we go the Newtonsoft.Json route, do we just keep using the current Polly package name (which is currently unsigned) and do away with the Polly-Signed package altogether starting with v6.0? Is there any benefit to going with a new package/namespace to differentiate signed and unsigned packages? It'd be interesting to hear what the ASP.Net team's view on this is, as well as others in the community who have dealt with this already.

@iancooper
Copy link
Author

@reisenberger I guess that if you go the Newtonsoft route we will cope the same way we do there. I understand your position, that it might be easier for you to just adopt a 'one-only' policy and I do appreciate the visibility the project gets from inclusion in this way.

From our point of view if there is an ecosystem change to sn-only from point X it does make management easier, we now know that Polly is always sn, so worry less about clashing with other deployments of Polly which will also be sn. As you say, we reach the Newtonsoft position.

I'm not a big fan of stong naming, but having only one dep chain is the issue we need to solve and given I won't change the sn issue, maybe it is the way forward.

@iancooper
Copy link
Author

I'm ok with folks closing this issue if you agree the 'Polly all-signed' approach is the way forward

@joelhulen
Copy link

I think the 'Polly all-signed' approach really is our best option, but I'd still love to hear opinions from the .NET team...

@DamianEdwards
Copy link
Member

.NET team member here. Indeed, I'd recommend you go with the "single package containing strong-named assemblies" approach. I'd deprecate the "-Signed" package, and rev the "Polly" package to a new major version, which now contains an OSS-signed assembly. WRT to the actual assembly version, and adopting the JSON.NET strategy or not (major version only, or SemVer) I have less of an opinion, other than to say that JSON.NET is pretty special in the world of .NET, as it's depended on by even Visual Studio, so the versioning strategy it has adopted is one of ultra conservatism.

As others have pointed out, strong-naming is a reality of the .NET ecosystem for the foreseeable future, but this approach has been successfully adopted by many OSS projects now.

@joelhulen
Copy link

@iancooper, at this point I believe you can close this issue. The consensus is that we will move to a single Polly package that is signed, as has been covered at depth here and here. What we, as the Polly team, need to decide is our version strategy. We'll keep that conversation going here.

@reisenberger
Copy link
Contributor

@DamianEdwards Thank you for the valuable input; it's great to have the team's perspective.

@iancooper
Copy link
Author

OK, thanks @reisenberger @joelhulen and @DamianEdwards We will track against the Polly issue and move that way.

@joelhulen
Copy link

joelhulen commented May 3, 2018

@iancooper 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

Obviously the first item is the one that addresses the issue as raised here. Could you please test it out when you get the chance? We also have the .NET Core team testing it out on their end, and we're working on releasing Polly.Extensions.Http 2.0 alpha tomorrow which follows the same single-sn package method and will reference Polly v6.0.

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

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

No branches or pull requests

7 participants