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

Stop publishing of additional packages in fsharp #19404

Merged
merged 4 commits into from
Apr 10, 2024

Conversation

NikolaMilosavljevic
Copy link
Member

Fixes: dotnet/source-build#4203

This is a follow up on the issue discovered in original fsharp PR: dotnet/fsharp#16838. We had to add additional packages for publishing, to match previous VMR experience. Without these packages, we were getting 2 prebuilts.

The issue with prebuiilts was a bug in prebuilt detection infra. We were discovering all *.nupkg in NuGet package cache. This is not working fine due to several *.nupkg carried as content in Microsoft.FSharp.Compiler package.

The fix is to modify glob pattern to only discover restored packages, and nothing that is carried as content.

Here's the partial NuGet cache layout showing the root cause of this issue:

├── microsoft.fsharp.compiler
│   └── 12.8.300-beta.24203.1
│       ├── contentFiles
│       │   ├── Release
│       │   │   ├── FSharp.Compiler.Service.43.8.300.nupkg
│       │   │   └── FSharp.Core.8.0.300.nupkg
│       │   └── Shipping
│       │       ├── FSharp.Compiler.Service.43.8.300-preview.24203.1.nupkg
│       │       └── FSharp.Core.8.0.300-beta.24203.1.nupkg
│       └── microsoft.fsharp.compiler.12.8.300-beta.24203.1.nupkg
└── microsoft.identitymodel.abstractions
    └── 7.4.1
        └── microsoft.identitymodel.abstractions.7.4.1.nupkg

carried as content, i.e. in Microsoft.FSharp.Compiler nupkg, and cause false positives.
Content nupkgs were not restored and should not be used in prebuit detection.
-->
<RestoredPackageFile Include="$(NuGetPackageRoot)*/*/*.nupkg"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an example of a path in the above description to better understand the glob.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I.e. something like <packageid>/<packageversion>/...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will update - thanks!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 9744c07

carried as content, i.e. in Microsoft.FSharp.Compiler nupkg, and cause false positives.
Content nupkgs were not restored and should not be used in prebuit detection.
-->
<RestoredPackageFile Include="$(NuGetPackageRoot)*/*/*.nupkg"
Copy link
Member

@ViktorHofer ViktorHofer Apr 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if a nuget package includes another nuget packages in the package root? Would that then still be included by this glob? Can we make the glob more specific?

I guess it's hard to get this right. Given that there shouldn't be any packages that are doing this, maybe it's OK?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm not sure if that would be easy, but I agree that this solution isn't perfect. We've only ever seen this single FSharp package carrying other nupkgs as content.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left one comment about making the xml comment clearer. The other one is just a thought that I had that shouldn't block this PR.

@NikolaMilosavljevic NikolaMilosavljevic merged commit 8f9999e into dotnet:main Apr 10, 2024
22 checks passed
@mthalman
Copy link
Member

As a result of this change, FSharp.Compiler.Service and FSharp.Core are no longer showing up in the source built artifacts. Is that correct?

@baronfel
Copy link
Member

Does that impact the building of f# projects using source built SDKs? The F# targets insert an implicit PackageReference to FSharp.Core (you can think of this like a FrameworkReference almost) that is required to compile and run F# code.

@mthalman
Copy link
Member

We have tests which run scenarios with FSharp project templates and they're all working.

@baronfel
Copy link
Member

Perfect, thank you! I just got a bit of the heebie-jeebies :)

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.

Stop publishing additional packages in fsharp
5 participants