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

.Net 4.7 doesnt require the package System.ValueTuple #348

Closed
Rand-Random opened this issue Sep 12, 2017 · 26 comments
Closed

.Net 4.7 doesnt require the package System.ValueTuple #348

Rand-Random opened this issue Sep 12, 2017 · 26 comments

Comments

@Rand-Random
Copy link

Even though .Net 4.7 doesnt require the package System.ValueTuple and my projects are in .Net 4.7, morelinq requires the System.ValueTuple NuGet package.

@atifaziz
Copy link
Member

See #280. This was resolved in version 2.7.

@Rand-Random
Copy link
Author

Rand-Random commented Sep 12, 2017

@atifaziz Just tested it with a new Project and I end up with that:
image

So, I dont see that it was solved.

@Rand-Random
Copy link
Author

Rand-Random commented Sep 12, 2017

Also the dependencies of morelinq dont state that it has No dependencies with .Net 4.7 (not sure if it has to say it)
image

@atifaziz
Copy link
Member

@Rand-Random MoreLINQ depends on System.ValueTuple 4.4.0 and that dependency then does the right thing for .NET 4.7. I'd encourage you to read through the discussion in #280 for more background.

@Rand-Random
Copy link
Author

Rand-Random commented Sep 12, 2017

@atifaziz Sorry, but its more like you need to read it once more.

The problem was the combination .Net 4.7 and the NuGet Package System.ValueTuple inside a single project.
Where .Net 4.7 internally already has the System.ValueTuple in-build into the mscorlib and the NuGet Package providing the same namespaces/classes with an external DLL.

Both of them just simply cannot work together and you would result in having the same classes twice.

Microsoft updated the NuGet Package so that it will redirect to use the mscorlib if System.ValueTuple AND .Net 4.7 are both in use, rendering the NuGet Package System.ValueTuple useless.

You simply dont need the NuGet Package System.ValueTuple in .Net 4.7.
Microsoft was just nice enough to say, hey we fix it - that they can work together (again rendering the NuGet Package useless since it just redirects) - instead of telling every developer you need to explicitly target .Net 4.7 and say it doesnt need the dependencies.

And so, no you arent doing the right thing for .Net 4.7, the right thing to do is target 4.7 and remove the dependency.

@atifaziz
Copy link
Member

atifaziz commented Sep 12, 2017

the right thing for .Net 4.7, the right thing to do is target 4.7 and remove the dependency.

@Rand-Random Correct but as I said in the following comment in #280:

This should really be solved by the System.ValueTuple package instead of every dependent having to do it. The issue I see with adding the 4.7 target to MoreLINQ is that anyone wanting to build and contribute to the project will also need to have 4.7 installed, which is setting the bar pretty high for what seems to be an artificial requirement.

MoreLINQ doesn't have an explicit target for 4.7 so our only way to solve the issue is to exploit, as you put it, the kind favour from Microsoft.

Let me ask you, how is this bothering you today? In #280, the issue was compilation errors for people. If you're having the same issue then this is a duplicate but I don't think that's your case. If this is a kind reminder that the dependency should be removed the day we decide to add .NET 4.7 as an explicit target, then thanks (and sorry to discard this issue as a duplicate of #280); and so I'm re-opening the issue on that basis. If you think there's another way to resolve this earlier then I'm listening.

@atifaziz atifaziz reopened this Sep 12, 2017
@Rand-Random
Copy link
Author

Rand-Random commented Sep 12, 2017

Its bothering me that it adds a NuGet Package thats useless.
No, I didnt face the problem mentioned in #280.

I am not familar with deploying a NuGet Package, but cant you "just" simply state what the dependencies are? How did you remove the dependency for .Net Standard 2.0 ?

@Rand-Random
Copy link
Author

Rand-Random commented Sep 12, 2017

A quick Google search (again I have no knowledge in that topic)
but this sounds promosing.

Have a look at:
https://docs.microsoft.com/en-us/nuget/schema/nuspec

The part about "Dependency groups", where you can state:

<dependencies>
    <group>
        <dependency id="RouteMagic" version="1.1.0" />
    </group>

    <group targetFramework="net40">
        <dependency id="jQuery" />
        <dependency id="WebActivator" />
    </group>

    <group targetFramework="sl30">
    </group>
</dependencies>

So maybe you can just simply say:

<dependencies>
    <group targetFramework="net40">
        <dependency id="System.ValueTuple" />
    </group>

    <group targetFramework="net470">
    </group>
</dependencies>

And just to make clear I am not asking to change your Target Framwork of your Projects to .Net 4.7, when I previously talked about "target" I meant that the NuGet package should "target" the .Net 4.7 to state "I dont need dependency when I get installed on a .Net 4.7 project".

@atifaziz
Copy link
Member

And just to make clear I am not asking to change your Target Framwork of your Projects to .Net 4.7, when I previously talked about "target" I meant that the NuGet package should "target" the .Net 4.7 to state "I dont need dependency when I get installed on a .Net 4.7 project".

I don't think that will work. If you target the package to support .NET 4.7 but don't provide a specific build of MoreLinq.dll then it will fallback to using one targeting the highest earlier version that's still compatible. In our case, that'll be .NET 4.0 but that'll contain references to value tuple types from System.ValueTuple.dll. It will be bad if installed in an app targeting .NET 4.7 because that library will then be missing. You will need a .NET 4.7 build so that MoreLinq.dll will contain references to value tuple types from mscorlib. Since we don't want to raise the bar yet for this project, the next best thing is what System.ValueTuple 4.4.0 does. The dependency is there but for .NET 4.7 you get a System.ValueTuple.dll that simply contains type redirections (forwards) to mscorlib. This process is transparent to MoreLinq.dll and we can ship the same binary for all of 4.x. It may not be nice to see that dependency get pulled but it's a practical solution for library authors and users.

@Rand-Random
Copy link
Author

Sorry, but I dont get why this should be such big issue.
Why can you define those different dependencies:
image

And not for 4.7?!?
So how did implementing a different dependency for .NetStandard 2.0 "raise the bar" for the project but at the same time you still have a different dependency for .NetStandard 1.0, or the same question for 3.5.
Why is it no problem to have different dependencies between 3.5 and 4.0 without the "raise the bar" issue you are claiming?

@atifaziz
Copy link
Member

atifaziz commented Sep 13, 2017

@Rand-Random .NET Framework 4.0 was released some 7 years ago. .NET Framework 4.7 is an in-place replacement and released just months ago. You cannot install the two side-by-side so requiring .NET 4.7 raises the bar for anyone wanting to work on the project to have it installed (I'm not aware of a way to target later versions with targeting packs alone). I for one don't have .NET 4.7 so that's already going to be problematic. .NET Core 1.0 and 2.0 are lesser of an issue as they can be installed side-by-side and a lot more self-contained. You also have to keep in mind that we support building on non-Windows platforms so we have to also keep what all works with Mono.

@Rand-Random
Copy link
Author

After changing the following lines in MoreLinq.csproj
<TargetFrameworks>net40;net35;netstandard1.0;netstandard2.0</TargetFrameworks>

To:
<TargetFrameworks>net40;net35;netstandard1.0;netstandard2.0;net47</TargetFrameworks>

and

  <ItemGroup Condition="'$(TargetFramework)' == 'netstandard1.0' Or '$(TargetFramework)' == 'net40'">
    <PackageReference Include="System.ValueTuple">
      <Version>4.4.0</Version>
    </PackageReference>
  </ItemGroup>

To:

<ItemGroup Condition="'$(TargetFramework)' != 'net47' And ('$(TargetFramework)' == 'netstandard1.0' Or '$(TargetFramework)' == 'net40')">
  <PackageReference Include="System.ValueTuple">
    <Version>4.4.0</Version>
  </PackageReference>
</ItemGroup>

I end up with the following:
image

Notice the missing, NuGet package "System.ValueTuple" in .NETFramework 4.7.

@Rand-Random
Copy link
Author

Rand-Random commented Sep 13, 2017

Before making those changes:
I get the error msg:
image

And after the above mentioned changes, everything builds fine:
image

(Notice that it hit the breakpoint)


FYI - I added a ConsoleApp1 (.Net Framework 4.7) directly into the solution I downloaded from here and referenced MoreLinq through project referencing.
image

@Rand-Random
Copy link
Author

Can you provide this? Or do I have to manage a copy of morelinq myself with the mentioned changes?

@atifaziz
Copy link
Member

atifaziz commented Sep 13, 2017

Can you provide this? Or do I have to manage a copy of morelinq myself with the mentioned changes?

@Rand-Random It's very simple. You have .NET Framework 4.7 installed so you are able to target it for MoreLINQ. I don't so I cannot. If the extra pull of System.ValueTuple 4.4.0 is unbearable for you then I'm afraid that the quick answer is that you'll have to manage a copy/clone yourself until a time it can be addressed here.

If you want to help, here's what it'll take:

  • Make the build process (especially the scripts) work on Windows & non-Windows environments
  • Make the .NET 4.7 target optional so people without it can build the solution on their box without errors
  • Investigate if the CI servers, like AppVeyor and Travis, can build and test for .NET 4.7; this is what will enable us to publish packages off there

@fsateler
Copy link
Member

fsateler commented Sep 13, 2017

Make the .NET 4.7 target optional so people without it can build the solution on their box without errors

I think this is the key feature. Currently the build is for all targets. It can be desirable to be able to build a single target (and have travis/appveyor build the nuget packages for each target).

@atifaziz
Copy link
Member

I think this is the key feature.

@fsateler Right and I don't have the cycles right now to investigate how to make that optional across the various environments (perhaps it's not complicated but it never is on the outset 😏) and it seems like no one who participated in the discussion in #280 has a distaste for seeing a System.ValueTuple dependency. Of course, help is welcome from anyone with knowledge and time to spare.

