Duplicated library references in 5.151.1

[Ravadre]

Description

After patching Paket to 5.151.1 (from 5.150.0) resolution behavior changed for some reason for package System.Net.Http - it is now referenced through Paket, even though it should not be.

Repro steps

1. Create .Net Framework 4.6 project.

2. (potentially optional) Make sure, that System.Net.Http is in packet.lock file (as a depedency to something else, for example .NetStandard 2.0. Make sure that System.Net.Http is NOT inside paket.dependencies or paket.reference

3. Reference System.Net.Http through assemblies project reference (reference .net framework library version 4.0.0).

4. run paket.exe install

Expected behavior

Only 1 reference, through <Reference Include="System.Net.Http" /> should be present in *.csproj file - this basically references System.Net.Http 4.0.0 from .Net Framework.

Actual behavior

Reference in expected behavior exists, however, additional one is added from packages in form of:

<Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.6'">
      <ItemGroup>
        <Reference Include="System.Net.Http">
          <HintPath>..\packages\System.Net.Http\lib\net46\System.Net.Http.dll</HintPath>
          <Private>True</Private>
          <Paket>True</Paket>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>

References version is slightly newer, so compilation fails because 2 libraries with same types are references.

Known workarounds

Downgrade to packet version 5.150.0.

[forki]

this was done in https://github.com/fsprojects/Paket/pull/3107

@0x53A @matthid I knew this would only take couple of hours to pop up

[Ravadre]

Hi,

I read through conversion in #3107 as it looked relevant. However, I don't get I think the extent of changes based on the conversation. From what I understood from "after" section - it should be ok to have duplicate references (compiler will take higher version - not working in my case) and this should be relevant to people who had referenced .net framework library and then added same reference through Paket (which I am explicitly not doing - hence not having anything in dependencies or references files which touches System.Net.Http).

I am probably mis understanding what has changed from conversation in #3107?

[forki]

ok reverting for now since I have to sign off for today. @0x53A can you please look at the case? I can then merge again tomorrow

[matthid]

@0x53A @matthid I knew this would only take couple of hours to pop up

This is what I meant with it was an active decision to do it as it was before, we just didn't search for the old issue ;)

@Ravadre what errors do you get? Can you post for reference?

[Ravadre]

I can confirm that with reverted changes (5.151.3) it works fine.

@matthid We noticed the problem on our CI server, where we're running new paket with every build. Problem can be pinpointed to basically System.Net.Http package added to references in csproj (the choose section in actual behavior in OP was basically added between what we have after paket.exe install with older version (locally) and CI version (newer paket.exe). This as a result made compiler errors, which can be pinpointed to this one error:

Currencies\FixerApiClient.cs(16, 33): error CS0433: The type 'HttpClient' exists in both 'System.Net.Http, Version=4.1.1.2, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a' and 'System.Net.Http, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'

Multiplicated many times for every usage of a type from System.Net.Http.

I can try to create a small repro project if it will help you.

[Ravadre]

@matthid I thought it might be useful anyway :) :

https://github.com/Ravadre/paket-151_1-repro

If you will download paket.exe 5.151.1 with .paket\paket.bootstrapper.exe 5.151.1 it should fail to build the project.

There are probably smaller number of packages which can trigger this behavior added to paket.dependencies (I took this list from our smallest project which broke on CI) - I cannot dive into it more, altough list is small enough I think. I guess the problem can be somewhere in paket.lock with NETStandard.Library (2.0.1) - which references few packages, System.Net.Http (4.3.2) being one of it.

With paket 5.151.1, after paket install command you will see double references in csproj as described in OP.

I hope that helps :)

[0x53A]

@Ravadre sorry, for breaking you.

Please note that the "old" behavior is really dangerous - you (implicitly) reference the nuget package System.Net.Http, which contains a dll ....

But that Dll is missing from /bin!

I thought that MSBuild would resolve these duplicated references to warnings, but apparently that was wrong.

@forki @matthid I would suggest to do the same as nuget, and just remove the FW-Reference. What do you think?

[Ravadre]

Hi @0x53A .

I think that the library is missing from bin, because if it references as a GAC library (the 4.0.0) version. The one that is included by Paket in 5.151.1 version is newer (4.1.XX) and would be put into bin as a package - but what I think matters here is, as you said "implicitly".

An user could implicitly target such problematic library (Net.Http or similar) without knowing it and Paket would assume that it need to copy it. If the reference would be explicit (in paket.references) I would just remove the GAC reference.

To be honest, solving this issue is pretty simple: once I knew what is going on (implicit reference included problematic library in project) I could have just included it explicitly and then removing GAC reference, which would (probably) work. - I posted a ticket because I knew that for some people this implicit include would be a mystery and I wanted you guys to know about it ASAP before more people would start banging on the doors ;-).

I guess the 5.151.1 behavior is not that wrong, but it's kind of "hidden". Out of 2 solutions (remove GAC by Paket or not include implicitly targeted libraries (instead, force them to be explicitly targeted in references) I would probably lean towards 2nd one (as 1st one will change people's GAC references without letting them know) - it for for you guys to decide though.

[0x53A]

It is not related to whether the reference is implicit or explicit. Rather the deciding factor currently is just whether the project file already contains a GAC reference or not (<Reference Include="System.Net.Http" />). So you would get the same bug with an explicit reference.

Yes, the "fix" is to remove the GAC-Reference and rerun paket install. Then it will add the correct reference to the nuget version. You don't need to add it explicitly to the paket.references.

5.151.1 is definitely wrong, because twice referenced causes an error, I thought it would just cause a warning.

Imo the behavior of paket < 5.151.1 (and now after the revert) is also wrong, but the question is how it can be remedied without causing too much breakage. Maybe wait for the next major version?

[Ravadre]

Just to clarify - I didn't mean that explicit / implicit would behave differently. I just meant that if I (or more broadly - any user) would explicitly reference the problematic library, I would know in case of compilation error what I did wrong (reference same library through GAC and through Paket), in case of implicit reference - one can be surprised and don't know from where the error came.

Btw, is possible to inject a compiler error through Paket.targets? Maybe a solution would be to, in case of seeing both GAC referenced library and a different version referenced by Paket to include both - reference newer package (which will break compilation), but also include some tag, for example <DuplicateReference packageName="System.NetHttp"></DuplicateReference> which would add compiler error with a message to signal user what's wrong? Something like "One of your packages included {packageName} implicitly, you have however referenced older version through GAC. Consider adding explicit reference to {packageName} in paket.references and remove GAC reference to include only newer version of the library". - Just a thought, if possible to achieve?

[matthid]

Maybe wait for the next major version?

We either have removing existing references, leave it as is or break (but then a better error message is needed).

It is unlikely that we remember this issue when we decide to release a new major version.

[matthid]

Btw, is possible to inject a compiler error through Paket.targets?

We can emit an error on paket install time.