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

Change to Paket.Restore.targets breaks restore for Paket 5.174.0 #3284

Open
garfoot opened this Issue Jul 6, 2018 · 17 comments

Comments

Projects
None yet
3 participants
@garfoot

garfoot commented Jul 6, 2018

Description

Performing a build with Paket 5.174.0 of .NET Standard 2.0 projects resulted in an updated Paket.Restore.targets file. This update includes the section below from PR #3273.

true

This stops the package restore on build when using Paket 5.174.0 and breaks all future builds for .NET Standard / Core projects which require a package restore to get their references.

Repro steps

Please provide the steps required to reproduce the problem

  1. Create a new .NET Standard project with Paket 5.174.0

  2. Add packages using Paket 5.174.0

  3. Build

  4. Check NuGet references

If possible then please create a git repository with a repro sample or attach a zip to the issue.

Expected behavior

The NuGet references should be visible in VS and usable in code.

Actual behavior

Build breaks with missing references and NuGet packages are not visible.

Known workarounds

Downgrade to Paket 5.173.4.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

I think something is wrong with the formatting above. I don't understand what error you have

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

are you referring to

<PropertyGroup Condition="'$(PaketPropsVersion)' != '5.174.0' ">
  <PaketRestoreRequired>true</PaketRestoreRequired>
</PropertyGroup>

?

this should always call restore when the props files are not generated yet or are in wrong version.

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

Sorry forgot to quote out the code block so it got messed up.

I just performed a build on a project that I went home building ok last night and when I built it this morning it replaced the Paket.Restore.targets file with a new one including the statement below.

<PropertyGroup Condition="'$(PaketPropsVersion)' != '5.174.0' ">
      <PaketRestoreRequired>true</PaketRestoreRequired>
</PropertyGroup>

This seems to force the build to not restore packages if using Paket 5.174.0 on build. The behaviour I'm seeing is that subsequent build now fail as all of the NuGet packages are missing from all .NET Standard projects as though the restore step had not been performed. I'm guessing it's something to do with the change in the referenced PR.

Reverting the changes to the Paket.Restore.targets or downgrading to 5.173.4 fixes the build, but trying to build with 5.174.0 updates the Paket.Restore.targets with that change and breaks all subsequent builds.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

@garfoot can you please upload a repro? because this all works for me.

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

I'll try, unfortunately my repro has started working and I can't upload the project that's having a problem. I'm seeing the paket.props file in the obj folder but it's looking like this on the failing project

<?xml version="1.0" encoding="utf-8" standalone="no"?>
<Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
    <PropertyGroup>
        <MSBuildAllProjects>$(MSBuildAllProjects);$(MSBuildThisFileFullPath)</MSBuildAllProjects>
        <!-- <RestoreSuccess>False</RestoreSuccess> -->
    </PropertyGroup>
</Project>

I'm guessing that it's finding the props file and not restoring again even though it failed?

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

can you please nuke all /obj in the failing project and retry?

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

also: are we talking for build inside VS or build from cmd line?

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

Building in VS. I tried nuking the obj folder and it regenerated the props file ok but still wouldn't build.

I've uploaded a repro to https://github.com/garfoot/PaketTest which seems to repro on my machine. If I set Paket version to 5.174.0 it fails to build with missing references. If I set it to 5.173.4 and rebuild a couple of times it works fine.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

can you please take a look at yoir csproj. The TargetFramework should be netstandard2.0 right? did you miss the .?

/cc @matthid

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

You're right, I had missed the . from it and it fixes it. Odd that VS detects it correctly without though and compiles. I also had to go and nuke the obj folders in all of the other projects too but it's all compiling now.

Thanks for the help getting to the bottom of it, PEBKAC strikes again.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

actually I do wonder if "netstandard20" is valid, then we need to fix our props file generator

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

Well VS did seem to have no problem with it either in compiling it or showing the correct version in project properties so it could be a valid value. But new projects that I've not manually tweaked do use netstandard2.0.

I also tried netstandard16 without the . and VS detects that correctly as .NET Standard 1.6 so it's not just defaulting to 2.0.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

ok. let me fix that

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

The official list of target framework monikers here https://docs.microsoft.com/en-us/dotnet/standard/frameworks does include the . though, so whilst it works in VS I don't think it's an offically supported moniker.

@forki

This comment has been minimized.

Member

forki commented Jul 6, 2018

ok latest version should also accept without .

@garfoot

This comment has been minimized.

garfoot commented Jul 6, 2018

Great, thanks.

@matthid

This comment has been minimized.

Member

matthid commented Jul 6, 2018

Yes I don’t think it actually is a officially supposed moniker. And afaik I left it out on purpose because it didn’t work properly. we might emit a warning...

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