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

Support group-specific dependencies #140

Closed
cdrnet opened this issue Sep 20, 2014 · 17 comments
Closed

Support group-specific dependencies #140

cdrnet opened this issue Sep 20, 2014 · 17 comments
Labels

Comments

@cdrnet
Copy link
Member

cdrnet commented Sep 20, 2014

Project A has a dependency on B, which in turn depends on TaskParallelLibrary, but only for target platform .Net 3.5. TaskParallelLibrary is an indirect dependency only and not mentioned in paket.references or or paket.dependencies (but is mentioned in paket.lock).

The nuspec dependencies of B look like the following:

<dependencies>
    <group targetFramework=".NETFramework3.5">
        <dependency id="TaskParallelLibrary" version="1.0.2856.0" />
    </group>
    <group targetFramework=".NETFramework4.0" />
</dependencies>

With Paket v0.2.26, the following references are generated in A:

  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v3.5'">
      <ItemGroup>
        <Reference Include="System.Threading">
          <HintPath>..\..\packages\TaskParallelLibrary\lib\Net35\System.Threading.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework'">
      <ItemGroup>
        <Reference Include="System.Threading">
          <HintPath>..\..\packages\TaskParallelLibrary\lib\Net35\System.Threading.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>

The first one for v3.5 seems to be correct, but the latter should not be there, as it causes TaskParallelLibrary to be referenced in v4.0 and newer as well.

See: https://github.com/mathnet/mathnet-symbolics/blob/paket/src/Symbolics/Symbolics.fsproj

Does Paket not support framework-conditional dependencies at all yet or is this merely a resolver issue?

@cdrnet
Copy link
Member Author

cdrnet commented Sep 20, 2014

Note that the nuspec reference is quite unclear if not wrong on what happens if multiple groups specify a compatible target framework. The behavior of NuGet itself is equivalent to the /lib/net40 style folders, i.e. the "best" fit wins and the others are ignored.

From the nuspec reference: "All dependencies inside a group are installed together if the target framework is compatible with the project's framework profile.". This could be interpreted in a way that all groups with a compatible target framework should be installed - and the current Paket behavior correct. However, that would make it impossible to restrict to specific versions only (= constraint) and is not how NuGet behaves.

@cdrnet
Copy link
Member Author

cdrnet commented Sep 20, 2014

Verbose Output:

$ paket install --verbose
Paket version 0.2.26.0
FsUnit 1.3.0.1 already downloaded
FSharp.Compiler.Service 0.0.44 already downloaded
MathNet.Numerics.FSharp 3.2.3 already downloaded
FParsec 1.0.1 already downloaded
Microsoft.AspNet.Razor 2.0.30506.0 already downloaded
MathNet.Numerics 3.2.3 already downloaded
FSharp.Core.Microsoft.Signed 3.1.1.1 already downloaded
FSharp.Formatting 2.4.4 already downloaded
MathNet.Numerics 3.2.3 already extracted
FsUnit 1.3.0.1 already extracted
Microsoft.AspNet.Razor 2.0.30506.0 already extracted
FSharp.Compiler.Service 0.0.44 already extracted
FParsec 1.0.1 already extracted
MathNet.Numerics.FSharp 3.2.3 already extracted
FSharp.Core.Microsoft.Signed 3.1.1.1 already extracted
FSharp.Formatting 2.4.4 already extracted
NUnit 2.6.3 already downloaded
NUnit.Runners 2.6.3 already downloaded
RazorEngine 3.3.0 already downloaded
TaskParallelLibrary 1.0.2856.0 already downloaded
NUnit 2.6.3 already extracted
NUnit.Runners 2.6.3 already extracted
TaskParallelLibrary 1.0.2856.0 already extracted
RazorEngine 3.3.0 already extracted
Installing to c:\Users\Christoph\Math.NET\mathnet-symbolics\src\Symbolics\Symbolics.fsproj
  - direct packages: [|"FParsec"; "FSharp.Core.Microsoft.Signed"; "MathNet.Numerics";
  "MathNet.Numerics.FSharp"|]
  - all packages: [|"FParsec"; "FSharp.Core.Microsoft.Signed"; "MathNet.Numerics";
  "TaskParallelLibrary"; "MathNet.Numerics.FSharp"|]
  - installing FParsec
  - custom nodes for FParsecCS ==> skipping
  - installing FSharp.Core
  - installing MathNet.Numerics
  - installing MathNet.Numerics.FSharp
  - installing System.Threading
Installing to c:\Users\Christoph\Math.NET\mathnet-symbolics\src\SymbolicsUnitTests\SymbolicsUnitTests.fsproj
  - direct packages: [|"FParsec"; "FSharp.Core.Microsoft.Signed"; "FsUnit"; "MathNet.Numerics";
  "MathNet.Numerics.FSharp"; "NUnit"|]
  - all packages: [|"FParsec"; "FSharp.Core.Microsoft.Signed"; "FsUnit"; "NUnit";
  "MathNet.Numerics"; "TaskParallelLibrary"; "MathNet.Numerics.FSharp"|]
  - installing FParsec
  - custom nodes for FParsecCS ==> skipping
  - installing FSharp.Core
  - installing FsUnit.NUnit
  - installing MathNet.Numerics
  - installing MathNet.Numerics.FSharp
  - installing nunit.framework
  - installing System.Threading
02.19s - ready.

@forki
Copy link
Member

forki commented Sep 21, 2014

omg

Does Paket not support framework-conditional dependencies at all yet or is this merely a resolver issue?

didn't know this stuff exists ;-)
will look into it.

@forki forki added the bug label Sep 21, 2014
@cdrnet
Copy link
Member Author

cdrnet commented Sep 21, 2014

