ionide / proj-info

Parse and evaluate MsBuild project files
MIT License
64 stars 37 forks source link

Resolve Framework and COM references #172

Closed dsyme closed 1 year ago

dsyme commented 1 year ago

Loading a project should fully resolve framework and COM references, see #171

Tests needed

dsyme commented 1 year ago

@baronfel I tried to look into the test failures but I'm finding the engineering in this repo a bit difficult.

image

baronfel commented 1 year ago

expecto runs via dotnet run on the test project, and can be filtered with --filter "TEST NAME PATTERN". There's a dotnet tool that you can run against a project with dotnet run as well to do things more interactively.

Due to the MSBuild dependency, many more traditional dotnet test wrappers like nunit/xunit don't work well (the xunit runner enforces a local dependency on Microsoft.Build.Framework, for example).

dsyme commented 1 year ago

@baronfel I see thanks.

Given how core this component is I do wonder if we should simplify a few things - e.g. remove Paket, remove Ionide.KeepAChangeLog whatever that is.

baronfel commented 1 year ago

Paket is a potential for removal, but the Ionide.KeepAChangelog dependency is a valuable way to ensure that version numbers and release notes are a) kept, and b) included in all assembly and nuget package outputs automatically.

dsyme commented 1 year ago

but the Ionide.KeepAChangelog dependency is a valuable way to ensure that version numbers and release notes are a) kept, and b) included in all assembly and nuget package outputs automatically.

OK, though it fails in VisualStudio, not sure why. Removing it through makes the solution loadable in latest VS 2022 Preview

baronfel commented 1 year ago

Correct, this is a known issue logged on that repo. If we moved off of Paket it would be very easy to conditionally include it based on if the user is accessing from VS (at least until the bug is fixed).

baronfel commented 1 year ago

I think what's going on with the tests is that some projects (.NET SDK projects?) may not have some of the targets based on TFM - some of the new targets may need to pivot on TFM.

dsyme commented 1 year ago

I tried these:

dotnet run --framework net6.0 --project test\Ionide.ProjInfo.Tests\Ionide.ProjInfo.Tests.fsproj
dotnet run --framework net7.0 --project test\Ionide.ProjInfo.Tests\Ionide.ProjInfo.Tests.fsproj

The first failed various tests with some errors due to .NET 7, the second with this:

C:\Program Files\dotnet\sdk\7.0.100-rc.1.22431.12\Sdks\Microsoft.NET.Sdk\targets\Microsoft.PackageDependencyResolution.
targets(267,5): error NETSDK1005: Assets file 'C:\GitHub\dsyme\proj-info\test\Ionide.ProjInfo.Tests\obj\project.assets.
json' doesn't have a target for 'net7.0'. Ensure that restore has run and that you have included 'net7.0' in the Target
Frameworks for your project. [C:\GitHub\dsyme\proj-info\test\Ionide.ProjInfo.Tests\Ionide.ProjInfo.Tests.fsproj::Target
Framework=net7.0]
baronfel commented 1 year ago

@dsyme I've got this change on another PC, but we need to add a 7.0 TFM to the tools project and rerun dotnet paket install, then you should be all clear.

baronfel commented 1 year ago

Well, belay that a bit - you need to set BuildNet7 to true in your local environment. There are reasons for this related to runtime selection by the SDK, but this is how we do the 7.0 testing in CI.

dsyme commented 1 year ago

@baronfel OK thanks. I'll have to leave this for a while but does the change here look right? I'm really surprised we haven't needed this before. How else are our tools all getting framework references?

TheAngryByrd commented 1 year ago

Closing as https://github.com/ionide/proj-info/pull/177 should fix this