-
Notifications
You must be signed in to change notification settings - Fork 447
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
Stop publishing of additional packages in fsharp #19404
Conversation
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>/...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update - thanks!
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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.
As a result of this change, |
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. |
We have tests which run scenarios with FSharp project templates and they're all working. |
Perfect, thank you! I just got a bit of the heebie-jeebies :) |
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: