Sdk: Conflict resolution from desktop MSBuild fails to get assembly identity from cross-gen'ed linux binaries

[ericstj]

1. Crete a new console project.
2. Modify the contents to the following.

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.0</TargetFramework>
    <RuntimeIdentifier>centos.7-x64</RuntimeIdentifier>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="System.Threading" Version="4.3.0" />
  </ItemGroup>

</Project>

1. dotnet restore
2. dotnet publish --> works correctly
3. msbuild /t:publish --> app will fail to start with a refdef mismatch for System.Threading.

This is because the System.Threading.dll conflict isn't being resolved. If you examine the log you'll see

Encountered conflict between 'CopyLocal:C:\Users\ericstj\.nuget\packages\runtime.linux-x64.microsoft.netcore.app\2.0.0\runtimes\linux-x64\lib\netcoreapp2.0\System.Threading.dll' and 'CopyLocal:C:\Users\ericstj\.nuget\packages\system.threading\4.3.0\lib\netstandard1.3\System.Threading.dll'.  Could not determine a winner because 'CopyLocal:C:\Users\ericstj\.nuget\packages\runtime.linux-x64.microsoft.netcore.app\2.0.0\runtimes\linux-x64\lib\netcoreapp2.0\System.Threading.dll' is not an assembly.

Compare this to the following in dotnet publish:

Encountered conflict between 'CopyLocal:C:\Users\ericstj\.nuget\packages\runtime.linux-x64.microsoft.netcore.app\2.0.0\runtimes\linux-x64\lib\netcoreapp2.0\System.Threading.dll' and 'CopyLocal:C:\Users\ericstj\.nuget\packages\system.threading\4.0.11\lib\netstandard1.3\System.Threading.dll'.  Choosing 'CopyLocal:C:\Users\ericstj\.nuget\packages\runtime.linux-x64.microsoft.netcore.app\2.0.0\runtimes\linux-x64\lib\netcoreapp2.0\System.Threading.dll' because AssemblyVersion '4.1.0.0' is greater than '4.0.11.0'.

It would seem that the desktop implementation for getting the assembly version can't handle the cross-gened assemblies, however the core implementation can.

[ericstj]

/cc @dsplaisted

Opened as a result of https://github.com/dotnet/corefx/issues/23283

[rainersigwald]

Related: https://github.com/Microsoft/msbuild/issues/2253 (though this should be possible by unifying to (slower) System.Reflection.Metadata where that one isn't AFAIK.

[nguerrera]

I'm seeing related strangeness from sn.exe saying crossgened binaries do not have a strong name.

What's the proposed fix on this: move everything to S.R.M? Are we just holding the COM API wrong?

[nguerrera]

@ericstj In mail, you said this was lower priority, but seems severe. What's the mitigation?

[nguerrera]

S.R.M should be just as fast if we can use an ngen'ed version as roslyn does.

[ericstj]

Mitigation is to use dotnet.exe to publish. I'm looking at another mitigation: use the S.R.M based task from VS/MSBuild.

I had initially thought this was lower pri because folks weren't doing as much cross-targeting (building on Windows to publish to unix) from VS, but I may be wrong. Just saw another case where someone hit this.

EDIT: update, I cannot use the existing S.R.M based task easily. MSBuild will only load one copy of the assembly into the process (since they have the same name) and first in wins. I can't switch everything because we're missing some of the facades that would be required by desktop to load the netcoreapp assembly.

[ericstj]

@jkotas is it expected that desktop COM metadata APIs throw BadImageFormat on x-plat cross-gen images or are we just calling them wrong?

[jkotas]

Yes, the desktop COM metadata reader is likely going to have problem with Unix x-plat cross-gen images.

[ericstj]

This PR would have fixed this by switching over to S.R.M, but we had more reasons than just perf for not taking it: https://github.com/dotnet/sdk/pull/1215

One hacky/partial solution if we can't switch to S.R.M is to use the platform manfiest as a fast-path for this metadata. All of the problematic assemblies come from our platform package so we can apply the metadata from the manifest if the packageId is correct.

[ericstj]

@livarcocc We are seeing lots of hits on this issue. I think we need to get a fix in.

[nguerrera]

@ericstj What's the proposed fix approach? Is the "partial" solution using platform manifest good enough for now?

[ericstj]

Probably: we're likely the only people cross-gen'ing assemblies and putting them in packages. If we just preferred items from platform manifest whenever they overlapped with runtime items I think that'd fix most instances of this issue.

[oacsed]

Any news on this issue? This is very frustrating to say the least.

[ericstj]

@livarcocc @dsplaisted are you working on this? I just looked at an issue today where we were hitting this and it surfaces in a gnarly way. Successful build and publish, then MissingMethodException at runtime for customer code method (types don't agree at runtime do to mix of v1 and v2 assemblies).

[dsplaisted]

@ericstj @weshaggard The platform manifest is only set in Microsoft.NETCore.App.props if the app is not self-contained:

  <ItemGroup Condition="'$(RuntimeIdentifier)' == '' or '$(SelfContained)' != 'true'">
    <PackageConflictPlatformManifests Include="$(MSBuildThisFileDirectory)Microsoft.NETCore.App.PlatformManifest.txt" />
  </ItemGroup>

So we don't have access to it when publishing a self-contained app. In order to use it for the assembly versions, we'd need the Microsoft.NETCore.App to include it even for self-contained apps (although I'm not sure what other affects that would have on conflict resolution, so it might need to go in a different item so we only use it to get the assembly versions).

[ericstj]

True, the platform manifest is used in lieu of the real run-time packages and is meant to indicate items that conflict resolution won't see.

Another thing to look at might be the package override resolver and making that work on publish as well. That's not perfect though since its currently missing a lot of runtime packages.

Both of these feel like hacks though. What did you end up doing for the AssemblyVersion reading for the deps feature?

[dsplaisted]

@ericstj The feature to write assembly and file versions to deps.json uses the same code to read the assembly versions. So it would fail to get the assembly version for cross-gened assemblies. However, I don't think there's any scenario where cross-gened assemblies would be deployed in a framework-dependent app, so the deps.json feature is probably OK.

[ericstj]

I see. I guess that depends on whether or not any customer's are cross-gen'ing assemblies in NuGet packages above the shared framework. Regardless, we're not going to find a solution there: on the plus side if we fix this issue it will also fix that.

I think we should aim at getting the assembly reading code fixed rather than trying to workaround it. It feels like there are too many moving pieces with workarounds. I was looking at MSBuild and I see it's using S.R.M itself so it seems reasonable that we can somehow share that implementation or at least get it to successfully load in the task. Do you think you can look into this?

[dsplaisted]

@ericstj Looking at the MSBuild project files, it looks like System.Reflection.Metadata is only used if not targeting .NET Framework, the same as our task. So that doesn't help us if we're running in desktop MSBuild.

[ericstj]

That's odd, I see it in the layout under \

If you're concerned with perf on the happy path you could pass a flag that only attempts to fallback to SRM on desktop for publish (or even for cross-publish).

[nguerrera]

I see it in the layout under \MSBuild\15.0\bin

I see it only under 15.0\Bin\Roslyn where it will not be loaded into MSBuild.exe or devenv.exe, but rather csc.exe/vbc.exe/VBCSCompiler.exe.

[nguerrera]

Now VS generally is prepared to load the same version as Roslyn, but I am terrified about being another thing that needs to get involved when its version in VS changes. I'm also terrified at creating appdomains or assembly resolve events, etc. etc. :(

[nguerrera]

cc @tmat @jaredpar

[ericstj]

For completeness: We could use CCI or LMR, or carry a private copy of part of SRM. Any of these less scary?

Why do we care about loading the same version as the rest of VS and roslyn? We aren't exchanging types. If we're only talking about cross-publish I don't think inner-loop perf / working set is a huge issue.

[nguerrera]

Those are even scarier.

If there's RTM versions we can take that don't require redirects then we're probably ok. But I've been involved in multiple task dll hell episodes, and sometimes we get dragged into devenv.exe.

IIRC, you attempted to use SRM on desktop once before and ended up closing the PR with something like "this is exactly why I avoided it in the first place"

I want to do the right thing. I just want to be sure we've thought it all through.

[jaredpar]

FWIW: in Roslyn we decided to not use SRM in our build task. Instead we pulled out a small enough sample to read the MVID into a separate code file and just used that source directly.

https://github.com/dotnet/roslyn/blob/master/src/Compilers/Core/MSBuildTask/MvidReader.cs

As @nguerrera noted that copy of SRM is only there to support the compilers which exist in the same directory.

Why do we care about loading the same version as the rest of VS and roslyn? We aren't exchanging types. If we're only talking about cross-publish I don't think inner-loop perf / working set is a huge issue.

It just makes it yet another copy of SRM / SCI that we have to consider when we try and rev to a new version. Due to implementation details of VS builds we can't really consider only the copies we want to upgrade. Instead we have to consider every copy of the DLLs which get laid out in every VS SKU installation. They all need to be updated at the same time in order for us to move SCI / SRM forward. (sound terrifying? Then you're reading that right).

[ericstj]

The versions I mentioned above (SRM 1.5.0) don't require redirects. Our build in CoreFx changed in 2.0 such that we don't require redirects. In 1.0 folks had to decide if they wanted that behavior and we typically didn't because we had different priorities: preserve independent update of components. In 2.0 our priorities changed to prefer building a set/wave of packages.

The issue I hit before was due to redirects being needed in the pre-2.0 packages: SRM 1.4.x. If you pick up SRM 1.5 this shouldn't be an issue, and so long as we (CoreFx) don't do anything funny with those updates in the future (like revert to 1.x priorities) it will continue to work.

I buy the desire for unification within the VS process but I'd argue that you can't solve msbuild task unification problem since it isn't a closed set. Moreover, doesn't the SDK ship as a separate thing to VS? I don't understand how a VS build implementation detail would constrain the SDK if they don't even ship together. Perhaps whatever forces roslyn to unify doesn't apply to the SDK. FWIW you're already loading a different version than the rest of VS when running on .NETCore's shared framework (which if I recall ships with the SDK).

[jaredpar]

I buy the desire for unification within the VS process but I'd argue that you can't solve msbuild task unification problem since it isn't a closed set.

True in the sense that MSBuild is a plugin model and everyone can bring the dependencies they choose. But this isn't true in the VS case I'm referring to. There is a specific set of binaries on disk for a given SKU and they can be viewed in their totality.

Moreover, doesn't the SDK ship as a separate thing to VS?

It can ship separately. It also ships in box (just like Roslyn).

[nguerrera]

Moreover, doesn't the SDK ship as a separate thing to VS?

Yes. There have been lots of issues with our nuget.*.dll dependencies around this. It has been mostly fixed by ensuring nuget now bumps its assembly version so that we can load SxS successfully if version in devenv is older than us. But the latest episode involved an extension adding assembly resolve events for nuget.*.dll and implementing it wrong. The fewer common things that get loaded into the net46 copy of our build tasks the better. Each one adds some risk of some form of DLL hell.

FWIW you're already loading a different version than the rest of VS when running on .NETCore's shared framework (which if I recall ships with the SDK).

True. The netcoreapp version of our build tasks don't end up in devenv.exe ever.

[ericstj]

We always version our assemblies between releases so we won’t have the nuget issue and the only way to avoid the ResolveEvent is to not attempt it which I believe we are safe.

I do understand the netcoreapp difference: just sharing that to demonstrate that the generalization Jared made wasn’t quite so universal. I’m curious to understand the real requirements behind unification since that’s what is required to understand if it applies here.

[tmat]

I think it would be interesting to look into using ILLink for task assemblies. Merge and tree-shake all dependencies of a task except for MSBuild assemblies into a single DLL. Then we would not have any versioning issues.

[nguerrera]

Spoke to @tmat offline about @jaredpar's specific concern of just having a mismatched S.R.M or S.C.I on disk in VS install breaking VS ngen. It seems to not be an issue here because the directories probed are defined in vsn.exe.config. There are various copies of different versions in Program Files\dotnet (where we install, not in the list) and they haven't ever triggered a VS NGEN break.

So I think we can carry v1.5.0 and use it on a fallback path (so that we don't load / JIT unnecessarily). And without any app-domain or assembly resolve shenanigans.

I think there's non-negligible risk here, but I'm not seeing how to fix this bug without taking it.

I do like the idea of static linking things like nuget and srm into the build task at some point, but I don't think we can afford to make that a pre-req of this bug fix.