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

Unable to build using bootstrap: Method not found: 'NuGet.ProjectModel.LockFile.GetTarget' #6289

Closed
KirillOsenkov opened this issue Mar 22, 2021 · 18 comments · Fixed by #6301
Labels
Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. triaged

Comments

@KirillOsenkov
Copy link
Member

I have installed VS 16.9.2 on my local machine and now my bootstrapped MSBuild no longer works:

C:\Program Files\dotnet\sdk\5.0.201\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.targets(241,5): error MSB4018: The "ResolvePackageAssets" task failed unexpectedly.
System.MissingMethodException: Method not found: 'NuGet.ProjectModel.LockFileTarget NuGet.ProjectModel.LockFile.GetTarget(NuGet.Frameworks.NuGetFramework, System.String)'.
   at Microsoft.NET.Build.Tasks.LockFileExtensions.GetTargetAndReturnNullIfNotFound(LockFile lockFile, String frameworkAlias, String runtimeIdentifier)
   at Microsoft.NET.Build.Tasks.LockFileExtensions.GetTargetAndThrowIfNotFound(LockFile lockFile, String frameworkAlias, String runtimeIdentifier)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheWriter..ctor(ResolvePackageAssets task)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader.CreateReaderFromDisk(ResolvePackageAssets task, Byte[] settingsHash)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.CacheReader..ctor(ResolvePackageAssets task)
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ReadItemGroups()
   at Microsoft.NET.Build.Tasks.ResolvePackageAssets.ExecuteCore()
   at Microsoft.NET.Build.Tasks.TaskBase.Execute()
   at Microsoft.Build.BackEnd.TaskExecutionHost.Microsoft.Build.BackEnd.ITaskExecutionHost.Execute() in C:\msbuild\src\Build\BackEnd\TaskExecutionHost\TaskExecutionHost.cs:line 585
   at Microsoft.Build.BackEnd.TaskBuilder.<ExecuteInstantiatedTask>d__26.MoveNext() in C:\msbuild\src\Build\BackEnd\Components\RequestBuilder\TaskBuilder.cs:line 809 [C:\temp\net472\net472.csproj]

Looks like it's some binary incompatibility?

@KirillOsenkov KirillOsenkov added the Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. label Mar 22, 2021
@KirillOsenkov
Copy link
Member Author

I build using build /p:CreateBootstrap=true and then run C:\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe

@KirillOsenkov
Copy link
Member Author

Looks like this method is in the SDK: https://github.com/dotnet/sdk/blob/main/src/Tasks/Microsoft.NET.Build.Tasks/LockFileExtensions.cs

@dsplaisted in case you know what's going on

@dsplaisted
Copy link
Member

The LockFile.GetTarget method is from NuGet, the SDK extension methods are wrappers around it.

I'm not sure why this would be breaking, but try to figure out what version of the NuGet APIs are being used in the bootstrapped MSBuild.

@KirillOsenkov
Copy link
Member Author

Hey @brianrob I'm narrowing this down to this commit:
0fc8263

I'm seeing NuGet.Frameworks.dll loaded from two places:
C:\MSBuild\artifacts\bin\bootstrap\net472\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.Frameworks.dll
C:\MSBuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\NuGet.Frameworks.dll

I'm confirming that the code I'm debugging is using LoadFrom:
image

Interesting that in VS NuGet.Frameworks.dll only deployed in C:\Program Files (x86)\Microsoft Visual Studio\2019\Enterprise\Common7\IDE\CommonExtensions\Microsoft\NuGet\NuGet.Frameworks.dll, it is not deployed next to MSBuild.exe. But for our bootstrap, another copy of the same .dll is copied next to MSBuild.exe and loaded from there.

So I suspect this is only an issue with bootstrap since it has two copies?

@KirillOsenkov
Copy link
Member Author

I've tried various things and this is what worked for me:

                var NuGetAssembly = AppDomain.CurrentDomain.GetAssemblies()
                    .FirstOrDefault(a => string.Equals(a.GetName().Name, "NuGet.Frameworks", StringComparison.OrdinalIgnoreCase));
                if (NuGetAssembly == null)
                {
                    var name = AssemblyName.GetAssemblyName(Path.Combine(assemblyDirectory, "NuGet.Frameworks.dll"));
                    NuGetAssembly = Assembly.Load(name);
                }

Not sure if this should be the official fix, but at least it's something that unblocks local bootstrap for net472.