@fsateler
Copy link
Member

Right and I don't have the cycles right now to investigate how to make that optional

@atifaziz Neither do I, and I don't have much interest either. The ValueTuple dependency does not bother me at all.

@valdisiljuconoks
Copy link

hi guys,

did you get this working? I read also #280 comments, but seems like my compilation is still failing. Targeting 4.7.1 and getting this duplicate import errors..

@atifaziz
Copy link
Member

@valdisiljuconoks Are you using MoreLINQ 2.7 or later?

@valdisiljuconoks
Copy link

I'm on 2.8

@atifaziz
Copy link
Member

@valdisiljuconoks I don't have .NET Framework 4.7.1 to verify your case quickly. It would help if you can prepare a repo with a very simple console app demonstrating your build problem in a container and share it here. This is also the wrong issue to be tracking this on so please open another. Do keep in mind also that this is not a fundamental problem with MoreLINQ so you might get more help or eyes on the problem if you report in repos related to .NET Core.

@fsateler
Copy link
Member

We now have a netstandard2.0 target. That binary does not have a ValueTuple dependency. I'm therefore closing this issue.

@igitur
Copy link

igitur commented Nov 26, 2018

A .NET 4.7.2 project will reference the .NET Framework build in a NuGet package if it's available in preference over any .NET Standard library. So even if MoreLINQ includes a netstandard2.0 build, a .NET 4.7.2 project will reference the .NET 4.5.1 build of MoreLINQ, and hence also pull in the System.ValueTuple reference. Please... we need a net47x build.

See the discussion in the comments at https://stackoverflow.com/questions/53407579/prefer-net-standard-2-0-library-reference-over-net-4-5-1-from-a-net-4-7-2-pro

@atifaziz
Copy link
Member

So even if MoreLINQ includes a netstandard2.0 build, a .NET 4.7.2 project will reference the .NET 4.5.1 build of MoreLINQ, and hence also pull in the System.ValueTuple reference.

@igitur That's correct but that System.ValueTuple reference will be specific for .NET 4.7.x that contains nothing but type forwarding hints to the real implementation in mscorlib. I understand that have a monkey-patching System.ValueTuple reference bothers people but it doesn't functionally impact anything.

See my earlier comment on why we don't have a .NET 4.7 target specifically to address this issue.

@igitur
Copy link

igitur commented Feb 21, 2020

.NET Framework 4.7 is an in-place replacement and released just months ago.

So this isn't true anymore. .NET 4.7.2 is now very ubiquitous. I don't see a reason for not providing a net472 build.

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

No branches or pull requests

5 participants