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

NuGet.Frameworks dependency is causing weird test failures when trying to load MSBuild #3154

Closed
twsouthwick opened this issue Nov 3, 2021 · 8 comments · Fixed by #4693
Closed

Comments

@twsouthwick
Copy link
Member

Description

I have a project (https://github.com/dotnet/upgrade-assistant) that loads MSBuild from an available source and lets that run how it will. Tests employing this fail when moving to .NET 6 because the test framework has already loaded an (old) version of the NuGet.Frameworks library.

System.IO.FileLoadException: Could not load file or assembly 'NuGet.Frameworks, Version=6.0.0.220, Culture=neutral, PublicKeyToken=31bf3856ad364e35'. Could not find or load a specific file. (0x80131621)

I've verified that before the tests start, NuGet.Frameworks is already loaded. This worked fine when it was a .NET 5 version of msbuild as the versions aligned. This can be fixed by adding the following line to the test project file:

<ItemGroup>
  <!-- The test framework brings in an old NuGet.Frameworks that ends up getting used rather than what MSBuild wants -->
  <PackageReference Include="NuGet.Frameworks" Version="6.0.0-rc.258" />
</ItemGroup>

I see a PR opened that is still in draft mode to address this (#2544). For anyone that is trying to test something that uses MSBuild, they'll probably hit this and end up spending too much time fiddling with things to figure out what is going on.

@twsouthwick
Copy link
Member Author

twsouthwick commented Nov 3, 2021

It seems like #2544 could be replaced with a source generator to autogenerate the right values. Happy to take a stab at that if that sounds good to you.

Another option would be to move the usage of it to a separate AppDomain on framework and AssemblyLoadContext on core (this doesn't work if it needs to be functional on a .NET Standard build).

@twsouthwick
Copy link
Member Author

I do think this highlights that the test framework should be very careful about what dependencies they are requiring. I don't know if you have a policy at this point, but any external dependency should be done in a separate appdomain/assemblyloadcontext (depending on runtime) so that it doesn't conflict with users' code that may have the same dependency

@nohwnd
Copy link
Member

nohwnd commented Nov 4, 2021

We depend on newtonsoft.json and nuget.frameworks. For .net framework tests we do have appdomains, but they often fail to unload, causing hangs in CI runs. For .NET AssemblyLoadContexts are not implemented, yet, unfortunately.

@JoeRobich
Copy link
Member

The NuGet.Frameworks dependency in the TestPlatform.ObjectModel is a terrible experience for tooling that loads MSBuild.

In dotnet-format I must ensure that my test projects reference a matching version of NuGet.Frameworks as the global.json pinned SDK brings in otherwise I get version mismatch at runtime. This is because the Test SDK of course uses TestPlatform.ObjectModel which has the NuGet.Frameworks dependency.

In OmniSharp where we use TestPlatform for test discovery and running, we have a similar issue in our vnext branch which runs on .NET 6. If we are not version matched with the version of the SDK we are attempting to run O# on top of, then we get version mismatches at runtime. We will likely have to copy the code we require from the TestPlatform.ObjectModel into O# just so we can avoid the NuGet Reference.

In Roslyn we get issues against our MSBuildWorkspace because users are using it in their tests and don't realize that a NuGet.Frameworks dependency is being added that will conflict with the version of the SDK they are running on top of. (dotnet/roslyn#52954 (comment))

Does the benefit that this dependency provides outweigh the many sometimes hard to diagnose issues that developers must workaround?

mjrousos added a commit to dotnet/upgrade-assistant that referenced this issue May 11, 2022
This updates the workaround used to address an issue in integration tests
due to microsoft/vstest#3154 to work with newer
versions of NuGet.
brandonh-msft added a commit to dotnet/upgrade-assistant that referenced this issue May 11, 2022
* Update the workaround to microsoft/vstest#3154

This updates the workaround used to address an issue in integration tests
due to microsoft/vstest#3154 to work with newer
versions of NuGet.

* Use wildcard package version so that the workaround doesn't need updated every time we use a newer NUGet version

* fix misspelling in comment

* Now let's see if we can use more recent net6 versions in our CI builds

Co-authored-by: Brandon H <brandonh-msft@users.noreply.github.com>
Co-authored-by: Brandon H <hurlburb@microsoft.com>
@Evangelink Evangelink added needs-triage This item should be discussed in the next triage meeting. wontfix by-design enhancement triaged and removed needs-triage This item should be discussed in the next triage meeting. wontfix by-design labels Jul 27, 2022
@Evangelink
Copy link
Member

See #2544

@pavelhorak
Copy link
Member

see #4693 which replaces #2544

@nohwnd
Copy link
Member

nohwnd commented Sep 25, 2023

@JoeRobich if we drop this dependency in net9 (but not in net7 or net8) will it help?

@JoeRobich
Copy link
Member

@JoeRobich if we drop this dependency in net9 (but not in net7 or net8) will it help?

@nohwnd It would. Even if we couldn't remove the NuGet dependency from our test projects right away. We could at least stop bumping the version with each new SDK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants