Assembly redirection equivalent on netcore not working. #3408
Comments
|
Also, I should mention that I tried to find a workaround by using assembly redirects (which is how you would do it in the full framework). However in this particular case it seems to be impossible to workaround because "Suave" is not strongly named. So I have not found any workaround whatsoever. |
|
I can repro on <Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp2.0</TargetFramework>
</PropertyGroup>
<ItemGroup>
<Compile Include="Program.fs" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Suave">
<Version>2.1.1</Version>
</PackageReference>
<PackageReference Include="Suave.OAuth">
<Version>0.9.0-pre2</Version>
</PackageReference>
</ItemGroup>
</Project> |
|
@matthid I have taken a look at this, I need to talk to some folk first, but I think F# is doing the right thing, unless there is special handling for unsigned versioned assemblies. The SDK tells both compilers reference these two dll's: /reference:C:\Users\kevinr.nuget\packages\suave\2.1.1\lib\netstandard1.6\Suave.dll The Suave .dll has a version 0.0.0.0 : The Suave.oath references suave.dll version 2.0.0 Effectively the assembly load for Suave.dll v2.0.0.0 should fail. However ... your repro demonstrates that C# compiles and runs, and F# compiles and doesn't run so we sort of need to get to the bottom of that. Anyway long story short this will take more investigation, and is kind of neat and quirky. Kevin |
|
I already thought that would be the case. In that case some compiler changes are probably required. From your investigation (can you post the full compiler command lines?) my assumption is that C# compiler notices the different versions and takes the one from command line, while F# takes the latest one, which is wrong in this case because 0.0.0.0 is actually bewer than 2.0.0.0. It didn't even occur to me that 0.0.0.0 is the actual assembly version of the 2.1.1 assembly. Good catch. So a workaround might be a newer suave release with a higher assembly version. If that is indeed the case the issue is not as urgent as I thought |
|
Just one more note on this. There was a discussion on slack if this could be considered a runtime bug as well. And you can argue that the runtime should use the deps.json file for loading assemblies which contains the correct entries. So it might make sense to ask one of the runtime guys about this as well... |
|
@matthid The F# compiler unifies all assembly references to the highest observed assembly version. When it writes the reference it writes this unified highest observed reference. In the compiler types are not versioned, they match solely based on simple assembly names and type name. So ... we make a reference by getting the info from all assemblies passed in on the command line. Furthermore, we amend this set of versions, by importing all of the types from the referenced assemblies, including transitive dependencies. So from the command line we get 0,0,0,0 for the Suave reference, but from Suave.OAth we see 2.0.0.0. So we up raise our reference to 2.0.0.0 What I think is wrong here is that we don't give a build error saying I can't find assembly Suave,2.0.0.0. Although TBH If we don't use any Suave types via OAuth then the 0.0.0.0 reference would probably be okay. I'm honestly conflicted about this ... at least it fails early and during testing, In-fact, I wouldn't mind the compiler being more proactive, on the other hand erroring because of unused references might be too strict. |
|
@matthid no it is not a runtime bug. The coreclr will load an assembly of the version or higher than the reference. If the assembly discovered is lower than the reference then it fails. So the coreclr is doing the right thing. |
|
If I remember it correctly, the runtime behavior of CoreClr is that either the same, or a higher version can satisfy a reference. That would mean that 2.0 can satisfy 0.0, but 0.0 can't satisfy 2.0. Following, the behavior of the runtime would be correct, if the assembly on disk really has version 0.0.
I assume that step is missing from the C# compiler |
|
@0x53A The C# unification approach is slightly different, they seem to prefer to use the reference from the /r: assemblies rather than the transitive references from importing those types. Effectively they won't see Suave, 2.0.0.0. Although there is a question about what happens if you use an API from OAth that has types from Suave(2.0.0.0) on it's public surface. |
|
So this basically means you can never decrease an assemblyversion while increasing the nuget package version. /cc @haf because this happened in suave Let me think about this a bit and give another answer tomorrow. I'm also not sure I'm liking this and I'm a bit sceptical what this means for transitive deps in different versions. Tbh my first instinct is that nuget version should be honored, this is completely unexpected... |
|
@matthid What I think you mean to say is: "I think the reference should use the assembly version that matches the assembly of the file passed on the command line" I.e "do what the developer said". I think that is a fine position to take. However, In general, I would say this is a badly specified project, You have a dependence on an assembly that requires 2.0.0.0 of the dll and you provide 0.0.0.0 of the dll. If the author of OAuth had intended 0.0.0.0 to be the version he should have specified it. I honestly believe the error here is that the compiler doesn't issue an error rather than that the refs are wrong .... let's face it ... you didn't know the version number had gone away, that would normally indicate some sort of bug. |
|
The net40 version of the suave.dll has a version number 2.1.1.0 So I think this should be regarded as a nuget package authoring error. |
|
The problem is: This issue shows that we are not guaranteed to get the package manager resolution at runtime, which feels wrong. From a compilers perspective I agree (as it doesn't see packages) however when looking at it from a higher level I'm really really skeptical. We really need to test how it behaves when we only talk about transitives, where we cannot just "prefer" one or another in the compiler - we need the help of the runtime here and need a way to tell it which one to take. If we go down this road this "hard" requirement needs to be documented somewhere (ie. that a higher nuget package version may only contain a higher assembly-version dll). At least I never saw that requirement anywhere and sure would have expected that it would work. In fact when using full dotnet it probably would work - at least with assembly redirects and strongly named assemblies. |
|
I think : Here is the rationalization for unifying up, and it is a rationalization and therefore an opinion. Here is the rationalization for unifying to the ref passed on the command line list of references, and it too is a rationalization and therefore also an opinion. Here is the rationalization for erroring if the compiler needs to write a reference not in the list of assemblies on the command line list of references, and it too is a rationalization and therefore also an opinion. The problem with making a change to what we do know is it is a breaking change. There are some projects that work now, will not work in the future, and some that don't work ... such as your example will start working. The above is why I am conflicted, I can see arguments for all three approaches. Regardless of that ... the example you provided is clearly a bug, and I am glad the F# highlighted the bug for you I think C# being a bit sloppy there lead you to miss a clear bug in the Suave package, and also why in my heart I prefer the 3rd option. Kevin |
probably, yes. But I strongly assume that, with a slightly different setup, the C# compiler will have the same issues. In your example, the app started up. But as soon as suave tried to use anything from suave.odata, it would also blow up because it would try to load 2.0, which is greater than the already loaded 0.0. So in that regard, the behavior of fsc may be preferable, at least it fails earlier. I agree with kevin that in my opinion the best resolution would be to error out if a transitive dependency has a higher version than an explicit dependency. |
|
@0x53A I'm talking myself into adding an error when we detect this, so it will fail even earlier. Do you think that makes me evil? |
|
if anything, it would make you a saint. |
|
@0x53A I already added code from Suave.OData, what you probably mean is calling code interacting with the transitive Suave code (via Suave.OData), so that the runtime needs to load "Suave 2.0.0.0" in this case. Would be interesting to check if this is indeed the case.
What I'm saying is I never read or seen that anywhere, and I read a lot of NuGet docs because of Paket :/ I'm still on a different position. Take a step back. If you look at this globally than we have a resolution by the package manager via nuget-package versions. If yes, we need to figure out which components need to do what to ensure that. Clearly some important information is lost in the NuGet -> SDK -> Executing pipeline... Note that if you don't agree on this global statement than we have (IMHO) a really big problem and it doesn't really matter what we decide to do in the compiler for this particular case... So, I'm suggesting one of the following:
Sorry for the confusion. At least I no longer get a heart attack on this issue and clarified the severity a bit ;) |
|
@matthid Yes, that is what I meant. The consoleapp is compiled againstsuave.dll v0, so that works ok. but suave.oauth.dll is compiled against suave.dll v2, so will fail (as soon as it actually tries to load it, which it probably didn't in your example) I did just check it. ConsoleApp references lib1 and lib2. ConsoleApp references Lib1 v1. The app starts, but as soon as Lib2 tries to call into lib1, it fails at runtime. Here you can see that v1 is loaded into the process already, but lib2 needs v2: consoleapp can call into both lib1 and lib2 successfully, but as soon as lib2 calls into lib1, it will fail.
fullfx works quite different. If you don't have a SN, then the version doesn't matter at all. If you do have a SN, you need an exact match (or binding redirect). corefx, the version always matters, but a higher can satisfy a lower. This fixes a lot of issues, i like the new behavior. |
|
Does it work on the desktop? I think it may do but only because the desktop loader is sloppier with versions for weakly named assemblies, although I don't have a lot of recent experience with weak named assembly binding. For sure the desktop will complain that the manifest doesn't match if the assembly being loaded doesn't match the reference for strongly signed assemblies. It is true that binding redirects, Machine Policy and GAC policy can impact the reference. The CORECLR doesn't have policy knobs such as binding redirects or machine policy or GAC policy. Instead it has a binding logic that unifies references up at runtime. It would seem that they follow that policy also for weak named assemblies. Which does seem like it may be a detectable change on the CORECLR binding policy. |
afaik, it would probably work, afaik without sn the version is ignored and only the assembly name is relevant.
If this is a breaking change of the core-RUNTIME, then I would prefer FSC to notify me in an error condition. Otherwise I may get a nasty surprise at runtime |
|
@0x53A --- I think you are right. |
|
@matthid: this will fail, because protectedPart calls into Suave.dll
using System;
using Suave;
namespace suavefail
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine("ClientId: {0}", OAuth.EmptyConfig.client_id);
Console.WriteLine("Hello World!");
OAuth.protectedPart(null, null);
Suave.Web.startWebServer(Suave.Web.defaultConfig, (Successful.OK("Hello World!")));
}
}
} |
|
@0x53A I'm not really convinced by this, there is no reason to not use the resolution from the package manager but instead try to find different versions (and fail at it). However, this might be a fight which need to be fought at another place. So a change which prevents a runtime error and fails at compile time might be the right choice. BUT, we need to be careful. I'm pretty sure you can hook into the runtime resolver and make this work (by adding a line or two at the start of "Main"). So we might break people which already have build their own solutions (Maybe even FAKE). |
|
There is no package manager, there is just a flat list of dll files. And yes, this would need to be fought at coreclr itself. What I would like is a comeback of assembly redirects in some form. The default behavior of unifying up is great, and a vast improvement over fullfx, but a way to override this would sometimes be handy. |
Yes there is ;). In my repro steps NuGet is resolving a perfectly valid set of nuget packages when you type |
And no. I don't really care how it gets there ;). If we embed the result in the references metadata in the assembly or a binding redirects file or some similar solution doesn't really matter to me. |
|
Ultimately, I would judge this a package authoring error. Versions go up, not down. |
|
The compiler did use exactly the packages and dlls specified in the project. However, those packages produced a non-working program. On coreclr the loader has a different versioning heuristic. It is symmetrical between signed/unsigned and it says version go up and are expected to be compatible. An error is probably the much preferred solution here, for the project you supplied, until it is fixed. |
|
I've reported this upstream as SuaveIO/suave#635. But I think this discussion over the right interplay between compiler, package manager, and runtime has value in its own right, so I'd be in favor of keeping this issue open until the discussion has reached a conclusion. |
|
I hope we get more relevant information regarding the runtime internals via https://github.com/dotnet/coreclr/issues/13168. Apparently, assembly redirects should have worked - at least according to gitter. So if binding redirects should work I'm not so sure about the compiler error anymore. Also you can easily make it work: class Program
{
static void Main(string[] args)
{
AssemblyLoadContext.Default.Resolving += Default_Resolving;
OldMain();
}
private static void OldMain()
{
Console.WriteLine("ClientId: {0}", OAuth.EmptyConfig.client_id);
Console.WriteLine("Hello World!");
OAuth.protectedPart(null, null);
Suave.Web.startWebServer(Suave.Web.defaultConfig, (Successful.OK("Hello World!")));
}
private static System.Reflection.Assembly Default_Resolving(AssemblyLoadContext arg1, System.Reflection.AssemblyName arg2)
{
if (arg2.Name == "Suave")
{
return typeof(Suave.Web).GetTypeInfo().Assembly;
}
return null;
}
}I'm too lazy to implement the F# solution (because there is no good IDE), but I'm pretty sure it works in a similar way. You have to be a bit more careful, because the compiler itself is resolving the WRONG assembly, but it shouldn't matter (you just cannot use Basically this is "binding-redirects" done brute-force programmatically. Note that you need to introduce the additional method because of the way how assembly loading works. So should we forbid something which can be made work? |
|
@matthid what makes you think that coreclr has binding redirects? To the best of my understanding it doesn't ... the coreclr binding policy is this: Yes you can assembly handle resolution in application code. however, that is a super advanced solution and not one the compiler would get involved in. |
I edited the issue already, apparently it was a mistake from gitter. Documentation around it is a bit sparse and I'd like to get pointed to docs ;)
My argument is that with a compiler error we'd make that completely impossible. To be completely honest with you, currently I don't see any solution which makes me completely happy. I have wasted enough time in my life with assembly binding errors but still every solution has potential downsides. I guess we kind of have arranged ourself with binding redirects (in Paket world) to force the runtime to use our resolution, so I guess we are a bit unhappy that there is no replacement. Maybe we just need to arrange ourself with this highest version philosophy. I don't know jet. |
|
@matthid this higher philosophy is not new ... Silverlight has always had this binding policy, |
|
Closing as by design |
|
Is there a bug that tracks that assembly redirects are needed on core? |
|
@forki In which context? |
|
Ok that question implies you don't think it's true ;-) |
|
@forki This all sounds like we have to hack our own thing again :) |
|
Sigh. I just want to have dotnet use the version I specify. How hard can it
be?
Am 14.02.2018 20:13 schrieb "Matthias Dittrich" <notifications@github.com>:
… @forki <https://github.com/forki> This all sounds like we have to hack
our own thing again :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3408 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNB3-wAyLqOitKmfRNwocMbQ1yZpzks5tUzBLgaJpZM4OpmWy>
.
|
|
I think the root of the issue that you're having is that the (preview) Functions v2 runtime simply isn't ready for prime-time yet. At any rate, unless @KevinRansom and @dsyme change their minds I don't see a reason to re-open this as it is explicitly marked as by design. |
|
Nah it's not that. V1 runtime has no assembly redirects and it sucks big
time.
Am 14.02.2018 20:17 schrieb "Phillip Carter" <notifications@github.com>:
… I think the root of the issue that you're having is that the (preview)
Functions v2 runtime simply isn't ready for prime-time yet.
At any rate, unless @KevinRansom <https://github.com/kevinransom> and
@dsyme <https://github.com/dsyme> change their minds I don't see a reason
to re-open this as it is explicitly marked as by design.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3408 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADgNJASKEvEoX91VFwnyfvf7bEKQM_Kks5tUzE3gaJpZM4OpmWy>
.
|
|
For my future me. The above C# workaround in F# (as this happened again, see fsharp/fsharp-compiler-docs#906): System.Runtime.Loader.AssemblyLoadContext.Default.add_Resolving(Func<_,_,_> (fun arg1 arg2 ->
if arg2.Name = "FSharp.Compiler.Service"
then typeof<FSharp.Compiler.Ast.Ident>.Assembly
else null)) |
Initial Bugreport fsprojects/Paket#2576
It seems the compiler is not emitting the correct reference when given the same reference multiple times in different versions (could be a SDK bug)
Repro steps
dotnet new console -lang f#edit fsproj and add
Program.fsand adddotnet restore && dotnet runExpected behavior
Program executes
Actual behavior
Known workarounds
Use C#, because:
dotnet new consoleedit csproj and add
dotnet restore && dotnet runworksRelated information
If you look at the generated assemblies you can see that the C# version references:
// Suave, Version=0.0.0.0, Culture=neutral, PublicKeyToken=null(dotpeek -> no specific version)The F# version references:
// Suave, Version=2.0.0.0, Culture=neutral, PublicKeyToken=null, which is clearly wrong. I guess it was just the first (or last) in the command line to the F# compiler./cc @enricosada
The text was updated successfully, but these errors were encountered: