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

Castle.Core having minor and patch level version in AssemblyVersion #288

Closed
skovsende opened this issue Jul 13, 2017 · 28 comments
Closed

Castle.Core having minor and patch level version in AssemblyVersion #288

skovsende opened this issue Jul 13, 2017 · 28 comments
Labels
Milestone

Comments

@skovsende
Copy link

I had a good discussion with @stakx from the Moq project in this thread: devlooped/moq#418

It seems to be Castle.Core does not follow best practice when it comes to AssemblyVersion, which leads to all kinds of problems when people depend on Castle.Core.

We ended up finding a really good explanation of the problem here: https://codingforsmarties.wordpress.com/2016/01/21/how-to-version-assemblies-destined-for-nuget/

Is it something you have any opinions about? If needed a may be able to find some time in the coming weeks to create a PR, but as I believe a major version bump is required, I would like to run the idea by you before doing anything.

@ghost
Copy link

ghost commented Jul 14, 2017

@skovsende - I did not read the discussion but did read the the blog post. I think the blog post obfiscates the problem and the solution in a pile of waffle and is not very well written. Can you summarise this for clarity?

@skovsende
Copy link
Author

skovsende commented Jul 14, 2017

I'll try. The current situation as I see it:

Imaging I have an application, MyApp, that uses two nuget packages: PackageX and Castle.Core

Library PackageX is a nuget package containing a strong named assembly that uses version 4.1.0 of Castle Core. In the nuspec the dependency is set to 4+, to be compatible with new versions as long as Castle.Core follows semantic versioning.

Then version 4.1.1 of Castle.Core is released, and I update MyApp to use the new version of Castle.Core. This gives problems for PackageX, because it's strong named and uses the AssemblyVersion of the Castle.Core dll to make sure it's running against the correct version. A way to fix this is to add assembly redirects in your app.config, which tells that whenever anyone want's 4.1.0.0 - that 4.1.1.0 is usable instead. It works, but it's pretty cumbersome, especially when the amounts of packages increase.

If Castle.Core instead changed it's strategy to set AssemblyVersion to 4.0.0.0 for all nuget package releases with version 4.x, the situation becomes much simple for the consumers of Castle.Core.

In that case MyApp updates the reference, the strong-named PackageX uses the new dll from Castle.Core without problems because the AssemblyVersion is the same as the one it was built against originally.

I have seen a lot of libraries handle AssemblyVersion this way, Serilog and EntityFramework is examples I've run in to and all Microsoft.VisualStudio dlls/packages handle versioning this way as well.

And note that this is only for AssemblyVersion - AssemblyFileVersion would still follow the version of the nuget package, so if you do a Properties->Details on the dll, you would still get the full version number.

I hope that makes my point slightly clearer.

@ghost
Copy link

ghost commented Jul 14, 2017

I believe you have explained this way better than the blog post! 👍

It is also one of the main reasons I use Paket instead of Nuget for managing my dependencies. It takes care of bindings for you. @forki is the author and I think he has done a great job of dealing with this. It not only deals with Assembly bindings but can also garbage collect transitive dependencies that are no longer valid. Nuget is very bad at cleaning up after itself also.

@jonorossi has not responded to this yet but I am almost certain he would be happy with this "work around". It is cheap to implement and it saves people heaps of trouble.

Please go ahead and submit a PR, we can review it further there.

@jonorossi
Copy link
Member

It seems to be Castle.Core does not follow best practice when it comes to AssemblyVersion, which leads to all kinds of problems when people depend on Castle.Core.

Yep, Castle Core 4.x appears to have lost what we used to do with the change to the new .NET SDK tooling. 4.1.0 and 4.1.1 will be affected, 4.0.0 was obviously the major release.

We used to do x.x.0.0, but x.0.0.0 sounds even better. AssemblyFileVersion should still have the actual version.
https://github.com/castleproject/Core/blob/v3.3.3/Settings.proj#L45

(recovering from the flu so behind on emails)

@jonorossi jonorossi added the bug label Jul 14, 2017
@jonorossi
Copy link
Member

I just checked Windsor 4.0.0 to see what assembly version it has embedded, and it's all good, NuGet is giving it the lowest minor version and there is no patch version in the 4.0.x series.

.assembly extern Castle.Core
{
  .publickeytoken = (40 7D D0 80 8D 44 FB DC )                         // @}...D..
  .ver 4:0:0:0
}

@stakx
Copy link
Member

stakx commented Jul 16, 2017

@jonorossi, since Moq is having the same issue and I am thinking about applying a fix, I'd be interested in how you're going to fix this for Castle.Core: Will you reset the assembly major version to 4 or 5? That is, should changing the [AssemblyVersion] be considered a breaking change?

@jonorossi
Copy link
Member

I'd be interested in how you're going to fix this for Castle.Core: Will you reset the assembly major version to 4 or 5? That is, should changing the [AssemblyVersion] be considered a breaking change?

It won't be a major version. Looking at how we handled AssemblyVersion in the 2.x and 3.x series (x.x.0.0) the only release that is broken is 4.1.1 which should have been 4.1.0. We'll definitely release 4.1.2 (with 4.0.0 assembly version) to fix that line, then consider 4.2.0 however both NuGet and Paket will pull in "patch" versions in their dependency resolution so 4.2.0 is probably unnecessary.

@jonorossi
Copy link
Member

@skovsende @stakx @fir3pho3nixx I've had a quick Google and play, and couldn't get MSBuild to do what I wanted with AssemblyVersionAttribute it keeps outputting with package version. Do you guys know what needs to be set to get MSBuild to do this?

@stakx
Copy link
Member

stakx commented Jul 25, 2017

@jonorossi - This line in Microsoft.NET.GenerateAssemblyInfo.targets suggests that it might be possible to add a <GenerateAssemblyVersionAttribute>false</GenerateAssemblyVersionAttribute> to the .csproj, then define that attribute manually. I haven't tried it myself, though.

(If that doesn't work, then try the much coarser-grained <GenerateAssemblyInfo>false</GenerateAssemblyInfo>. I've used this before and it works, but it means you might then have to keep certain attributes in Castle.Core.csproj and AssemblyInfo.cs in sync manually.)

@jonorossi
Copy link
Member

Thanks @stakx, yer I saw those however didn't try them because our build automatically gets its release version from the git tag name we don't manually maintain an AssemblyInfo like file anymore, our git repo has 0.0.0 as the version.

There are a bunch of other documented MSBuild elements you are meant to be able to define to override the AssemblyVersion, but none seemed to work when I hardcoded a version.

@ghost
Copy link

ghost commented Aug 1, 2017

Ping back from MS:

You can set the properties AssemblyVersion (=> AssemblyVersionAttribute) and FileVersion (=> AssemblyFileVersionAttribute) in the project file to control the value of each generated attribute.

So a sample project file could contain for example:

<PropertyGroup>
  <!-- Set VersionPrefix instead of Version to allow a suffix to be added by CI builds -->
  <VersionPrefix>1.2.3</VersionPrefix>
  <AssemblyVersion>1.2.0</AssemblyVersion>
  <FileVersion>$(VersionPrefix)</FileVersion>
</PropertyGroup>

@jonorossi
Copy link
Member

Thanks @fir3pho3nixx, I tried exactly that, <AssemblyVersion> but it didn't work, maybe I had it in the wrong place or our common.props is getting in the way. Will have to try again.

@dasMulli
Copy link

dasMulli commented Aug 8, 2017

@jonorossi if you still have problems with it I can have a look (just tell me a branch/fork), I think I have a fairly good understanding of how that logic works. There also was a nasty issue with incremental builds that is fixed in the upcoming 2.0.0 tooling.

@ghost
Copy link

ghost commented Aug 8, 2017

@dasMulli thanks for offering. I think I have got it to work on this branch. I hardcoded the properties above in the common.props file here.

I can now see:

image

and

image

@jonorossi
Copy link
Member

Fixed by #298.

@tillig
Copy link

tillig commented Feb 11, 2018

Making this change in the mid-4.x system appears to be pretty breaking. Folks running prior to 4.2 when this came out are taking an upgrade to the package but getting a lower assembly version, which really hoses things up, especially in .NET Core projects where there's no app.config to manually enforce a binding redirect down. I understand the reason for the change, even if I don't agree with it; but not starting this with 5.0 is painful, like when log4net switched their signing key and broke all binding redirects.

@stakx
Copy link
Member

stakx commented Feb 11, 2018

@tillig:

especially in .NET Core projects where there's no app.config to manually enforce a binding redirect down

I was under the impression that .NET Core has a far more lenient assembly version matching algorithm than .NET, so if there are problems at all, I would have expected them on the full .NET Framework (with its GAC and strict versioning required by it) rather than on .NET Core.

Ever since devlooped/moq#424 (comment) (where I anticipated the kind of problem you're talking about, but didn't know whether anything would actually materialize) I have wondered under what precise circumstances an assembly version backstep with a simultaneous NuGet version increment (such as happened with Castle Core) will cause problems with the .NET tooling. Perhaps you have a concrete example that could shed some light on this?

PS: We can't change the past so this is perhaps mostly academic by now; I'm asking out of curiosity & to understand .NET tooling a little better.

@tillig
Copy link

tillig commented Feb 11, 2018

autofac/Autofac.Extras.DynamicProxy#25 shows an example of where this has affected downstream consumers negatively. I don't think folks would have noticed if the pointed version was greater than the previously released version, but back-versioning has hosed downstream consumers.

@dasMulli
Copy link

back-versioning is dangerous and should never be done – this is what caused most of the problems of .NET Standard on .NET Frameork compatibility issues (hence the loads of dll files + binding redirects if you reference a netstndard1.6 dll from a net461 project).
.NET Core does have a more relaxed loading system but that doesn't mean there won't be issues in tooling, build system ("ResolveAssemblyReferences" msbuild task), custom AssemblyLoadContext logic etc.

@jonorossi
Copy link
Member

tl;dr: please try a binding redirect with a valid assembly version.

I'm not going to debate what is right or wrong and who should never do what, passive aggressiveness isn't constructive.

The fact is 4.1.0 and 4.1.1 were "broken" for .NET Framework users nearly always requiring binding redirects, your linked issue (autofac/Autofac.Extras.DynamicProxy#25) doesn't mention what runtime it is using so I'll assume it is using the .NET Framework as we haven't experienced this with .NET Core's relaxed assembly binding. If that's the case, then please try a valid binding redirect using the actual assembly version (4.0.0.0 not 4.2.1.0) rather than throwing your hands up in the air and calling it a nightmare.

You could argue every release we do is a major version (because of behaviour changes) and that 4.1.0 and 4.1.1 should have been 5.0.0 and 6.0.0 (because they each had different assembly versions). This problem definitely isn't as bad as log4net changing strong name keys as that prevented binding redirects completely, they also had breaking APIs changes in 1.2.11.

We did the best we could do with the information we had at the time. In hindsight, we probably should have bumped the assembly version to 4.2.0.0 and kept it there until the end of the 4.x line.

This issue was open from mid-July to a release at the end of September, followed by notification in GitHub issue trackers of the 3 main mocking libraries (devlooped/moq#461, FakeItEasy/FakeItEasy#1208, nsubstitute/NSubstitute#317). We even had a discussion with Microsoft staff on the CoreCLR issue tracker (https://github.com/dotnet/coreclr/issues/14263). You were more than welcome to contribute your knowledge/opinions/concerns on this issue at that time. We thought it was well enough circulated, obviously Microsoft has also realised recently they've made versioning mistakes and that binding redirects are still a sore point for .NET generally.

@tillig
Copy link

tillig commented Feb 12, 2018

The challenge in .NET core is when you have a transient link to a 4.1.0 Castle.Core and a direct reference to a 4.2.0. The direct reference has a higher package version (which is how the reference gets added in the project) but a lower assembly version. The project system sees the project trying to force upgrade and downgrade at the same time, basically, and it breaks.

The reason I like it to log4net is that it was a breaking change that forced a lot of change without any workaround. That's the situation today with .NET Core as noted. Without binding redirects, the app dev is hosed. That seems pretty nightmare to me.

I'm not sure it's fair to expect everyone to track every issue in every repo for every direct and transient dependency they might use. Saying it was sitting here three months doesn't change that.

In hindsight, we probably should have bumped the assembly version to 4.2.0.0 and kept it there until the end of the 4.x line.

Agreed. Technically you could still do that and call the 4.2.x releases the broken ones. It would solve some problems for the .NET core folks and still allow you to keep the slow assembly versioning you're looking for.

@jonorossi
Copy link
Member

The challenge in .NET core is when you have a transient link to a 4.1.0 Castle.Core and a direct reference to a 4.2.0. The direct reference has a higher package version (which is how the reference gets added in the project) but a lower assembly version. The project system sees the project trying to force upgrade and downgrade at the same time, basically, and it breaks.

I am still missing how this breaks on .NET Core? Doesn't the loader ignore the assembly version as everything is versioned in packages and there is no GAC? If you have a repro could you create a new issue with those details. This is the first time someone has reported this, so it is news to me.

The reason I like it to log4net is that it was a breaking change that forced a lot of change without any workaround. That's the situation today with .NET Core as noted. Without binding redirects, the app dev is hosed. That seems pretty nightmare to me.

So as you said there is a workaround? I dislike it, but Microsoft has literally been telling people for the last 12 months bindings redirects are the answer to assembly version problems. I considered the option of bumping the assembly version to 5, but that would mean a binding redirects anyway or everyone upgrading immediately.

I'm not sure it's fair to expect everyone to track every issue in every repo for every direct and transient dependency they might use. Saying it was sitting here three months doesn't change that.

This was in response to my read of @dasMulli's comment as being passive aggressive, and high and mighty that we don't know what we are doing, yet while Microsoft was just starting to realise the same problems in September for the core of .NET, the BCLs. We didn't want this problem, we juggled the pros and cons of various options at the time, obviously more people are aware of these problems today. Words don't convey tone very well, so if that was unintentional the comment could have been more useful.

Agreed. Technically you could still do that and call the 4.2.x releases the broken ones. It would solve some problems for the .NET core folks and still allow you to keep the slow assembly versioning you're looking for.

One problem with changing the assembly version again is that we'll introduce breaking changes for .NET Framework users since all 3 major mocking libraries have already upgraded to 4.2.0+ so there is no need for binding redirects on .NET Framework, bumping to 4.3.0 (or something) would then require them again; the same for Castle Windsor 4.x as it has always been built against Castle Core assembly version 4.0.0, it never got released built against 4.1.0 or 4.1.1, only 4.0.0 and 4.2.0 (on purpose #288 (comment)).

@tillig
Copy link

tillig commented Feb 12, 2018

For reference, here's a small repro showing the issue in action.

ConsoleApp1.zip

Right now, with the Castle.Core as a transient reference, it works. If you uncomment the direct reference in the .csproj, you get a FileNotFoundException looking for Castle.Core 4.1.0.0, which is the transient reference. The app.config in there is totally ignored, so binding redirects aren't an option. Given the official way to require an upgrade to a transient package is to add a direct reference (eg, when a security patch is released in ASP.NET Core, you have to add direct references to the patched transient to force an upgrade), this may not be as uncommon as you might think. Folks needing or wanting the updated Castle.Core will take the update... and find themselves in this situation.

I don't have a problem updating Autofac.Extras.DynamicProxy to Castle.Core 4.2.x, but that only fixes it in this one instance. It's possible you'll see similar issues from other packages in the future, so at least now you are aware of the pain point and that it's really not fixable at the client app end. If, for example, someone has a fixed version of Autofac.Extras.DynamicProxy and Moq 4.7.127 in the same project, there's still going to be weirdness if the project has a direct C.C ref. There's no workaround if the client app needs the direct reference to Castle.Core.

Again, sorry if this seems argumentative. It's not intended to be, but this is definitely a pain point for which there really isn't a great fix from any angle other than pinning Castle.Core at 4.2.0.0 or higher... which, as you mentioned, will generate pain elsewhere.

Over on Autofac we've definitely seen some of the pain folks have in figuring out binding redirects, so I understand the challenge. Given NuGet has emerged to handle/automate most of that in recent years it's been far less of an issue, though I understand it's non-zero. It is nice to hear that they're looking at improving this for the full .NET Framework so I'm curious to see how that pans out.

@dasMulli
Copy link

This was in response to my read of @dasMulli's comment as being passive aggressive

I am deeply sorry about that, it absolutely was not my intention. I just had to deal with similar issues previously and did not take the time to write a proper response that underlined the importance of the issue in a proper tone. (I'm not a native English speaker but that is no excuse)

@ghost
Copy link

ghost commented Feb 12, 2018

@tillig

I am also partly responsible for the change, sorry this happened. I tried to let everyone know but I forgot about AutoFac.

Interestingly, you can code a binding redirect in the example you posted which does work. Please see revised code below:

class Program
{
	static void Main(string[] args)
	{
		RedirectAssembly("Castle.Core", new Version("4.0.0.0"), "407dd0808d44fbdc");

		try
		{
			var builder = new ContainerBuilder();
			builder.RegisterType<Service>()
				.As<IService>()
				.EnableInterfaceInterceptors()
				.InterceptedBy(typeof(CallLogger));
			builder.RegisterType<CallLogger>();
			builder.RegisterInstance(Console.Out)
				.As<TextWriter>();

			var container = builder.Build();

			var service = container.Resolve<IService>();
			service.DoWork();
		}
		catch (Exception ex)
		{
			Console.WriteLine(ex);
		}

		if (Debugger.IsAttached)
		{
			Console.WriteLine("Press any key to exit.");
			Console.ReadKey();
		}
	}

	public static void RedirectAssembly(string shortName, Version targetVersion, string publicKeyToken)
	{
		ResolveEventHandler handler = null;

		handler = (sender, args) => {
			// Use latest strong name & version when trying to load SDK assemblies
			var requestedAssembly = new AssemblyName(args.Name);
			if (requestedAssembly.Name != shortName)
				return null;

			Debug.WriteLine("Redirecting assembly load of " + args.Name
							+ ",\tloaded by " + (args.RequestingAssembly == null ? "(unknown)" : args.RequestingAssembly.FullName));

			requestedAssembly.Version = targetVersion;
			requestedAssembly.SetPublicKeyToken(new AssemblyName("x, PublicKeyToken=" + publicKeyToken).GetPublicKeyToken());
			requestedAssembly.CultureInfo = CultureInfo.InvariantCulture;

			AppDomain.CurrentDomain.AssemblyResolve -= handler;

			return Assembly.Load(requestedAssembly);
		};
		AppDomain.CurrentDomain.AssemblyResolve += handler;
	}
}

Looks like the MSBuild has difficulty in generating the assembly redirect for Castle Core when using the following properties:

    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <GenerateBindingRedirectsOutputType>true</GenerateBindingRedirectsOutputType>

I even tried brute forcing it by renaming the App.config to ConsoleApp1.dll.config and copying it to the output directory but nothing was taking hold.

Everything not allowing the work around appears to be pointing to the SDK IMHO.

@tillig
Copy link

tillig commented Feb 12, 2018

Coding a binding redirect is academically interesting but not terribly practical. If XML binding redirects are too hard in .NET full framework, this is a few steps beyond that. I guess it's interesting to see, though.

Especially with the advent of .NET Core, package version == assembly version has somewhat been an assumption. The concept of the binding redirect hasn't entered the picture since it's been "smart enough" to figure it out... based on that assumption. This slow-assembly-version-change thing would have been seamless, I think, had it not been for the back-version of the assembly to 4.0.0.0. But it is what it is, and I can at least alleviate some of the pain by forcing the Autofac.Extras.DynamicProxy reference forward and adding some docs on our end pointing folks back to this issue for more detail.

@ghost
Copy link

ghost commented Feb 12, 2018

Coding a binding redirect is academically interesting but not terribly practical.

Yup, you are right this is not an answer. You are worse off than with the XML config :)

I guess it's interesting to see, though.

Very much so. It proves that netcore can bind up or down(unfortunate you have to drop code) but good to know it is there.

@ghost ghost reopened this Feb 12, 2018
@ghost ghost closed this as completed Feb 12, 2018
@jonorossi
Copy link
Member

Again, sorry if this seems argumentative. It's not intended to be, but this is definitely a pain point for which there really isn't a great fix from any angle other than pinning Castle.Core at 4.2.0.0 or higher... which, as you mentioned, will generate pain elsewhere.

@tillig not at all, apologies if I came across a bit annoyed, I'm so tired of binding problems with .NET over the last 10 years and I probably should have been in bed, not replying to issues 😕 .

I knew .NET Core didn't support binding redirects or app.config, but I wrongly assumed that it ignores the assembly version (can't find any docs, can anyone help with a link?) based on only testing it with a newer version but not the other way, however the coreclr code appears to only load higher versions (https://github.com/dotnet/coreclr/blob/282f350b0c78c858ddef9c3c4cdcf0ea68a97e72/src/binder/assemblybinder.cpp#L83-L162). I don't know why Microsoft would do this, what value would you get by even bothering with the assembly version, not sure why they didn't just stop using the attribute completely as it is pretty useless information as the package version is what the tooling cares about, I guess it is still needed for .NET Framework and the new project templates define it so people continue doing what they used to, the coreclr runtime should at least ignore it though.

I am deeply sorry about that, it absolutely was not my intention. I just had to deal with similar issues previously and did not take the time to write a proper response that underlined the importance of the issue in a proper tone. (I'm not a native English speaker but that is no excuse)

@dasMulli no hard feelings, thanks for participating. If you know of any way out of this then I'd be interested to hear it.

Interestingly, you can code a binding redirect in the example you posted which does work. Please see revised code below

@fir3pho3nixx interesting to know, I think this proves again that .NET's assembly identity is mostly obsolete today and that they shouldn't be using assembly version at all. The whole open source code signing stuff for .NET Core also proves that strong naming should be dead. The code in AssemblyBinder.cpp indicates if you don't define a requesting assembly version it'll load the one you've got too.

With hindsight, since Microsoft had to update the .NET Framework to support .NET Standard anyway, it would have been great if they'd created a flag in the CLI header that said this is a "v2" assembly that doesn't have all the crudy strong names and assembly versions and both runtimes could happily use these new assemblies, and it wouldn't break back compat on .NET Framework.

Especially with the advent of .NET Core, package version == assembly version has somewhat been an assumption.

@tillig except the BCLs for netfx compat 😉

The concept of the binding redirect hasn't entered the picture since it's been "smart enough" to figure it out... based on that assumption. This slow-assembly-version-change thing would have been seamless, I think, had it not been for the back-version of the assembly to 4.0.0.0. But it is what it is, and I can at least alleviate some of the pain by forcing the Autofac.Extras.DynamicProxy reference forward and adding some docs on our end pointing folks back to this issue for more detail.

@tillig yes unfortunately from the info I now know about the coreclr binder things would have worked on .NET Core if we hadn't back versioned, and .NET Framework would have been all good (without needing binding redirects) once consumers rebuilt against the newly assembly versioned binary. Thanks for releasing Autofac.Extras.DynamicProxy v4.4.0, I too don't see any way around this now.

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

No branches or pull requests

5 participants