-
Notifications
You must be signed in to change notification settings - Fork 11
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
Build migrated System.ValueTuple #186
Conversation
Any idea why for |
System.ValueTuple is part of the shared framework in net461 and net47, but not in all others. |
System.ValueTuple isn't part of .NET Framework 4.6.1. It got moved inbox with 4.7: |
Spoke with @ericstj offline. System.ValueTuple got moved inbox with 4.7 but into mscorlib.dll, therefore type forwards are needed. Later with 4.7.1 a System.ValueTuple facade assembly got added inbox. For the new package, net462 has the real implementation, net47 should be a typeforwards assembly and (optionally, but IMO makes sense) net471 should be a placeholder file. |
@carlossanlop I was able to fix the build. The Note that the build in general is currently broken: #192 (comment) |
<AssemblyVersion Condition="'$(IsPackable)' == 'true'">4.0.4.0</AssemblyVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ericstj I fail to understand the upper limit that we should keep in mind for .NET Framework (this only applies to the net462 and net47 TFMs above, everything else doesn't compile and is just a placeholder).
The System.ValueTuple facade that got introduce in net471 has the following version:
C:\Users\vihofer.nuget\packages\microsoft.netframework.referenceassemblies.net471\1.0.3\build.NETFramework\v4.7.1\Facades\System.ValueTuple.dll -> [assembly: AssemblyVersion("4.0.2.0")]
Do we actually have any constraints here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep it <= the inbox version. That way folks will never get a reference to a higher assembly version than the one that's eventually inbox. That said, it seems the package already shipped I higher version (4.0.3.0)
I think that might cause a warning from RAR when folks targeted a newer framework and referenced an assembly built against an older framework (if it had a higher reference than the framework assembly). You could try this net4.8 project -> net4.6.2 project, using valuetuple in public API. It's also possible that the "AutoUnify" flag on RAR causes it to ignore the difference -- my memory is fuzzy without testing it.
It wouldn't cause any runtime problems, since the NETFx framework unification will just ignore last two parts of the version and load the framework copy 4.0...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, it seems the package already shipped I higher version (4.0.3.0)
Exactly. That's the part that was especially confusing to me.
You could try this
I couldn't get a warning produced even with AutoUnify (AutoUnifyAssemblyReferences
) turned off. The handle package file conflicts task emits the following expected output:
Encountered conflict between 'Reference:C:\Users\vihofer.nuget\packages\system.valuetuple\4.5.0\ref\net47\System.ValueTuple.dll' and 'Platform:System.ValueTuple.dll'. Choosing 'Reference:C:\Users\vihofer.nuget\packages\system.valuetuple\4.5.0\ref\net47\System.ValueTuple.dll' because AssemblyVersion '4.0.3.0' is greater than '4.0.2.0'.
Encountered conflict between 'Platform:System.ValueTuple.dll' and 'CopyLocal:C:\Users\vihofer.nuget\packages\system.valuetuple\4.5.0\lib\net47\System.ValueTuple.dll'. Choosing 'CopyLocal:C:\Users\vihofer.nuget\packages\system.valuetuple\4.5.0\lib\net47\System.ValueTuple.dll' because AssemblyVersion '4.0.3.0' is greater than '4.0.2.0'.
So the package asset wins over the platform assembly in the app which is expected because of the higher assembly version.
Here's my sample: valuetup.zip
FWIW the added placeholder TFM for net471
should fix this.
Merging in. The remaining open discussion might result in a follow-up change but not necessarily. |
Migrated from 2.1.
The last released version of this package (4.5.0) targeted these frameworks:
The new version of the package will have to target: