Don't ship the FSharp.Core XmlDocs as Content in the nuget package.

[teo-tsirpanis]

Fixes #12706.

[KevinRansom]

@teo-tsirpanis ---

This was added so that when developers reference the FSharp.Core nuget package they get the .xml in the output file, without having to add the file to the project and copying it. Are you sure that is not necessary ?
#10579

[teo-tsirpanis]

they get the .xml in the output file, without having to add the file to the project and copying it

I don't understand why they would want this, and why they would want it only for FSharp.Core. The issue mentions "mak[ing] tooltips for F# Scripts in Ionide better" but can't Ionide just see the XML file in the lib folder?

Are you sure that is not necessary?

No idea, I opened it because I saw the issue. 😅

I was also seeing two FShap.Core.xml files in Rider's solution explorer in my project, and was getting confused, until I stumbled upon this. That's a cosmetic issue though (at least as far as I know).

[baronfel]

This issue here wasn't for ionide - it was for any consumer that tries to use the FSharp.Core shipped in the .NET SDK as the source of truth (so anyone wanting to do analysis/editor services for dotnet fsi, for example).

That being said, I completely agree that content files aren't needed in the main, NuGet.org-ified version of this package. We should be able to do things in the SDK's layout project to retrieve the content files and put them in the correct position.

[dsyme]

I agree these shouldn't be content files. I can see the need to get them into the SDK in the right spot but yes, can we solve that another way please?

(so anyone wanting to do analysis/editor services for dotnet fsi, for example).

Should we maybe have a separate package with the FSharp.Core.xml as content?

[vzarytovskii]

So, looked at it with Kevin, and it turns out, that we already do not publish it for the implicit FSharp.Core:

fsharp/src/FSharp.Build/Microsoft.FSharp.NetSdk.props

Lines 84 to 94 in e2782fd

	<PackageReference
	Include="FSharp.Core" Version="$(FSharpCoreImplicitPackageVersion)"
	Condition="'$(DisableImplicitFSharpCoreReference)' != 'true' and '$(FSharpCoreImplicitPackageVersion)' != ''">
	<ExcludeAssets Condition="'$(FSharpCoreIncludeDocFileInOutput)' != 'true'">contentFiles</ExcludeAssets>
	</PackageReference>
	<PackageReference
	Include="FSharp.Core" Version="$(FSCorePackageVersion)"
	Condition="'$(DisableImplicitFSharpCoreReference)' != 'true' and '$(FSharpCoreImplicitPackageVersion)' == ''">
	<ExcludeAssets Condition="'$(FSharpCoreIncludeDocFileInOutput)' != 'true'">contentFiles</ExcludeAssets>
	</PackageReference>

To enable the publishing, the following property has to be set:

<FSharpCoreIncludeDocFileInOutput>true</FSharpCoreIncludeDocFileInOutput>

and MSBuild will take care of it.

If the project is using explicit FSharp.Core for some reason, contentFiles need to be excluded explicitly:

<PackageReference Include="FSharp.Core" Version="7.0.0">
      <ExcludeAssets>contentFiles</ExcludeAssets>
</PackageReference>

Given that, I don't think we should explicitly exclude it from the nupkg, since some tooling may need it for displaying tooltips and docs.

@baronfel @teo-tsirpanis am I missing something, and some other scenarios are broken?

[NinoFloris]

I wouldn't call that 'not published', it's there in the package after all.
It's just hidden after the fact on the implicit reference (a concept that doesn't work with NuGet CPVM for instance, see #3678).

I just fail to see why this needs to be in this particular place in the package.

It's tooling data, not something for end user projects.

[vzarytovskii]

I wouldn't call that 'not published', it's there in the package after all. It's just hidden after the fact on the implicit reference (a concept that doesn't work with NuGet CPVM for instance, see #3678).

I meant that if the end-user does dotnet publish, the xml doc is not being copied.
CPVM should probably be looked at again though.

I just fail to see why this needs to be in this particular place in the package.

What do you think, would be a good alternative of distributing xml docs?

It's tooling data, not something for end user projects.

But if the end-user project is F#-related tooling package, with this change, what would be the way of acquiring xmldoc?

[baronfel]

Just package it in the lib directory like you would PDBs. The SDK team is working on implementing a new flag for .NET 7 in the SDK that will let users include PDB and XML outputs of references on their built/published outputs as well, so users that need the FSharp.Core Xml can use that mechanism.

[KevinRansom]

I am not at all sure what the problem here is:

1. The xml doc files are provided so that authors who wish to redistribute this information can do so. The chosen mechanism 'nuget contentFiles' seems to have been designed and implemented for such a purpose.
2. Because redistributing this information is not the majority scenario we have turned it off by default when referencing FSharp.Core implicitly within the SDK
3. Developers who use the SDK and choose to reference FSharp.Core explicitly can use a well known and well documented nuget mechanism to disable the content. Should they choose to do so.

  <ItemGroup>
    <PackageReference Include="FSharp.Core" Version="6.0.5">
      <ExcludeAssets>ContentFiles</ExcludeAssets>
    </PackageReference>
  </ItemGroup>

4. Paket has this: 'thread through existing omitcontent setting into ExcludeAssets=contentFiles' in the SDK thread through existing omitcontent setting into ExcludeAssets=contentFiles in the SDK fsprojects/Paket#4040

It looks to me like developers have all of the tools they need to deal with this. I haven't really heard any SDK developer ask about this. The issues have tended to come from paket users, but I would have thought that with the PR in #4. paket users should be able to handle this easily too.

[KevinRansom]

@baronfel , when can we expect this new feature, and is there a link?

[vzarytovskii]

Just package it in the lib directory like you would PDBs.

Oh, I see, do you know where I can read about differences having it in lib and in contentFiles?

[NinoFloris]

It's just another workaround that adds more nonsense. Especially if you want to use NuGet central package management.
First you hit the problem that the 'implicit' reference is not allowed (because it's not actually implicit) and after fixing that you have to figure out how to get the content file removed in a decent way.

A package has a free form folder structure so I really still don't get the issue, there are like tens of tooling packages to thousands of non tooling packages.

Worst case these packages need to re-export a content file by referencing the xmldocs at the custom path by using https://docs.microsoft.com/en-us/nuget/consume-packages/package-references-in-project-files#generatepathproperty, I don't think we have to wait for an easy sdk feature to do this (as I understand it)

[vzarytovskii]

Ok, I understand now, we have 4 copies of the xml docs packed, 1 (or 2, depending on targes) of which are getting copied when explicit PackageReference is used (for example, some Paket, and CPVM scenarios).

So, in case if contentFiles is removed, then package/tools authors will have to do bundle the doc themselves, if they need it.

Questions answered

[baronfel]

@KevinRansom The issue is here (one of our oldest, most desired requests) and the PR for it is here. For the 7.0.100 release we won't default it to on, but users can explicitly request it with <CopyDocumentationFilesFromPackages>true</CopyDocumentationFilesFromPackages>. We'll likely turn this on by default in 7.0.200 based on the defaults for some other related flags, the timing was just too short for 7.0.100 to be happy with testing coverage. And in any case I'll be adding docs before release for all of these properties since they don't yet exist.

[KevinRansom]

@baronfel, okay we will remove these to align with 7.0.100 then.

[KevinRansom]

We will rely on the new (Not yet merged - but approved and implemented) sdk feature, to enable developers to ship an FSharp.Core.xml with their apps. Merging now because time is growing short.