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

SDK doesn't find/use bundled library-packs when additional NuGet sources are specified. #44547

Closed
tmds opened this issue Oct 31, 2024 · 14 comments · Fixed by dotnet/fsharp#18000 · May be fixed by #44863
Closed

SDK doesn't find/use bundled library-packs when additional NuGet sources are specified. #44547

tmds opened this issue Oct 31, 2024 · 14 comments · Fixed by dotnet/fsharp#18000 · May be fixed by #44863
Assignees
Labels
Area-NetSDK untriaged Request triage from a team member

Comments

@tmds
Copy link
Member

tmds commented Oct 31, 2024

The .NET installation comes with a bundled FSharp.Core package that lives at: ./sdk/<version>/FSharp/library-packs/FSharp.Core.<version>.nupkg.

This package is NOT found when a user specifies RestoreAdditionalProjectSources.

This behavior results in restore errors with source-built SDKs (for versions that are not yet published on nuget.org), and causes the SDK to use binary assets from nuget.org instead of source-built assets bundled with the SDK.

Reproducer (with source-built SDK for upcoming 9.0.100):

rm -rf ~/.nuget
dotnet new console --no-restore -o console --language f#
cd console
dotnet restore console.fsproj /p:RestoreAdditionalProjectSources=file:///tmp

Error:

    /tmp/console/console.fsproj : error NU1102: 
      Unable to find package FSharp.Core with version (>= 9.0.100)
        - Found 99 version(s) in nuget.org [ Nearest version: 9.0.100-beta.24466.6 ]
        - Found 0 version(s) in /tmp

Expected behavior:

The library-packs folder under /sdk/<version>/FSharp/library-packs/ should be used also when RestoreAdditionalProjectSources is set.

cc @baronfel @MichaelSimons @omajid @Swapnali911 @Vishwanatha-HD

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-NuGet untriaged Request triage from a team member labels Oct 31, 2024
@tmds
Copy link
Member Author

tmds commented Oct 31, 2024

The issue also affects the $DOTNET_ROOT/library-packs directory.

For NativeAOT with the bundled ILCompiler:

$ dotnet restore /p:RestoreAdditionalProjectSources=file:///tmp
    /tmp/console/console.csproj : error NU1102: 
      Unable to find package Microsoft.DotNet.ILCompiler with version (>= 9.0.0)
        - Found 56 version(s) in nuget.org [ Nearest version: 9.0.0-rc.2.24473.5 ]
        - Found 0 version(s) in /tmp
    /tmp/console/console.csproj : error NU1102: 
      Unable to find package Microsoft.NET.ILLink.Tasks with version (>= 9.0.0)
        - Found 34 version(s) in nuget.org [ Nearest version: 9.0.0-rc.2.24473.5 ]
        - Found 0 version(s) in /tmp

Restore failed with 2 error(s) in 4.5s

vs

$ dotnet restore                                               
Restore complete (3.7s)

Build succeeded in 5.0s

@tmds tmds changed the title SDK doesn't find/use bundled FSharp.Core nuget package when additional NuGet sources are specified. SDK doesn't find/use bundled library-packs when additional NuGet sources are specified. Oct 31, 2024
@tmds
Copy link
Member Author

tmds commented Oct 31, 2024

The functional impact of this issue is limited because all nuget files we currently include with the source-build SDK are also shipped by Microsoft through nuget.org.

The issue should still be fixed because it nullifies the why these packages are included with the SDK: use source-built packages (source-built SDK), use bundled F# (source-built and Microsoft-built SDK).

@baronfel
Copy link
Member

That's going to be hard because by setting RestoreAdditionalProjectSources at the command line you've pinned the value of that property and it can no longer be changed/updated by MSBuild logic run during the build. The property is used directly my the NuGet targets. This is a common quasi-anti-pattern - it's like we need some other property to set so that the real property doesn't get unfortunately pinned.

@tmds
Copy link
Member Author

tmds commented Oct 31, 2024

it's like we need some other property to set so that the real property doesn't get unfortunately pinned.

Yes, to fix this we need a separate (internal) property that the SDK can use to bring these library-packs directories to the NuGet restore operation.

Copy link
Contributor

Thanks for creating this issue! We believe this issue is related to NuGet tooling, which is maintained by the NuGet team. Thus, we closed this one and encourage you to raise this issue in the NuGet repository instead. Don’t forget to check out NuGet’s contributing guide before submitting an issue!

If you believe this issue was closed out of error, please comment to let us know.

Happy Coding!

@tmds
Copy link
Member Author

tmds commented Oct 31, 2024

If you believe this issue was closed out of error, please comment to let us know.

@baronfel is this something you triggered? If not, can you re-open the issue?

@baronfel
Copy link
Member

No, it's a bot that operates on any issue with the 'Area-NuGet' label 😡

@tmds
Copy link
Member Author

tmds commented Nov 6, 2024

The suggested change is to track library-packs in a separate property from RestoreAdditionalProjectSources (for example: ImplicitLibraryPacksFolders) to avoid the unintended effect that setting RestoreAdditionalProjectSources can make functionality that is bundled with the SDK unavailable.

The root library-packs folder is being added here:

<RestoreAdditionalProjectSources Condition="Exists('$(_WorkloadLibraryPacksFolder)')">$(RestoreAdditionalProjectSources);$(_WorkloadLibraryPacksFolder)</RestoreAdditionalProjectSources>

The fsharp one here:

https://github.com/dotnet/fsharp/blob/72cf286e02160a9df6478785ca0ec1a2dbf2eeac/src/FSharp.Build/Microsoft.FSharp.NetSdk.targets#L57

note: both are guarded by DisableImplicitLibraryPacksFolder != true.

NuGet picks them up here:

https://github.com/NuGet/NuGet.Client/blob/b34beb6039fba765b922a4b7d654a8cacb04185b/src/NuGet.Core/NuGet.Build.Tasks/NuGet.targets#L836

I can look into implementing this. @dsplaisted, @baronfel wdyt?

@baronfel
Copy link
Member

baronfel commented Nov 6, 2024

This kind of safe-property-buffering seems reasonable to me.

@dsplaisted
Copy link
Member

I think this would be good to change, although maybe there's a remote chance it could break someone somehow.

If we do fix it, it might be better to set TreatAsLocalProperty="RestoreAdditionalProjectSources" in the Project element for the .targets file where we update this property, rather than inventing an entirely new property.

@nkolev92 @zivkan How do you feel about RestoreAdditionalProjectSources? Should people be using NuGet.config files instead?

@nkolev92
Copy link
Contributor

nkolev92 commented Nov 6, 2024

RestoreAdditionalProjectSources is one of those properties that was added for the SDK to be able to add sources in addition to what the user specified themselves.

I think it'd be ideal if anyone outside of the SDK did not use this property.
There's already 2 other ways to provide sources. We can clarify the documentation with those details.

@marcpopMSFT
Copy link
Member

@nkolev92 is that documented anywhere? Do we know if library-packs work with the other methods of providing sources?

@tmds
Copy link
Member Author

tmds commented Nov 13, 2024

it might be better to set TreatAsLocalProperty="RestoreAdditionalProjectSources" in the Project element for the .targets file where we update this property, rather than inventing an entirely new property.

Sounds good. I didn't know about TreatAsLocalProperty.

Should people be using NuGet.config files instead?

RestoreAdditionalProjectSources enables to configure the sources using MSBuild (and use conditions, ...) while NuGet.config is static. These are not direct alternatives.

I think it'd be ideal if anyone outside of the SDK did not use this property.
There's already 2 other ways to provide sources. We can clarify the documentation with those details.

It seems to be widely used outside the SDK at this point.

The property is documented at https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#restore-properties.

Do we know if library-packs work with the other methods of providing sources?

I think all the properties for providing sources to NuGet are part of that document. This means that to the users none of these are "SDK only".

@nkolev92 what do you think about adding the TreatAsLocalProperty to the target files? Is this solution ok for you?

@nkolev92
Copy link
Contributor

nkolev92 commented Nov 13, 2024

I think adding it to TreatAsLocalProperty is fine.

It is documented there, but I'd argue that it's one of those that needs a better explanation because of the overriding behavior of MSBuild properties. We can do better there; I've filled an issue: NuGet/Home#13922

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-NetSDK untriaged Request triage from a team member
Projects
None yet
5 participants