This all seems hacky and fragile, moreover I don't understand how did it use to work fine two months ago?

@KirillOsenkov
Copy link
Member Author

Huh, interesting, reverting back to LoadFile also fixes it. Feels like my latest hack and the original LoadFile are equivalent??

In general, I have a feeling that all of this dynamic assembly loading is terribly fragile and fraught with peril. Feels like all of this could be drastically simplified if we just shipped NuGet and MSBuild in the same directory (famous last words??)

But anyway, short term let's think about how we can unbreak local bootstrap. I'm afraid that my Assembly.Load() fix will result in the same JITting that #6126 solved.

@brianrob
Copy link
Member

@KirillOsenkov, can you tell me how to repro what you're seeing?

There are some load behavior differences in terms of LoadFile and LoadFrom, and so I'm not surprised that switching back to LoadFile might solve this. What I'm interested in really understanding is whether or not there is an expectation that multiple versions of NuGet.Frameworks.dll get loaded at the same time.

@KirillOsenkov
Copy link
Member Author

Sorry I should have added that you need to pass /r to hit the NuGet codepaths.

build /p:CreateBootstrap=true
C:\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\MSBuild.exe C:\temp\net472\net472.csproj /r /bl

@KirillOsenkov
Copy link
Member Author

I'm realizing that no matter what we use to load the assembly the real problem is that we have two copies of NuGet.Frameworks.dll loaded in the bootstrap case, which doesn't seem to be the case for VS.

We need to figure out how VS ensures that only a single copy is loaded and replicate that mechanism. For bootstrap, we deploy NuGet*.dll assemblies right next to MSBuild.exe and that's not the case in VS.

@KirillOsenkov
Copy link
Member Author

@nkolev92 @loic-sharma @rrelyea @zivkan how does VS load NuGet.Frameworks.dll from a single location? When we build MSBuild bootstrap locally, we end up with two NuGet.Frameworks.dlls and we're thinking about how to replicate what VS does. Any tips greatly appreciated!

@nkolev92
Copy link

nkolev92 commented Mar 24, 2021

We don't do anything special to prevent duplicate loading of NuGet.Frameworks. In fact I'd expect that it happens in some occasions.

  • Components should not depend on NuGet.Framework directly.
  • The only 2 components that specifically depend on this assembly are NuGet itself and the SDK. Each of these components brings their own version of the assembly, it just so happens that most frequently these versions are the same.

If you use global.json in a newer VS, I'd expect you might run into a double loading scenario.

Related: NuGet/Home#9611.

@zivkan
Copy link
Member

zivkan commented Mar 24, 2021

I think in VS there's only a single NuGet.Frameworks.dll loaded when msbuild is compiled against the same assembly version of NuGet.Frameworks.dll that NuGet itself ships. Hence when different assemblies in VS try to load the same NuGet.Frameworks.dll assembly version, the .NET Framework assembly loader knows to just re-use the same loaded version, where ever that was loaded from.

Before msbuild took a dependency on NuGet.Frameworks.dll, nothing else in VS was "allowed to" directly use NuGet's NuGet.* packages, other than NuGet.VisualStudio or NuGet.VisualStudio.Contracts. NuGet's APIs are not stable enough (the error message from this issue's first post is a perfect example) to risk allowing other VS components to take a direct dependency and breaking when NuGet ships a new version with a breaking API. Hence we intentionally do not provide binding redirects, and if any VS component asks us about using NuGet APIs, we point them to our VS APIs and if it doesn't cover the scenario they need, to request new VS APIs for what they need. A 3rd party using NuGet in their VS extension had to re-compile NuGet in their extension, to use a different strong name key, to avoid breaking VS.

None of that helps msbuild in this case, but I hope it gives background to why in the past only one copy of the dll was loaded. It's because only NuGet's own VS component used it, nothing else does.

I think msbuild could use app.config's probingPath (or codebase?) to tell the .NET Framework assembly loader to look for NuGet.Frameworks.dll in NuGet's install directory. The relative path between NuGet's install directory and MSBuild.exe will always be the same. However, then there will be binding redirect (assembly version) issues when msbuild was compiled against a different version of nuget that ships in VS.

I'm not sure my message here is helping solve this issue though. I'm happy to go on a call to talk if that helps.

@brianrob
Copy link
Member

I have taken some traces of the good and bad cases. Interestingly, NuGet.Frameworks.dll gets loaded twice in both the good and bad cases. The actual DLL path is different, but the files are identical, and the requested load versions are identical. However, the load context within the runtime is different. My expectation is that this would work, but obviously it is not.

The MissingMethodException is for NuGet.ProjectModel.LockFileTarget NuGet.ProjectModel.LockFile.GetTarget(NuGet.Frameworks.NuGetFramework, System.String). I have also confirmed that NuGet.ProjectModel.dll is only loaded once and the location of the load is the same in both the good and the bad cases.

In the functional case, there is one more assembly load than in the failed case: C:\src\msbuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\Roslyn\Microsoft.Build.Tasks.CodeAnalysis.dll This assembly, and none of it's transitive dependencies depend upon any of the NuGet dlls.

I suspect that I'm going to need to get some more loader expertise on this. I'll report back on what I find out.

@dsplaisted
Copy link
Member

Isn't this just an issue with the way we create the bootstrap version of MSBuild? Would it work if we weren't copying NuGet DLLs to the bootstrapped versions MSBuild bin folder?

@brianrob
Copy link
Member

Unfortunately not - it just fails to load the DLL. This feels like a load context limitation that I don't fully understand. FWIW, if I change NuGetFrameworkWrapper to load NuGet.Frameworks.dll via the LOAD context (e.g. Assembly.Load) that fixes the problem, or at least I don't get an error. This does change the load behavior though - because you can't choose a specific path to NuGet.Frameworks.dll. I want to try and understand if this is OK.

@brianrob
Copy link
Member

This is also going to mean that we only load one version of NuGet.Frameworks.dll in this build - specifically, this one: C:\MSBuild\artifacts\bin\bootstrap\net472\MSBuild\Current\Bin\NuGet.Frameworks.dll. Do we know if there is a reason that NuGetFrameworkWrapper tries to prefer the Visual Studio one?:

            // Resolve the location of the NuGet.Frameworks assembly
            var assemblyDirectory = BuildEnvironmentHelper.Instance.Mode == BuildEnvironmentMode.VisualStudio ?
                Path.Combine(BuildEnvironmentHelper.Instance.VisualStudioInstallRootDirectory, "Common7", "IDE", "CommonExtensions", "Microsoft", "NuGet") :
                BuildEnvironmentHelper.Instance.CurrentMSBuildToolsDirectory;

I'm concerned that if we load via Assembly.Load that we're going to end up with another of these types of issues down the line.

@brianrob
Copy link
Member

FYI, I've posted #6301 to fix this. If you're interested, there are details in the PR description on what caused the failure.

@KirillOsenkov
Copy link
Member Author

And for the NuGet folks on this thread, here's a longer term issue to rationalize how we deploy NuGet binaries for MSBuild bootstrap: #6302

It may be that we need to stop deploying them next to MSBuild.exe and instead add some kind of probing path or other mechanism (AppDomain.CurrentDomain.AssemblyResolve?) to fish them out from Common7\IDE\CommonExtensions\Microsoft\NuGet

Forgind pushed a commit that referenced this issue Mar 31, 2021
Fixes #6289

Context
#6126 changes the way that NuGet.Frameworks.dll is loaded to be used by NuGetFrameworkWrapper from a call to LoadFile to a call to LoadFrom. The path passed in to the Load*` call is the same in both cases, but the behavior is different when running on .NET Framework.

On .NET Framework, calling LoadFile results in IJW loading rules being followed, and a different copy of NuGet.Frameworks.dll
than the one requested being loaded. It also appears to match the one loaded into the LOAD context. Calling LoadFrom results in the specified assembly being loaded, but it doesn't match the copy of the assembly loaded into the LOAD context. Thus, it remains in the LOADFROM context instead of being promoted to the LOAD context.

Later on, there is a collision between code from the two instances of NuGet.Frameworks.dll that are loaded into the LOAD context and the LOADFROM context, and this is where the MissingMethodException is thrown.

Note, this does not happen on the .NET Core bootstrap build because the loader behavior is significantly simpler.

Changes Made
Choose the Load* API based on the target framework at build time. On .NET Core, use LoadFrom and on .NET Framework, use LoadFile. This type of precedent already exists in MSBuild where there is different load behavior for .NET Framework and .NET Core.
@AR-May AR-May added the triaged label Feb 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Our Own Build Problems affecting the build or build infrastructure of the MSBuild repo itself. triaged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants