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

CollectPackageReferences in .NET Core 2.0.2 results in Paket failing to restore the dotnet-fable tool in fable-suave-scaffold #2784

Closed
cartermp opened this Issue Sep 21, 2017 · 9 comments

Comments

Projects
None yet
4 participants
@cartermp

cartermp commented Sep 21, 2017

Description

From @rrelyea in our internal investigation thread:

CollectPackageReferences was implemented by VS/ProjectSystem in 15.0.
They asked NuGet commandline to start respecting it, as a hook for customer scenarios (including paket).

For the 15.4 timeframe, we’ve added that support.

In .NET Core 2.0.0 – Paket downloaded many nupkgs and put them in the user package folder. However, they just put the .nupkg there, they didn’t extract it. It still worked because NuGet redownloaded all the packages and cleaned up their non-extracted install.

In .NET Core 2.0.2 – NuGet now is respecting CollectPackageReferences, which enables Paket to clear NuGet sources via the nuget.config that they pass us. This means we cannot go download things the 2nd time to make it all work.

This looks like the correct behavior from NuGet.

Paket is trying to prevent NuGet from downloading twice, despite the fact that they downloaded them improperly the first time.

Repro steps

On Windows (I haven't tested this on macOS or Linux)

  1. Install .NET Core 2.0.2

  2. Clone fable-suave-scaffold

  3. Instal Node LTS and Yarn latest

  4. Run build.cmd

Expected behavior

Success.

Actual behavior

Failure (paths are from a test VM, but this will repro on a local machine).

C:\Users\ddltd1\Documents\fable-suave-scaffold-master\src\Client\Client.fsproj : error NU1100: Unable to resolve 'dotnet-fable (>= 1.2.2)' for '.NETCoreApp,Version=v2.0'.

C:\Program Files\dotnet\sdk\2.0.2-vspre-006963\NuGet.targets(102,5): error : Value cannot be null.\r [C:\Users\ddltd1\Documents\fable-suave-scaffold-master\src\Client\Client.fsproj]

C:\Program Files\dotnet\sdk\2.0.2-vspre-006963\NuGet.targets(102,5): error : Parameter name: path [C:\Users\ddltd1\Documents\fable-suave-scaffold-master\src\Client\Client.fsproj]

Running build failed.

Error:

System.Exception: dotnet restore failed

   at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1379.Invoke(String message)

   at FSI_0005.Build.runDotnet(String workingDir, String args)

   at FSI_0005.Build.clo@137-11.Invoke(Unit _arg9)

   at Fake.TargetHelper.runSingleTarget(TargetTemplate`1 target) in D:\code\FAKE\src\app\FakeLib\TargetHelper.fs:line 626

Impact

This will impact fable-suave-scaffold on .NET Core moving forward. The first time people could encounter this en-masse will be when VS 2017 15.4 releases.

Known workarounds

Use 2.0.0 first, then switch back to 2.0.2.

Relevant info

NuGet/Home#4967
dotnet/project-system#653

cc @emgarten and @davkean as well.

@cartermp cartermp changed the title from Change in NuGet to respect CollectPackageReferences in .NET Core 2.0.2 breaks Paket's usage in fable-suave-scaffold to CollectPackageReferences in .NET Core 2.0.2 results in Paket failing to restore the dotnet-fable tool in fable-suave-scaffold Sep 21, 2017

@forki forki self-assigned this Sep 21, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 21, 2017

Member

good news: I can repro.
It looks like we indeed don't extract cli tools. All other packages are extracted.

I consider this a bug in paket. Thanks for reporting!

Member

forki commented Sep 21, 2017

good news: I can repro.
It looks like we indeed don't extract cli tools. All other packages are extracted.

I consider this a bug in paket. Thanks for reporting!

@forki forki added the bug label Sep 21, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 21, 2017

Member

It looks like we extracted it to .tools:
image

but not to the main folder:

image

Member

forki commented Sep 21, 2017

It looks like we extracted it to .tools:
image

but not to the main folder:

image

@forki forki closed this in 4c3e953 Sep 21, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 21, 2017

Member

this is fixed in paket 5.98
in order to test it you need to nuke the paket-files folder inside the project so that paket restores again.
Thanks again for reporting.

I still think the NuGet team should try to improve the error message.

Member

forki commented Sep 21, 2017

this is fixed in paket 5.98
in order to test it you need to nuke the paket-files folder inside the project so that paket restores again.
Thanks again for reporting.

I still think the NuGet team should try to improve the error message.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki
Member

forki commented Sep 21, 2017

related: NuGet/Home#5913

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Sep 21, 2017

I still think the NuGet team should try to improve the error message.

Do you have a suggestion on what the error message should be here?

In this scenario NuGet restore finds a corrupted install, zero sources, and has no idea that another tool has modified all of these things are not the user. The current NU1100 message is shown when a package cannot be found and no sources exist, otherwise the sources would be listed along with the versions that were found.

emgarten commented Sep 21, 2017

I still think the NuGet team should try to improve the error message.

Do you have a suggestion on what the error message should be here?

In this scenario NuGet restore finds a corrupted install, zero sources, and has no idea that another tool has modified all of these things are not the user. The current NU1100 message is shown when a package cannot be found and no sources exist, otherwise the sources would be listed along with the versions that were found.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Sep 21, 2017

Member

@emgarten Why is it trying to hide that it found a corrupt install?

Member

matthid commented Sep 21, 2017

@emgarten Why is it trying to hide that it found a corrupt install?

@emgarten

This comment has been minimized.

Show comment
Hide comment
@emgarten

emgarten Sep 21, 2017

Why is it trying to hide that it found a corrupt install?

I wouldn't say it is trying to hide the state. For NuGet restore the package is downloaded to the http cache, validated there, and retries occur as needed. If there are issues warnings or errors are displayed and the global packages folder remains untouched.

Once the package is completely downloaded and validated it is then extracted to the global packages folder and files are moved into place while the package is locked to avoid conflicts. If there is a corrupt install it will be fixed at this point by replacing it.

A warning about finding a previous corrupted install could be useful, however under normal circumstances this isn't something that the user needs to act on. Restore fixes the problem. In this case the primary problem is that the package cannot be found to install, and the user is shown that message.

emgarten commented Sep 21, 2017

Why is it trying to hide that it found a corrupt install?

I wouldn't say it is trying to hide the state. For NuGet restore the package is downloaded to the http cache, validated there, and retries occur as needed. If there are issues warnings or errors are displayed and the global packages folder remains untouched.

Once the package is completely downloaded and validated it is then extracted to the global packages folder and files are moved into place while the package is locked to avoid conflicts. If there is a corrupt install it will be fixed at this point by replacing it.

A warning about finding a previous corrupted install could be useful, however under normal circumstances this isn't something that the user needs to act on. Restore fixes the problem. In this case the primary problem is that the package cannot be found to install, and the user is shown that message.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 21, 2017

Member

after clear cache with 5.98 we now have new issues:

image

Member

forki commented Sep 21, 2017

after clear cache with 5.98 we now have new issues:

image

@forki forki reopened this Sep 21, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Sep 21, 2017

Member

ok scratch that. I didn't follow my own advice:

in order to test it you need to nuke the paket-files folder inside the project so that paket restores again.

Member

forki commented Sep 21, 2017

ok scratch that. I didn't follow my own advice:

in order to test it you need to nuke the paket-files folder inside the project so that paket restores again.

@forki forki closed this Sep 21, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment