Framework restriction is lost for global build folder

[matthid]

Description

I just did some paket install on FakeLib and got the following change

+  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Sdk.props" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Sdk.props')" Label="Paket" />
+  <Import Project="..\..\..\packages\FSharp.Compiler.Tools\build\FSharp.Compiler.Tools.props" Condition="Exists('..\..\..\packages\FSharp.Compiler.Tools\build\FSharp.Compiler.Tools.props')" Label="Paket" />
-  <Choose>
-    <When Condition="$(TargetFrameworkIdentifier) == '.NETStandard' And $(TargetFrameworkVersion) == 'v1.6'">
-      <PropertyGroup>
-        <__paket__FSharp_NET_Core_Sdk_targets>FSharp.NET.Core.Sdk</__paket__FSharp_NET_Core_Sdk_targets>
-        <__paket__FSharp_NET_Current_Sdk_targets>FSharp.NET.Current.Sdk</__paket__FSharp_NET_Current_Sdk_targets>
-        <__paket__FSharp_NET_Sdk_props>FSharp.NET.Sdk</__paket__FSharp_NET_Sdk_props>
-      </PropertyGroup>
-    </When>
-  </Choose>
-  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Sdk_props).props" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Sdk_props).props')" Label="Paket" />
-  <Choose>
-    <When Condition="$(TargetFrameworkIdentifier) == '.NETStandard' And $(TargetFrameworkVersion) == 'v1.6'">
-      <PropertyGroup>
-        <__paket__FSharp_Compiler_Tools_props>FSharp.Compiler.Tools</__paket__FSharp_Compiler_Tools_props>
-      </PropertyGroup>
-    </When>
-  </Choose>
-  <Import Project="..\..\..\packages\FSharp.Compiler.Tools\build\$(__paket__FSharp_Compiler_Tools_props).props" Condition="Exists('..\..\..\packages\FSharp.Compiler.Tools\build\$(__paket__FSharp_Compiler_Tools_props).props')" Label="Paket" />
-  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Core_Sdk_targets).targets" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Core_Sdk_targets).targets')" Label="Paket" />
-  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Current_Sdk_targets).targets" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\$(__paket__FSharp_NET_Current_Sdk_targets).targets')" Label="Paket" />+  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Core.Sdk.targets" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Core.Sdk.targets')" Label="Paket" />
+  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Current.Sdk.targets" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Current.Sdk.targets')" Label="Paket" />
+  <Import Project="..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Current.Sdk.targets" Condition="Exists('..\..\..\packages\FSharp.NET.Sdk\build\FSharp.NET.Current.Sdk.targets')" Label="Paket" />

The project did no longer compile.

Known workarounds

• Revert the changes from paket in the project file...
• I reverted to 4.1.8 for now

It might or might not be related to #2234 /cc @0x53A

I think there are actually two problems

• One problem is that Paket.Core pulls these dependencies https://www.nuget.org/packages/Paket.Core/ . I don't think paket.core should depend on FSharp.NET.Sdk at all
• Second Problem is that even with the incorrect dependency paket should not add the msbuild files like this...

[0x53A]

Urgh, this is a mess:

It only depends on FSharp.NET.Sdk for .NETStandard 1.6 so you are correct that it should not be included unconditionally.

---

So that means that even if the .targets / .props is in the /build/ folder, it may need a conditional import.

I think this partition will need a small nudge:
https://github.com/fsprojects/Paket/blob/master/src/Paket.Core/Files/ProjectFile.fs#L799

@matthid what are your framework restrictions?

[matthid]

I have no restrictions in the dependencies file

[0x53A]

After downloading half the internet I can reproduce it.

The easiest fix would be to revert #2234, which would resolve this, but break #2227 again.

---

Because FSharp.NET.Sdk is only a dependency for NetStandard1.6, it has a framework restriction at install-time, even though the overall paket.dependencies does not have a framework restriction.

Because of that, the partition will incorrectly tag it as global:

So basically we would need to get the global restriction list, not the per-package restriction list, because these two may differ.

@forki, @matthid do you know how I could access the global (for the group) framework restriction from ```ProjectFile.updateReferences``?

[matthid]

Note to myself: This also means my improvement in the fix_dotnetcore branch is incorrect as well.

[0x53A]

My proposed fix would be to plumb through the framework restrictions of the group, and compare LibFolder.Targets against that.

This is based on an assumption I made in my original PR, and which I would like to get confirmed. It was also touched upon here: #2245 (comment).

I assume that if a group has a restriction, and a project uses a package of that group, then the project implicitly fits that restriction.

E.g. if I have framework: net45, then the project will only ever be compiled against net45, and I can drop all conditions for such files and import them directly.

That's what I had wanted to implement with https://github.com/fsprojects/Paket/blob/master/src/Paket.Core/Files/ProjectFile.fs#L799.

Unfortunately that didn't exactly do what I had wanted it to do, because the set it compared against was not that of the group, but that of the package, which in this case contains a framework restriction against NetStandard, even though the group itself has no such restriction.

[matthid]

I think the assumption is correct, but there is one exception: there is this paket feature where you can set conditions to a group or package. Those need to be respected...
At the same time this provides a workaround in cases where your assumption is wrong...

[0x53A]

In case both the group and the package have a restriction, the one that should be used is the intersection, right?

e.g.

group Main
framework: >= net45

nuget MyPackage framework: net462

group Main
framework: net462

nuget MyPackage framework: >= net45

should both result in using net462 for MyPackage.

Or should the package restriction override the group restriction?

[matthid]

intersection makes the most sense imho. I never understood the package specific restrictions. Especially considering the consistent world view we have...

[0x53A]

I think the restrictions are generally just a workaround to reduce download size / resolve time.
Imo, in a well authored solution, the restrictions should never cause an observable change, just like compiler optimizations.

If no one else tackles this until then I will try to code something up on the weekend ...

[0x53A]

Paket seems to already use the intersection when resolving:

paket.dependencies:

group Main

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

framework: >= net45

nuget System.ValueTuple 4.3.0
nuget paket.core framework: net462, netstandard1.6

paket.lock:

FRAMEWORK: >= NET45
NUGET
  remote: https://api.nuget.org/v3/index.json
    Chessie (0.6) - framework: net462
      FSharp.Core
    FSharp.Core (4.1.12)
      System.ValueTuple (>= 4.3)
    Mono.Cecil (0.10.0-beta5) - framework: net462
    Newtonsoft.Json (10.0.2) - framework: net462
    Paket.Core (4.5.1) - framework: net462
      Chessie (>= 0.6)
      FSharp.Core
      Mono.Cecil (>= 0.10.0-beta4)
      Newtonsoft.Json
    System.ValueTuple (4.3)

I think wrt installing the targets, we can ignore the package restrictions and just compare against the group restrictions.

My reasoning is that even in the worst case, an import may be tagged as fw-specific, even though it should be tagged global, but never the other way around.

If we wanted to also take the package restrictions into account, then we would need to walk the dependency chain(s), and look where something was imported from.
E.g. in the case of FSharp.Net.Sdk, we would need to walk up to Paket.Core, and check the restrictions of that. And then a package may be a dependency of multiple packages, all with different explicit fw-restrictions ...