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

[5.0.0-beta008] Paket does not add dependencies correctly #2378

Closed
0x53A opened this issue May 29, 2017 · 47 comments
Closed

[5.0.0-beta008] Paket does not add dependencies correctly #2378

0x53A opened this issue May 29, 2017 · 47 comments

Comments

@0x53A
Copy link
Contributor

0x53A commented May 29, 2017

Full zip at the end.

paket.dependencies

source https://api.nuget.org/v3/index.json

framework: net461

nuget System.ValueTuple 4.3.0
nuget FSharp.Core 4.1.0 redirects:force

image

paket.references

FSharp.Core

image

pakettest.zip

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

So I have no idea if what nuget does is even correct ...

Add FSharp.Core to a net461 project.

Nuget:

<?xml version="1.0" encoding="utf-8"?>
<packages>
  <package id="FSharp.Core" version="4.1.17" targetFramework="net461" />
  <package id="System.ValueTuple" version="4.3.0" targetFramework="net461" />
</packages>

Paket:

RESTRICTION: == net461
NUGET
  remote: https://api.nuget.org/v3/index.json
    FSharp.Core (4.1.17)
      System.Collections (>= 4.0.11)
      System.Console (>= 4.0)
      System.Diagnostics.Debug (>= 4.0.11)
      System.Diagnostics.Tools (>= 4.0.1)
      System.Globalization (>= 4.0.11)
      System.IO (>= 4.1)
      System.Linq (>= 4.1)
      System.Linq.Expressions (>= 4.1)
      System.Linq.Queryable (>= 4.0.1)
      System.Net.Requests (>= 4.0.11)
      System.Reflection (>= 4.1)
      System.Reflection.Extensions (>= 4.0.1)
      System.Resources.ResourceManager (>= 4.0.1)
      System.Runtime (>= 4.1)
      System.Runtime.Extensions (>= 4.1)
      System.Runtime.Numerics (>= 4.0.1)
      System.Text.RegularExpressions (>= 4.1)
      System.Threading (>= 4.0.11)
      System.Threading.Tasks (>= 4.0.11)
      System.Threading.Tasks.Parallel (>= 4.0.1)
      System.Threading.Thread (>= 4.0)
      System.Threading.ThreadPool (>= 4.0.10)
      System.Threading.Timer (>= 4.0.1)
    System.Collections (4.3)
    System.Console (4.3)
    System.Diagnostics.Debug (4.3)
    System.Diagnostics.Tools (4.3)
    System.Globalization (4.3)
    System.IO (4.3)
    System.Linq (4.3)
    System.Linq.Expressions (4.3)
    System.Linq.Queryable (4.3)
    System.Net.Requests (4.3)
    System.Reflection (4.3)
    System.Reflection.Extensions (4.3)
    System.Resources.ResourceManager (4.3)
    System.Runtime (4.3)
    System.Runtime.Extensions (4.3)
    System.Runtime.Numerics (4.3)
    System.Text.RegularExpressions (4.3)
    System.Threading (4.3)
    System.Threading.Tasks (4.3)
    System.Threading.Tasks.Parallel (4.3)
    System.Threading.Thread (4.3)
    System.Threading.ThreadPool (4.3)
    System.Threading.Timer (4.3)


  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="FSharp.Core">
          <HintPath>..\..\packages\FSharp.Core\lib\net45\FSharp.Core.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="mscorlib">
          <Paket>True</Paket>
        </Reference>
        <Reference Include="System.Console">
          <HintPath>..\..\packages\System.Console\lib\net46\System.Console.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="System.ComponentModel.Composition">
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="System.Numerics">
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="System.Threading.Thread">
          <HintPath>..\..\packages\System.Threading.Thread\lib\net46\System.Threading.Thread.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6.1'">
      <ItemGroup>
        <Reference Include="System.Threading.ThreadPool">
          <HintPath>..\..\packages\System.Threading.ThreadPool\lib\net46\System.Threading.ThreadPool.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>

@matthid
Copy link
Member

matthid commented May 29, 2017

omg

@matthid
Copy link
Member

matthid commented May 29, 2017

whooah strictly speaking I'd could say it's a packaging error of FSharp.Core:

  • My understanding of the "Spec" is that you basically take the best matching group, at least that's how it's used in the ecosystem.
  • For net461 the netstandard16 group is clearly the best matching one
  • Therefore you don't have the valuetype dependency.

Workaround is to add the ValueType to the reference file. IMHO this clearly shows the new restriction system is working as intended...

NOW TO NUGET:
I HAVE NO FUCKING CLUE WHAT THEY ARE DOING. It seems like they look into the extracted package to decide which dependencies they are going to take. THIS IS INSANITY.

  • They downlaod the package: See the best matching folder is net45
  • They use the fallback dependency group
  • They only download ValueType

Solution is of course to clearly specify dependencies in the nuspec.

/cc @forki

@matthid
Copy link
Member

matthid commented May 29, 2017

Or nuget prefers the "any" group to the netstandard group in that case ...

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

I think it is even more fucked up:

https://github.com/dotnet/standard/blob/master/docs/versions.md

NOTE: The table above reflects the mappings that will happen when we release the .NET Standard 2.0 tooling. You can see that .NET Framework 4.6.1 mapping is being moved from 1.4 to 2.0. For the exact mapping table for pre-.NET Standard 2.0 tooling see .NET Standard Library

So the current nuget considers Net461 to support netstandard1.4, but NOT 1.6

FYI, my test was with VS2015.2, which does not contain the netstandard2 tooling afaik.

@forki
Copy link
Member

forki commented May 29, 2017

/cc @enricosada

@matthid
Copy link
Member

matthid commented May 29, 2017

Yeah It seems to change depending on what you select on nuget version: http://nugettoolsdev.azurewebsites.net/4.0.0/get-nearest-framework?project=net461&package=netstandard16%0D%0Aany
http://nugettoolsdev.azurewebsites.net/4.3.0-beta1-2418/get-nearest-framework?project=net461&package=netstandard16%0D%0Aany

So the current nuget considers Net461 to support netstandard1.4, but NOT 1.6

I don't think that is what the table tries to say. Basically they say that 4.6.2 no longer supports 1.5

@matthid
Copy link
Member

matthid commented May 29, 2017

But I still think paket is doing the "correct" thing if net461 is considered compatible with netstandard16 (which I'm assuming from both tables).
And you can clearly see that nuget is not installing ValueType on a netstandard16 project either (just tried that).

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

@matthid
https://docs.microsoft.com/en-us/dotnet/standard/library
image

Take note of Tooling1 vs Tooling2

@matthid
Copy link
Member

matthid commented May 29, 2017

yeah but I wouldn't interpret an empty cell as not compatible....
That just doesn't make any sense from an compat point of view...

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

I do interpret it that way ...

@matthid
Copy link
Member

matthid commented May 29, 2017

Or maybe it does, and I have to rething this

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

I think I read somewhere in passing (but can't remember) that net461 does NOT support netstandard1.6 ootb, and that they would need to somehow update the tooling to fix things up.

But that means that even if paket is correct to resolve it this way, it may break people if the rest of the tools does not support net461=ns16.

@matthid
Copy link
Member

matthid commented May 29, 2017

ah okey.

We already added the mapping net461 -> netstandard20 into paket. Therefore we already are at tooling 2.0.

@matthid
Copy link
Member

matthid commented May 29, 2017

Actually, I added net46 -> netstandard20. Possibly an earlier version of the table or a typo :/

@matthid
Copy link
Member

matthid commented May 29, 2017

We could remove that mapping. But fundamentally it's a FSharp.Core packaging error that needs to be fixed.

@matthid
Copy link
Member

matthid commented May 29, 2017

https://github.com/fsprojects/Paket/blob/master/src/Paket.Core/Versioning/FrameworkHandling.fs#L384
This is what I mean, we could remove netstandard20 from this line.

@matthid
Copy link
Member

matthid commented May 29, 2017

Then your use case would work, but still fundamentally FSharp.Core is broken, the error will just be delayed until tooling 2.0 arrives or we add that back...

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

Ok, I agree that FSharp.Core is broken, so the original issue is solved.

My question is now how to handle ns16 and whether any following tooling will break if we add keep this mapping.

@matthid
Copy link
Member

matthid commented May 29, 2017

So everything isn't as bad as initially thought :)

@matthid
Copy link
Member

matthid commented May 29, 2017

My question is now how to handle ns16 and whether any following tooling will break if we add this mapping.

Not sure if it matters, there basically is not a lot of tooling atm to care about. So I think it's ok for paket to be already compat with 2.0. But I don't know everything maybe @enricosada can help there.

@matthid
Copy link
Member

matthid commented May 29, 2017

It probably is not worth it to be compatible with both (1.0 tooling and 2.0 tooling)

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

It probably is not worth it to be compatible with both (1.0 tooling and 2.0 tooling)

I would rather say it is impossible, because at resolve time you don't know which tooling will be used in the future.

@matthid
Copy link
Member

matthid commented May 29, 2017

And this fallback group is really and edge case that only matters here. For most packages they properly specify their groups and paket will prefer the right one (for example full framework groups are prefered when building for full frameworks) so I guess there only a small number of packages where this actually matters.
Additionally I'd say rather take a package (ie have an additional compat mapping as we have right now) than not beeing able to compile at all.

I hope that clarifies my view on this.

@matthid
Copy link
Member

matthid commented May 29, 2017

I would rather say it is impossible, because at resolve time you don't know which tooling will be used in the future.

We actually do know from the runtime graph (Basically if the Packaging.Tools package is there). But I don't think it's worth it

@enricosada
Copy link
Collaborator

enricosada commented May 29, 2017

I dont know if targeting net461 is better to use netstandard1.6 Or net40

I think the net40 win.

But is better ask nuget team for clarification

@enricosada
Copy link
Collaborator

enricosada commented May 29, 2017

Anyway yes, fsharp.core should restrict "System.ValueTuple"" to o lo the needed fw

I'll check tomorrows

1.0 or 2.0 is not that different.

@matthid
Copy link
Member

matthid commented May 29, 2017

any framework is not the same as net40?

Yes they are not that much different but in this case it makes a difference.

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

#2381

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

ref #2381

My vote is for disabling the ns16 <> net461 mapping until we have figured this stuff out.

@matthid
Copy link
Member

matthid commented May 29, 2017

please send a pr to modify the line I was referencing above...

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

Will do that in a few minutes... But that would once again just be a band-aid fix.

Does anyone even understand how this shit is supposed to work?

For example, I found dotnet/standard#342 and this states that you need to add NETStandard.Library explicitly. But I tried that, and it still errors out. But I am trying to compile it with VS 2017.2, so maybe you can only do that with VS2017.3?

Suddenly I feel like Java got it right by just mashing a few .class files together into one folder.

@matthid
Copy link
Member

matthid commented May 29, 2017

sry but you have a bad timing, will look into this in a couple of days...

@matthid
Copy link
Member

matthid commented May 29, 2017

I guess what matters are not the dlls in the build folder but the targets file which should be added

@0x53A
Copy link
Contributor Author

0x53A commented May 29, 2017

The non-beta version still works, so no stress. I don't know about everyone else, but I am not blocked by this issue.

PR sent: #2380

@enricosada
Copy link
Collaborator

@matthid @0x53A sorry yesterday i was on mobile, and i read badly the issue (i had no access to zip repro)

I think we would somehow need to pull in these libs - but they are from the build folder?

@0x53A That's because they are using the new msbuild extensibility. if you see, the NETStandard.Library.NETFramework.props (and .targets) who are automatically imported at restore, to add <Reference and <ReferenceCopyLocalPaths.
So these props/target files, are imported because are added in obj\{projectName}.{projectExt}.nuget.g.props (and .targets)

about this issue.

I checked and Fsharp.Core v4.1.17 for me is correct.
try use to create a lib with fsharp.core (works the same with sdk1.0 and 2.0)

<Project Sdk="Microsoft.NET.Sdk">
  <PropertyGroup>
    <TargetFramework>net461</TargetFramework>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="FSharp.Core" Version="4.1.17" />
  </ItemGroup>
</Project>

the only ref (non facade) added are:

/reference:C:\Users\e.sada\.nuget\packages\fsharp.core\4.1.17\lib\net45\FSharp.Core.dll
/reference:C:\Users\e.sada\.nuget\packages\system.valuetuple\4.3.0\lib\netstandard1.0\System.ValueTuple.dll

@0x53A @matthid so the issue is just with old sdk?

@0x53A
Copy link
Contributor Author

0x53A commented May 30, 2017

@0x53A @matthid so the issue is just with old sdk?

Dunno, I only tested old sdk.
But old sdk is there to stay for at least some time, so if this only works with new sdk, then it is ... problematic imo.

@enricosada This issue describes two problems, the first was that System.ValueTuple was not added, which afaik was concluded to be an authoring error. So closed from my side.

The second problem is that I can't actually use a netstandard1.6 library from net461, it fails at runtime.

I think this is an issue with paket, where some conditions are too strict, but can't investigate more at the moment. Will try to find out more in the evening after work.

@enricosada
Copy link
Collaborator

enricosada commented May 30, 2017

But old sdk is there to stay for at least some time, so if this only works with new sdk, then it is ... problematic imo.

Yes, i was just trying to restrict the scenario. bug is in paket code for old sdk .

This issue describes two problems, the first was that System.ValueTuple was not added, which afaik was concluded to be an authoring error

Ihmo, adding System.ValueTuple (either as transitive deps, or directly) example 4.3.0 should:

  • not add additional packages: .NETFramework 4.5 deps are empty
  • use best lib directory => lib/netstandard1.0

Both old sdk and new sdk. if not, is a bug.

As a note, Fsharp.Core 4.1.17 add System.ValueTuple as a transitive deps already, so should work OOTB, users shouldnt mess with that (or at least that was the idea)
The only known issues is atm doesnt restrict to remove it for net47

The second problem is that I can't actually use a netstandard1.6 library from net461, it fails at runtime.

@0x53A using netstandard1.6 from net461 in old sdk? That's the scenario?
Afaik that's not a supported scenario.
Old sdk shouldnt really be used to create netstandard1.6, so adding a project ref to that, afaik is not supported either.

But old sdk is there to stay for at least some time, so if this only works with new sdk, then it is ... problematic imo.

Yes, is there to stay. But for net4* and pcl and everything before.
New netstandard/netcore libraries should use new sdk, so new fsproj/csproj.
Using netstandard1.6 packages probably works, but afaik not for project reference to netstandard lib.
That said, is possibile that works, but ihmo is not a supported scenario.

@matthid
Copy link
Member

matthid commented May 30, 2017

bug is in paket code for old sdk .

I don't agree

The only known issues is atm doesnt restrict to remove it for net47

I'll give a more detailed answer in a couple of days: But the problem is the reverse. It does remove it for all frameworks which are compatible with netstandard1.6. So it WILL remove it for net47 (Or NuGet just does weird things I don't understand).

@enricosada
Copy link
Collaborator

enricosada commented May 30, 2017

So it WILL remove it for net47

Atm that dependency is in the fallback group. that mean is used for all target framework except netstandard1.6.
that's a bug for .NET 4.7 who was released two weeks ago, created fsharp/fsharp#741 for that

@0x53A
Copy link
Contributor Author

0x53A commented May 30, 2017

I have extracted the netstandard1.6 stuff into it's own issue, otherwise it get's confusing: #2381

@matthid
Copy link
Member

matthid commented May 30, 2017

@enricosada since when do framework dependency groups have to match exactly? I thought we take the dependency group which is most compatible?

For net47 netstandard16 is clearly more compatible than the fallback rule?

What are the exact rules there? Are they defined anywhere or are you just assuming (as we are)?

@matthid
Copy link
Member

matthid commented May 30, 2017

@enricosada I commented on the FSharp.Core issue, which hopefully makes my point more clear. Feel free to ping me somewhere if you have more information on this as it seems that I just can't get your point :/

@0x53A
Copy link
Contributor Author

0x53A commented Jun 2, 2017

So the dependency on System.ValueTuple was removed anyway :)
https://www.nuget.org/packages/FSharp.Core/

@0x53A 0x53A closed this as completed Jun 2, 2017
@matthid
Copy link
Member

matthid commented Jun 2, 2017

I think we need to support 4.1 for quite some time as it's the only package providing xamarin support until everything works with netstandard20...

But yeah this is not a paket bug :P

@0x53A
Copy link
Contributor Author

0x53A commented Jun 2, 2017

paket4 did not yet contain the mapping net461 <> ns16, right?
And on the beta branch we have reverted the mapping.

So even though this may be a packaging issue, it should not actually occur.

@matthid
Copy link
Member

matthid commented Jun 2, 2017

@0x53A Well as long as they don't decide to make net47 compatible with netstandard16 for tooling 1.0, yes.

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

No branches or pull requests

4 participants