Stop merging Castle.Core

[blairconrad]

Recently we found a bug that users could've worked around by using a newer version of Castle.Core than we bundle in the .NET Framework flavour of our package.

@thomaslevesque commented (#1257 (comment)):

this issue makes me wonder if we should really ILMerge Castle.Core... We probably discussed it before, but there's a compelling argument against it: in such a case when there's a bug in Castle.Core, the end-user can't just work around the issue by upgrading to the next version of Castle.Core where the issue is fixed. They have to wait for us to release a version that uses the newer Castle.Core.

I responded:

I see your point. I think the original reason for merging was to work around situations where users' production code required a different version of Castle.Core than we liked. What's the lesser of the two evils?

@thomaslevesque reresponded:

I'm not sure... if they want a more recent version, fine, but if they need an older one, they're in trouble. Maybe we should offer a separate package where Castle.Core is not ILMerged. In fact, if we do that, it would be more consistent to stop using ILMerge in the main package, and do it only in the new one (which would only target net40/net45, since ILMerge doesn't work with .NET Standard). Anyway, it's just a random idea, and a bit off-topic...

And now we're here!

[adamralph]

An interesting question is: if ILMerge or ILRepack started to support netstandard, would FIE start merging in the Castle.Core netstandard assembly?

Effectively, the IL merge means a bug in Castle.Core is a bug in FIE. If a new Castle.Core is released which fixes a bug, then that bug gets fixed in FIE by a new release which upgrades to that version. Similarly, if a new version is buggy and the bug fix is delayed, FIE can also go back to the older version in a newer release. Going back a version doesn't necessarily mean losing anything.

The lack of IL merge for netstandard is effectively bringing back DLL-hell where it was originally vanquished by the use of IL merge. Originally I was strongly opposed to IL merging, but the reality of DLL-hell made me realise that it's a better thing to do. There is a case when you should obviously not IL merge, and that is when your public API is expressed using types defined in that third party package. These would typically be integration packages, sitting between two other packages. DLL-hell is something that those packages are explicitly buying into, and their maintenance and usage is necessarily under contract with that particular devil. However, in the case of a package which uses a third party package purely as an internal dependency, there is no reason for the consumer to even know that the third party package is being used. A package with no dependencies is the best behaved citizen wrt to versioning.

[thomaslevesque]

I'm exhuming this old issue, because the problem is getting worse, and I really think ILMerge is holding us back and preventing us from making improvements to FakeItEasy.

• I'm currently having problems with PdbGit, which is old and unmaintained. I'd like to switch to SourceLink instead, but we can't, because ILMerge loses the source information added by SourceLink.

• We have to keep emitting large legacy PDB files, because ILMerge doesn't support Portable PDB.

• We can't build on Linux, even using the netfx reference assemblies package, because ILMerge doesn't work on Linux.

• Some of the issues above might have workarounds (e.g. using PDB2PDB to convert PDBs to the new Portable format), but it would complicate the build process, which is already more complex than it should because of ILMerge (directly or indirectly).

An interesting question is: if ILMerge or ILRepack started to support netstandard, would FIE start merging in the Castle.Core netstandard assembly?

It is an interesting question. Maybe we would, if they implemented all the necessary features that are currently missing, and were well-maintained, and enabled all the scenarios that are currently blocked.

But the fact is that both ILMerge and ILRepack are as good as dead (actually, it looks like someone has taken over the maintenance of ILRepack and started publishing new releases, but it still doesn't support portable PDBs). As far as I know there's no alternative to these tools.

The lack of IL merge for netstandard is effectively bringing back DLL-hell where it was originally vanquished by the use of IL merge.

I hear your arguments, @adamralph, and they're sound. But we've started releasing non-merged assemblies for .NET Core in v3.0.0, almost 3 years ago, and we never received any report of DLL-hell problems caused by Castle.Core.

Neither Moq nor NSubstitute merge Castle.Core, they just declare it as a package dependency for all TFMs. @stakx, @zvirja, have you ever received issues due to conflicting Castle.Core versions? If not, it's unlikely that we will, since we don't have nearly as many users.

So, while having no dependencies is a worthy goal, I don't think it outweighs the benefits that dropping ILMerge would bring.

@blairconrad thoughts?

[blairconrad]

@thomaslevesque, I agree. While @adamralph is correct that a package with no dependencies is the best-behaved package, we are currently in a situation where we are releasing versions of FakeItEasy that do have an external dependency on Castle.Core, and it's unlikely to stop. (Unless ILMerge or an equivalent tool springs up for .NETStandard. And that point, maybe supporting tooling would be improved and we'd like to merge. We can revisit at that point.)

I think your points against ILMerging are strong enough to outweigh potential downsides. I am for the change. Perhaps in a major release? I doubt it will break anyone, but there's the potential and it's worth drawing attention to the change.

[stakx]

@stakx, @zvirja, have you ever received issues due to conflicting Castle.Core versions?

Yes, such reports pop up from time to time. For example, we got devlooped/moq#935 just recently.

(We used to get tonnes of NuGet versioning problem reports in the earlier days of .NET Core, when <PackageReference> was still new and NuGet & MSBuild hadn't yet learned to cooperate as well as they do today. For example, devlooped/moq#418 or castleproject/Core#307. These issues eventually led up to Castle.Core adopting a simpler assembly versioning scheme—setting the major version only and leaving all others at 0—which improved things a lot for projects targeting the full .NET Framework which has a very strict assembly version matching policy.)

BUT: That kind of report doesn't just pop up for Castle.Core. We've seen similar reports caused by any and all NuGet dependencies, direct and indirect; among them, System.ValueTuple, System.Threading.Tasks.Extensions, and System.Net.Http. The .NET BCL guys sometimes get their versioning wrong, too. So these kinds of reports perhaps shouldn't stop you from referencing Castle.Core via NuGet instead of ILMerge-ing it.

(At this point, I could start ranting about NuGet picking the lowest compatible version of a package instead of the highest one, provoking problems needlessly, and how the average .NET user doesn't understand NuGet & MSBuild package version resolution well enough to help themselves when version conflicts occur... but that's perhaps just how things are. And truth be told, the situation has subjectively improved a lot in the past 1 year or so.)

For Moq, I am roughly doing the following to minimize version conflict issues:

1. Have as few NuGet dependencies as possible.
2. Update all dependencies to the latest version every few months. (This follows directly from NuGet selecting the lowest compatible version by default, instead of the latest one, so .NET libs often take the approach of forcing users to upgrade their dependencies.)
3. Urge users to migrate their project files to the new SDK style format, which generally causes less problems, as it has a slightly different NuGet version selection algorithm & does automatic binding redirects.
4. Close issues about random NuGet problems with a blanket response. We can't rescue everyone.

Also, I usually publish a new Moq version very soon after Castle.Core publishes a new version... that seems to help, too.

Despite (1), I've resisted the occasional temptation to go back to ILMerge-ing Castle.Core (which Moq also used to do years ago) because ILMerge seems like it's potentially an out-of-date tool, and slowly becoming arcane: It seems safer in the long run to just go along with the "obvious" way of doing things, i.e. NuGet references.

Finally, I think things will get even better once Castle.Core targets netstandard2.x, as that will reduce the number of transitive dependencies a lot for our own netstandard2.x targets.

[thomaslevesque]

Thanks for your feedback, @stakx!

[adamralph]

But we've started releasing non-merged assemblies for .NET Core in v3.0.0, almost 3 years ago, and we never received any report of DLL-hell problems caused by Castle.Core.

I believe that proves that merging Castle.Core into the .NET Framework assembly is rather pointless.

[thomaslevesque]

Ha! I didn't think I'd convince you, @adamralph 😉

[blairconrad]

Upgrading from "discussion". Labelling as "breaking" just in case it'll break someone's use case, but I'd be surprised. I think we should do this in the next major release.

[zvirja]

Hi guys! I could speak for NSubstitute and AutoFixture projects (which are both testing tools; latter doesn't use Castle.Core) and don't remember about having regular issues about dependencies. For AutoFixture we get requests from time to time, as people don't know how to cook binding redirection correctly. But that's occasional.

I read also your concerns about dependency hell and could say that never saw issues about conflicting versions of dependencies. It could happen though that I just joined the project late 😅

Could also advocate the idea to extract dependency, as one or a few times we even asked our NSubstitute clients to update Castle.Core to fix the issue they have, as it was caused by the library itself. That was pretty convenient.

[blairconrad]

Thanks for chiming in, @zvirja. That's very helpful information, and has served to strengthen my pro-decoupling stance.

[blairconrad]

Thanks, @thomaslevesque. It's official: we're on the road to Majorreleasetown.

[afakebot]

This change has been released as part of FakeItEasy 6.0.0-beta.1.