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

Fix dependency specification of logging packages to the exact same explicit version of Castle.Core package #292

Closed
DianaLaa opened this issue Aug 5, 2017 · 30 comments
Labels
Milestone

Comments

@DianaLaa
Copy link

DianaLaa commented Aug 5, 2017

Hi all,

I had some trouble getting Castle NLog integration to work.

I'm using:

Castle.Core v4.1.1
Castle.Core-NLog v3.3.3
Castle.LoggingFacility v4.0.0
Castle.Windsor v4.0.0
Castle.Windsor-NLog v3.4.0
NLog v4.4.1.1
NLog.Config v4.4.1.1
NLog.Schema v4.4.1.1

Now, when I setup a simple installer:

public class Installer : IWindsorInstaller
{
    public void Install(IWindsorContainer container, IConfigurationStore store)
    {
        // Install logging facility
        container.AddFacility<LoggingFacility>(f => f.LogUsing(LoggerImplementation.NLog)
                                                     .WithConfig("NLog.config"));
    }
}

and write a straightforward test

[TestClass]
public class InstallerTests
{
    protected WindsorContainer Container;
    
    [TestInitialize]
    public void Initialize()
    {
        Container = new WindsorContainer();
    }

    [TestMethod]
    public void Install_ShouldInstallLogger()
    {
        // Arrange
        Container.Install(FromAssembly.Containing<Installer>());

        // Act
        ILogger result = Container.Resolve<ILogger>();

        // Assert
        Assert.IsNotNull(result);
    }
}

I got the error:

Could not convert string 'Castle.Services.Logging.NLogIntegration.NLogFactory,Castle.Services.Logging.NLogIntegration,Version=4.0.0.0, Culture=neutral,PublicKeyToken=407dd0808d44fbdc' to a type. ---> System.IO.FileLoadException: Could not load file or assembly 'Castle.Services.Logging.NLogIntegration, Version=4.0.0.0, Culture=neutral, PublicKeyToken=407dd0808d44fbdc' or one of its dependencies. The located assembly's manifest definition does not match the assembly reference.

In order to resolve this, I had to add in app.config:

  <dependentAssembly>
    <assemblyIdentity name="Castle.Services.Logging.NLogIntegration" publicKeyToken="407dd0808d44fbdc" culture="neutral" />
    <bindingRedirect oldVersion="0.0.0.0-4.0.0.0" newVersion="3.3.0.0" />
  </dependentAssembly>

What is causing this behavior? How can I resolve it in a better way? Or is this the right way?

@jonorossi
Copy link
Member

jonorossi commented Aug 5, 2017

You should not mix different versions of Castle Core's packages, i.e. if you use Castle.Core 4.1.1 you should also use Castle.Core-NLog 4.1.1. The same applies to Windsor facilities and those logging dependency packages.

It unfortunately wasn't mentioned in the Windsor 4.0.0 release notes but we dropped those logging dependency packages (Castle.Windsor-log4net and Castle.Windsor-NLog) because they were literally just an empty package that specified a dependency because they are unnecessary.

Are you following a tutorial somewhere that we need to get updated?

@jonorossi
Copy link
Member

I'm not sure how to get the new .NET CLI MSBuild-based tools to create an output package with the dependency version set as requiring an exact version not a range.

Currently Castle.LoggingFacility 4.0.0 defines a dependency Castle.Windsor >= 4.0.0 which is not desirable as mentioned above.

The project file uses a project reference so not sure how to override the output dependency range:
https://github.com/castleproject/Windsor/blob/v4.0.0/src/Castle.Facilities.Logging/Castle.Facilities.Logging.csproj#L21

@DianaLaa
Copy link
Author

DianaLaa commented Aug 5, 2017

jonorossi,

Thanks for your reply.
I removed the dependency on Castle.WIndsor-NLog and updated all Castle assemblies to 4.0.0.0

Castle.Core v4.0.0
Castle.Core-NLog v4.0.0
Castle.LoggingFacility v4.0.0
Castle.Windsor v4.0.0
Castle.Windsor-NLog REMOVED
NLog v4.4.1.1
NLog.Config v4.4.1.1
NLog.Schema v4.4.1.1

I have removed the redirects in my app.config an it works like a charm. Thanks

P.S. For some reason, I hadn't seen Castle.Core-NLog 4.0.0, which added to my confusion. Furthermore, I'm using Moq, which was asking for Castle 4.1.1.0. I've now downgraded to Moq v4.7.25, the last version that works with Castle 4.0.0.0. Hope this helps someone in the future :)

@stakx
Copy link
Member

stakx commented Aug 5, 2017

I'm not sure how to get the new .NET CLI MSBuild-based tools to create an output package with the dependency version set as requiring an exact version not a range.

Currently Castle.LoggingFacility 4.0.0 defines a dependency Castle.Windsor >= 4.0.0 which is not desirable as mentioned above.

The project file uses a project reference so not sure how to override the output dependency range[.]

Is there a specific reason why a project reference is used? Given that Castle.LoggingFacility and Castle.Windsor end up being separately distributed artifacts on NuGet, I would expect that the former depends on the latter via a package reference. I understand project references to be references to an "internal" dependency that will be included in the distribution, while package references are for "external" (i.e. separate) dependencies.

One solution might be to turn the <ProjectReference> into a <PackageReference>, which allows you to specify a version. Then put the exact required version in square brackets as explained in Specifying dependency versions for NuGet Packages: Version ranges.

You might end up with something like this:

<PackageReference Include="Castle.Windsor" Version="[4.0.0]" />

@jonorossi
Copy link
Member

I'm using Moq, which was asking for Castle 4.1.1.0. I've now downgraded to Moq v4.7.25, the last version that works with Castle 4.0.0.0

@DianaKoenraadt Castle Core and Castle Windsor are separate projects, their versions don't have to match. You can upgrade Castle Core to 4.1.1, Castle Windsor's most recent release is 4.0.0 which supports Castle Core 4.1.1.

@jonorossi
Copy link
Member

Is there a specific reason why a project reference is used? Given that Castle.LoggingFacility and Castle.Windsor end up being separately distributed artifacts on NuGet, I would expect that the former depends on the latter via a package reference. I understand project references to be references to an "internal" dependency that will be included in the distribution, while package references are for "external" (i.e. separate) dependencies.

@stakx good question. We've always used project references for projects in the same Git repo because tooling always stuffed up without doing that, e.g. MSBuild wouldn't detect the dependency had changed when you rebuilt or ran unit tests.

I don't know how the tooling works for PackageReference but I assume it would try to resolve the dependency within the same solution first before going off to a remote repository?

If we did make this change we'd have to use the MSBuild property we've got for the version: https://github.com/castleproject/Core/blob/v4.1.1/buildscripts/common.props#L14.

It looks like others are using project references and also trying to lock the version, there is a workaround in one of the following issues but completely ugly:

With the new MSBuild tooling can you even pack two assemblies into the same package? When should project references be used, only for applications where you don't pack but publish?

@ghost
Copy link

ghost commented Aug 7, 2017

Lol - So it is not just me then. I had to write this to make things work for fluentwindsor.

Would you guys like me to raise a PR that does this with a custom MsBuild task for now?

*** Edited ***

Failing that we can wait for v2.0 of netcore

@ghost
Copy link

ghost commented Aug 7, 2017

We could revert back to packing using nuspec files. The custom MsBuild task would then consistently include explicit versions as pre-build step honoring the BuildVersion property. This would mean taking a step back from where we are when it comes to NuGet packing and versioning and using the SDK(or the dotnet cli for that matter).

The release for dotnet core 2.0 appears to be very close though, anyone played with version suffixes yet?

@jonorossi
Copy link
Member

Failing that we can wait for v2.0 of netcore

Are you saying this is fixed with the .NET CLI 2.0 tooling?

@ghost
Copy link

ghost commented Aug 9, 2017

@jonorossi - I am currently investigating this, will ping back if I hit a positive result.

@jonorossi jonorossi changed the title Have to redirect Castle.Services.Logging.NLogIntegration from 4.0.0.0 to 3.3.0.0? Fix dependency specification of logging packages to the exact same explicit version of Castle.Core package Aug 22, 2017
@jonorossi jonorossi added the bug label Aug 22, 2017
@jonorossi jonorossi added this to the vNext milestone Aug 22, 2017
@jonorossi
Copy link
Member

@fir3pho3nixx did you get a chance to look at this one?

It sounds like project references are going to deprecated, and we should be moving to package references anyway:
dotnet/announcements#31

@ghost
Copy link

ghost commented Sep 13, 2017

@jonorossi - this is all starting to come together nicely.

We had some uglyness in that crept up with logging down in Windsor here which might have been the root cause of this issue. We have hard coded assembly references in the LoggingFacility which means no matter what every new Core release will break Castle.Facilities.Logging on NuGet when people upgrade.

My fix is nuclear, it requires a breaking change to facilities logging in windsor. I have a PR ready here

It did however lead me to the conclusion that all that facility stuff should live in a separate repository anyway and we should use Castle.Windsor in the facilities repo as a PackageReference. I asked you for the new repo here. This will naturally start moving us in the right direction.

Thoughts?

@jonorossi
Copy link
Member

@fir3pho3nixx you've mentioned a whole bunch of stuff which I've responded to in their own issues.

The logging facility has been that way for over a decade, the only reason things are breaking now is that the new build scripts were doing the wrong thing with the assembly version which we've now fixed. I still think we want our packages with dependencies in lockstep, if you are using the Castle.Core and Castle.Core-log4net packages then they need to be the exact same version and there is literally no reason you shouldn't, at the moment NuGet is allowing Castle.Core 4.0.0 to be used with Castle.Core-log4net 3.3.3 because Castle.Core-log4net just says it needs 3.3.3 and 4.0.0 meets that requirement.

@ghost
Copy link

ghost commented Sep 13, 2017

@fir3pho3nixx you've mentioned a whole bunch of stuff which I've responded to in their own issues.

Spent the day past 2 days working on Castle Windsor. Good innit?

The logging facility has been that way for over a decade, the only reason things are breaking now is that the new build scripts were doing the wrong thing with the assembly version which we've now fixed.

Yes, build scripts, my fault, sorry everyone. Feel free to send hate mail to fir3pho3nixx@gmail.com. However before we go off half cocked blaming my work let's consider my conclusion below.

I still think we want our packages with dependencies in lockstep, if you are using the Castle.Core and Castle.Core-log4net packages then they need to be the exact same version and there is literally no reason you shouldn't, at the moment NuGet is allowing Castle.Core 4.0.0 to be used with Castle.Core-log4net 3.3.3 because Castle.Core-log4net just says it needs 3.3.3 and 4.0.0 meets that requirement.

Lock step perhaps, not my call (I would do it differently).

Conclusion

What I eventually saw here was Castle.LoggingFacility v4.0.0 had a hardcoded reference:

private static readonly String ExtendedNLogLoggerFactoryTypeName =
			"Castle.Services.Logging.NLogIntegration.ExtendedNLogFactory," +
			"Castle.Services.Logging.NLogIntegration,Version=4.0.0.0, Culture=neutral," +
			"PublicKeyToken=407dd0808d44fbdc";

and

private static readonly String NLogLoggerFactoryTypeName =
			"Castle.Services.Logging.NLogIntegration.NLogFactory," +
			"Castle.Services.Logging.NLogIntegration,Version=4.0.0.0, Culture=neutral," +
			"PublicKeyToken=407dd0808d44fbdc";

in the LoggingFacility for Castle.Facilities.Logging. This eventually runs the NLogFactory and ExtendedNLogFactory through an ITypeConverter implementation here. This resolves through the MicroKernel subsystem to the TypeNameConverter and throws an exception which in a spooky way matches the problem description above. Case closed. Not the build scripts fault.

@tasoss
Copy link

tasoss commented Sep 14, 2017

Hello.I'm having the same problem.
I'm using v4.0.0 of castle.core castle.facilities.logging and castle.windsor.
When this is called
container.AddFacility<LoggingFacility>(f => f.LogUsing(LoggerImplementation.NLog));
i get

Castle.MicroKernel.SubSystems.Conversion.ConverterException: 'Could not convert string 'Castle.Services.Logging.NLogIntegration.NLogFactory,Castle.Services.Logging.NLogIntegration,Version=4.0.0.0, Culture=neutral,PublicKeyToken=407dd0808d44fbdc' to a type. Assembly was not found. Make sure it was deployed and the name was not mistyped.'
I haven't found any other library that references 4.1.1

Any ideas on how i can fix this?
Thanks!

@ghost
Copy link

ghost commented Sep 14, 2017

Try this in your config:

<runtime>
<assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
  <dependentAssembly>
	<assemblyIdentity name="Castle.Core" publicKeyToken="407dd0808d44fbdc" culture="neutral" />
	<bindingRedirect oldVersion="0.0.0.0-4.1.1.0" newVersion="4.1.1.0" />
  </dependentAssembly>
</assemblyBinding>
</runtime>

@tasoss
Copy link

tasoss commented Sep 14, 2017

I don't know what i'm doing wrong but i get the same error.
Would you like me to share the project with you?
Thanks!

@ghost
Copy link

ghost commented Sep 14, 2017

Sure ...

@tasoss
Copy link

tasoss commented Sep 14, 2017

I have emailed it to you.Thanks...

@ghost
Copy link

ghost commented Sep 14, 2017

From what I can see, you got the assembly binding correct. You were missing a NuGet reference to NLog in packages.config.

<package id="Castle.Core-NLog" version="4.1.1" targetFramework="net461" />

@tasoss
Copy link

tasoss commented Sep 14, 2017

Thank you very much for your help :)

@tasoss
Copy link

tasoss commented Sep 14, 2017

I'm very sorry but the fixed project didn't work again.
This is what i got from fuslogvw.
Thanks again...

@ghost
Copy link

ghost commented Sep 15, 2017

Hello again.I fixed it like this.
Added
<dependentAssembly> <assemblyIdentity name="Castle.Services.Logging.NLogIntegration" publicKeyToken="407dd0808d44fbdc" culture="neutral" /> <bindingRedirect oldVersion="0.0.0.0-4.1.1.0" newVersion="4.1.1.0" /> </dependentAssembly>

The problem is
namespace Castle.Facilities.Logging and class LoggingFacility
private static readonly string NLogLoggerFactoryTypeName = "Castle.Services.Logging.NLogIntegration.NLogFactory,Castle.Services.Logging.NLogIntegration,Version=4.0.0.0, Culture=neutral,PublicKeyToken=407dd0808d44fbdc";

as @fir3pho3nixx had mentioned already.
Thanks again @fir3pho3nixx you are very kind!

ps:i have replied with an alternative account that i own...my mistake!

@jonorossi
Copy link
Member

@fir3pho3nixx No need for hate mail, when I said this:

The logging facility has been that way for over a decade, the only reason things are breaking now is that the new build scripts were doing the wrong thing with the assembly version which we've now fixed.

... I forgot to mention what was obvious to me, that the logging facility has always done annoying reflection to create the loggers using its list of loggers (LoggerImplementation enum) for loggers that Castle Core owns.

@tasoss @tasosss you'd be best served to use the NLogFactory class directly as the next version of Windsor will deprecate the LoggerImplementation enum, this will avoid the need to do any binding redirects.

Instead of:

container.AddFacility<LoggingFacility>(f => f.LogUsing(LoggerImplementation.NLog));

... use:

container.AddFacility<LoggingFacility>(f => f.LogUsing<NLogFactory>());

The NLogFactory class lives in the Castle.Core-NLog package which you already have.

See the new notes in the Windsor documentation that resulted from this issue's changes.

@jonorossi
Copy link
Member

Now that the underlying logging facility problem that also surfaced is solved (castleproject/Windsor#336) by deprecating LoggerImplementation and directly passing the facility the Castle Core class, we should circle back around to the Castle Core version range problem.

We need to block people being able to specify this:

  • Castle.Core v4.1.1
  • Castle.Core-NLog v3.3.3

The easiest way is to make Castle Core's packages depend on the one exact version of Castle Core it is versioned as, i.e. forces upgrades of Castle Core's packages in lockstep. I don't see any disadvantage to doing this.

@fir3pho3nixx you've voiced objection to doing this for which I don't understand, I'd still like to understand why, I'm obviously missing something, and I'd love to hear if you've got a better suggestion.

@tasoss
Copy link

tasoss commented Sep 17, 2017

@jonorossi ok thanks for the info!

@ghost
Copy link

ghost commented Sep 17, 2017

@jonorossi

We need to block people being able to specify this:

Castle.Core v4.1.1
Castle.Core-NLog v3.3.3

@fir3pho3nixx you've voiced objection to doing this for which I don't understand, I'd still like to understand why, I'm obviously missing something, and I'd love to hear if you've got a better suggestion.

Lock stepping versions of vendor specific logging abstractions with something as important as Castle.Core should not be happening IMHO. Castle.Core should be set free from it's NLog/log4net/serilog vendor/versioning shackles (or ProjectReferences as @stakx said).

Let's think about this for a second.

If Castle.Core-NLog v3.3.3 was left behind(separate deploy cycle/repository) and not lock step versioned with Castle.Core v4.1.1, none of this would have happened in the first place. The real problem is that we cannot patch/upgrade Castle.Core then release it without having to consider all of the dependents. We need to pull this apart and separate our concerns. If nobody contributes to it, we let it die a natural death. Good riddens to dependencies the community wont maintain. This let's us get on with the important stuff and not battle assembly binding issues.

Default NuGet behaviour is to always take latest of any. I think this is correct. We need to embrace this and start getting our releases/versioning/dependencies in order and start explicit versioning but we have to break things apart first.

We need a way of releasing Castle.Core without any baggage what so ever. It is consumed by vendor frameworks and Castle.Windsor alike. Castle.Windsor keeps Castle.Core honest in terms of how it is consumed and at the moment it is exposing problems with what I feel is lock stepped versioning.

I think the real question we need to be asking ourselves here is: How important are the logging abstractions when it comes to Castle.Core vs everything else?

To quote @stakx:

One solution might be to turn the <ProjectReference> into a <PackageReference>, which allows you to specify a version. Then put the exact required version in square brackets as explained in Specifying dependency versions for NuGet Packages: Version ranges.

You might end up with something like this:

<PackageReference Include="Castle.Windsor" Version="[4.0.0]" />

This to me means, separate repositories/release cycles with separate NuGet publications/versions. Thoughts?

@jonorossi
Copy link
Member

This to me means, separate repositories/release cycles with separate NuGet publications/versions.

As discussed PackageReference shouldn't require any of that otherwise you wouldn't be able to have more than one project in a solution. Let's try this, otherwise we can post-process the NuGet package to insert the square brackets to ensure explicit versioning.

We'll look at the future of the logging integration packages for v5.

@jonorossi
Copy link
Member

Fixed in #302.

@DianaLaa
Copy link
Author

A bit late to the party, but I wanted to let you all know that I really appreciate that my "I'm trying to do this, what am I doing wrong?"-issue was taken up in such a serious and constructive manner. Kudos

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

4 participants