Thanks!

Just noticed this is also used in the Octokit nuspec testfile where the dependency on Microsoft.Net.Http is skipped on .Net 4.5+ and .Net-Core 4.5+ but used in all other configurations. Might get ugly to map this with Choice/When in MsBuild.

@forki
Copy link
Member

forki commented Sep 21, 2014

Might get ugly to map this with Choice/When in MsBuild.

it already is a big pain....

@forki
Copy link
Member

forki commented Sep 21, 2014

boah. this thing is really really ugly.

@bartelink
Copy link
Member

Related stuff at end of #68

@forki
Copy link
Member

forki commented Sep 21, 2014

this is completely fucked up. and I don't even see a way to solve this with @ilkerde's strict mode.

@cdrnet
Copy link
Member Author

cdrnet commented Sep 21, 2014

Most cases where such monkey-patching is needed is bad packages not restricting their target platforms properly, so ideally this could be fixed there instead. But this is not always possible unfortunately.

@forki
Copy link
Member

forki commented Sep 21, 2014

after think about this:

There is no way to do this without computing the whole installation model in advance. This will probably improve some other things too. But it will take a while.

So please manually edit to:

  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v3.5'">
      <ItemGroup>
        <Reference Include="System.Threading">
          <HintPath>..\..\packages\TaskParallelLibrary\lib\Net35\System.Threading.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>

Notice I also remoded the paket flag. This will make the nodes for paket to not touch it again.

@cdrnet
Copy link
Member Author

cdrnet commented Sep 21, 2014

Yes, this will work for me as workaround. Thanks!

cdrnet added a commit to mathnet/mathnet-symbolics that referenced this issue Sep 21, 2014
@bartelink
Copy link
Member

It's interesting that there seem to be two ways of modelling conditional NuGet package dependencies -

  • either the .nuspec declares groups and then only adds a <dependency to a subset (the case you're talking about)
  • there's always a dependency, but sometimes it nulls itself out (the Microsoft.Net.Http model)

It seems the latter is more sustainable or downstream packages are not respecting the Law of Demeter (though it's tempting if System.Threading only needs to be shimmed in net35)

@cdrnet #145 and #146 address the two concerns you note re octokit

@cdrnet How about renaming this issue to "support group-specific dependencies" (or calving a new issue and closing this)

<dependencies>
    <group targetFramework=".NETFramework3.5">
        <dependency id="TaskParallelLibrary" version="1.0.2856.0" />
    </group>
    <group targetFramework=".NETFramework4.0" />
</dependencies>

@forki (or @ilkerde) Can you explain what you mean by strict mode wrt this issue? (I'd like to understand it for #154). I've come across it in the docs but to be honest they didnt speak to me and I hence wasnt able to arrive a sufficient understanding to be able to verify whether it was explained wlll or whether I'm just missing a concept (or 3!)

@cdrnet cdrnet changed the title Conditional indirect dependencies (TargetFramework) not respected Support group-specific dependencies Sep 27, 2014
@cdrnet
Copy link
Member Author

cdrnet commented Sep 27, 2014

@bartelink I don't quite understand how the mentioned Microsoft.Net.Http model works for specifying conditional dependencies (where I cannot modify the packages I depend on). How would I have to declare my dependencies in my nuspec in the following similar case (other than the dependency groups)?

  • On .Net 3.5 depend on FSharp.Core.4.3.0.0.Microsoft.Signed
  • Evereywhere else depend on FSharp.Core.Microsoft.Signed
  • Must never reference both at the same time

@bartelink
Copy link
Member

@cdrnet No, not suggesting one can always be expressed in terms of the other.

Just saying they use a Null Object pattern in some cases.

I agree your example def can not be expressed that way.

The specific thing I was referring to is that in 4.0 there is a DLL whereas in 4.5 its in core FW. They could have extracted a package for 4.0 and put in a conditional dep but they have not yet. (They have quite a few DLLs in there and arguably they should split to avoid having to put so many redundant copies of things). But obviously that leads to stacks of packages. Old DRY vs Rule of 2-3+ thing etc.

@cdrnet
Copy link
Member Author

cdrnet commented Sep 27, 2014

Ah ok, thanks for the clarification.

@forki
Copy link
Member

forki commented Nov 19, 2014

the current Paket version generates the following lockfile for your case:

NUGET
  remote: http://nuget.org/api/v2
  specs:
    FAKE (3.9.9)
    FParsec (1.0.1)
    FSharp.Compiler.Service (0.0.67)
    FSharp.Core.Microsoft.Signed (3.1.1.1)
    FSharp.Formatting (2.4.36)
      FSharp.Compiler.Service (0.0.67)
      Microsoft.AspNet.Razor (2.0.30506.0)
      RazorEngine (3.3.0)
    FsUnit (1.3.0.1)
      NUnit (>= 2.6.3)
    MathNet.Numerics (3.2.3)
      TaskParallelLibrary (>= 1.0.2856.0) - net35
    MathNet.Numerics.FSharp (3.2.3)
      MathNet.Numerics (3.2.3)
    Microsoft.AspNet.Razor (2.0.30506.0)
    NuGet.CommandLine (2.8.3)
    NUnit (2.6.3)
    NUnit.Runners (2.6.3)
    RazorEngine (3.3.0)
      Microsoft.AspNet.Razor (>= 2.0.30506.0)
    TaskParallelLibrary (1.0.2856) - net35

So it should only reference TaskParallelLibrary for .NET 3.5. Can you check if it works?

@cdrnet
Copy link
Member Author

cdrnet commented Nov 23, 2014

It seems to work, the resulting project file looks good to me! Thanks!

There is now an issue with a group-specific framework assembly reference, but I'll open a new issue on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants