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

try to remove MSBuild assemblies #715

Merged
merged 1 commit into from
Oct 30, 2021
Merged

Conversation

nhirschey
Copy link
Collaborator

This is an attempt to remove MSBuild assemblies from the FSharp.Formatting.dll nuget. This change is requested in issue #714, which came up in @baronfel's attempt to upgrade Ionide/FSAC for dotnet 6.

The changes are small, but I am a novice with respect to fsproj files and builds, so somebody with better knowledge should double check that I'm not doing anything stupid.

Changes

  1. I removed the explicit assembly inclusion in FSharp.Formatting.fsproj that @baronfel mentioned in the above referenced issue 714.

  2. I set the MsBuild nugets in paket.dependencies to copy_local: false which "is equivalent to setting ExcludeAssets=runtime for a PackageReference in NuGet" (relevant paket docs). ExcludeAssets=runtime is the suggestion in the msbuild documentation link that @rainersigwald provided.

Testing:

  1. I ran dotnet fake build -t All and all tests pass.

  2. I inspected the internals of artifacts\FSharp.Formatting.11.4.4.nupkg\lib\netstandard2.1\. It included MsBuild dlls before the change, but after the change it does not have those dlls.

  3. I inspected the command line tool nupkg and it still includes the msbuild dlls in artifacts\FSharp.Formatting.CommandTool.11.4.4.nupkg\tools\net5.0\any\

image

  1. I tried using artifacts/fsdocs.exe to build a library of my own and to build the FSharp.Formatting docs itself. Everything worked fine. Below is a gif showing the results of building the libraries own documentation with artifacts/fsdocs.exe watch --eval --properties Configuration=Release --clean

no-msbuild

@dsyme
Copy link
Contributor

dsyme commented Oct 30, 2021

This looks great!!!

@dsyme dsyme merged commit be3e8ea into fsprojects:master Oct 30, 2021
@baronfel
Copy link
Collaborator

Perfect, that's exactly the changes required. Nice job doing validation as well 👍

@dsyme
Copy link
Contributor

dsyme commented Nov 6, 2021

I'm not sure this is a sound fix - I'm getting failures reporting missing MSBuild DLLs when running tests locally.

FCS has had an MSBuild dependency for a long time. What do we know about why that's currently causing a problem for FsAutoComplete?

@baronfel
Copy link
Collaborator

baronfel commented Nov 6, 2021

the dependency isn't the problem, it's that the dlls themselves were xcopied into the library. the dependencies should be expressed as nuget package dependencies (which is handled by the FSharp.Compiler.Service dependency itself), and not be inlined into the package output itself. Applications that need the dlls are probably running into a PackageReference transitive dependency copy issue and might be resolved by adding an explicit top-level dependency to their paket.references on the MSBuild nugets required. I think there's and SDK or Nuget issue about this, and it's bitten me a few times as well.

@dsyme
Copy link
Contributor

dsyme commented Nov 6, 2021

I see.

Yes, they shouldn't be copied into the package. I was meaning the copy_local: false part which is about inside this repo. Thanks

@nhirschey
Copy link
Collaborator Author

nhirschey commented Nov 6, 2021

  1. At the time I made this pull request, I ran dotnet fake build -t All locally and all tests passed. However, today I am seeing test failures when I run that too. The only difference I can think of is that in that time I have installed Visual Studio 2022 on my machines this past week.

  2. When I use global.json to pin to dotnet 5.0, I can still pass tests:

{
    "sdk": {
      "version": "5.0.200",
      "rollForward": "minor"
    }
  }

image

  1. The copy_local:false was added to follow this guide: https://docs.microsoft.com/en-us/visualstudio/msbuild/updating-an-existing-application?view=vs-2019#use-nuget-packages-preferred

  2. I believe the project cracker in this project uses Ionide's project cracker. On dotnet 6, could it be hitting the same msbuild issues that @baronfel is working through in ionide?

    let msbuildExe = Ionide.ProjInfo.Init.init ()

@baronfel
Copy link
Collaborator

baronfel commented Nov 6, 2021

It might be worth trying to see if version 0.54.2 solves this for you, then. We changed how we detect and isolate the msbuild versions in use. 0.55.0 also bumps to FCS 41, if you want to go that far.

@dsyme
Copy link
Contributor

dsyme commented Nov 8, 2021

It might be worth trying to see if version 0.54.2 solves this for you, then. We changed how we detect and isolate the msbuild versions in use. 0.55.0 also bumps to FCS 41, if you want to go that far.

In #719 I'm updating to .NET 6, but it's slow going, the combined bump to ProjInfo 0.55 and FCS 41 and .NET 6 is tricky ionide/proj-info#122

One issue was FAKE didn't have net6.0 support, I think that's being fixed.

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

Successfully merging this pull request may close these issues.

3 participants