fsprojects / Paket

A dependency manager for .NET with support for NuGet packages and Git repositories.
https://fsprojects.github.io/Paket/
MIT License
1.99k stars 520 forks source link

2nd/3rd Level Dependency (fsharp.core) #2560

Open BlythMeister opened 6 years ago

BlythMeister commented 6 years ago

I'm not sure if this is the right place for this, but seeking help/advice here 1st 🙂

I have a weird issue with fsharp.core/Paket/msbuild, I don't know where in the chain the issue is though.

I consume https://www.nuget.org/packages/Sproc.Lock/ which needs fsharp.core 4.0.0.1 or higher in a repo with latest fsharp.core (4.2.1).

Assembly a takes reference (in dependencies) to sproc lock (but not fsharp) and Paket puts binding redirect and uses correct version of fsharp in the bin according to Paket.lock. Great so far!

Assembly b project references assembly a, it does not have sproc.lock or fsharp.core in the dependencies file. This also gets the binding redirect (through a second level dependency). Again, so far so good.

However, when built you get the 4.0.0.1 version of fsharp in the bin. This is located from the machine install of fsharp, rather than taking the version out of assembly a.

I can "fix" this by adding fsharp.core (or sproc.lock) to assembly b dependencies file, but where does that stop...if assembly c takes reference to assembly b, it also needs these references?!?

I'm essentially looking for advice or clarification if this is a big or expected behaviour. I can try and setup an example public repo to show this behaviour of that helps.

matthid commented 6 years ago

I think this is a msbuild limitation. I've seen similar behavior when using nuget and referencing projects, at some level msbuild stops copying dependencies to the output and you need to add a nuget package manually to your startup project ...

BlythMeister commented 6 years ago

Msbuild does check the bin of assembly a (seen on a verbose build) but because of the version conflict it does not copy it.

Could this be added to Paket to auto add the dependency when adding a binding redirect? Paket is clever enough to know there is going to be an issue at runtime in assembly b, but no direct reference added, and we know msbuild will not use assembly a version making the binding redirect irrelevant as at runtime it has the wrong version anyways.

matthid commented 6 years ago

Yes maybe paket should consider solving this. Does it happen in the new SDK? It might not make a lot of sense to fix it for old-style projects.

BlythMeister commented 6 years ago

I'm using net45 and a recent version of Paket.

I'm happy to investigate if I can add this feature...I may need guidance as to where this area of code is though 👍

matthid commented 6 years ago

Can you zip a repro or show how a repro looks like (also specify if this is about project references or if they are not involved at all), because we might not speak of the same issue.

ideally show contents of reference/dependencies files.

BlythMeister commented 6 years ago

Sure, I'll make a mock-up of my situation complete with Paket files etc to show the issue I'm having (unfortunately, the actual codebase is closed source and quite verbose) A simplified example will probably help me understand too 😋

matthid commented 6 years ago

Thanks

BlythMeister commented 6 years ago

@matthid I have pushed a public repo with the situation we have in small scale https://github.com/BlythMeister/DependencyHell

I've added a read-me with details of the setup and the problem, let me know if you need any more info.

BlythMeister commented 6 years ago

Added a couple more branches with a workaround (adding reference in AssemblyB to FSharp.Core) and a "quirk" (adding something else which needs a different fsharp.core to AssemblyA) Both these actually result in the right output on AssemblyB.

Whilst i actually think MSBuild is being pretty dumb here (and maybe that should be fixed) we could make paket (once again) mop up for the natvie tooling ;)

matthid commented 6 years ago

I have seen the same time and time again. Basically I always try to reference everything again in all test and executable projects. And it was like this ever since.

Note there is no need to reference everything recursively when the project is a class library as this is only required for runtime. Therefore you need to figure out what the project type is and which other projects are referenced to apply the "missing" references. This is possible, question is: Does the new SDK "fix" this? Should we really get into this NOW, considering it has been like this ever since?

BlythMeister commented 6 years ago

well....it does copy the transient dependency...but the wrong version. What do you mean by new SDK? .net standard?

matthid commented 6 years ago

I mean the new fsproj/csproj format. It will allow you to compile for older frameworks with the dotnet cli tooling afaik.

BlythMeister commented 6 years ago

i wish i could just update to use new tooling like that :(

I think this is something different though, since i do get the FSharp.Core.dll in AssemblyB. I just get a machine installed/GAC registered version which better matches rather than the one i have told paket i want.

BlythMeister commented 6 years ago

@matthid can I assume by your reply this automatic 2nd level dependency adding you state you do yourself isn't something we could automate with Paket?

I think it would be a really useful feature for users like myself who unwittingly get a runtime error as I demonstrated in my mock repo.

Essentially, a project dependency should be treated as though it has all the nuget dependencies of the project it depends on. In my mock repo, this means AssemblyB would be treated as though it had Sproc.Lock in its references file.

matthid commented 6 years ago

No , what I'm saying is that we could do it. I'm just asking if it is worth the effort right now, considering a new project format is in front of the doors...

Also we probably would accept a PR, though we might need to discuss if this is enabled always or via some flag only

Hope that clarifies my position on this.