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

Build migrated System.ValueTuple #186

Merged
merged 12 commits into from
Jan 9, 2025

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented Nov 22, 2024

Migrated from 2.1.

The last released version of this package (4.5.0) targeted these frameworks:

  • net461 - Actual implementation.
  • net47 - Type forwards.
  • netcoreapp2.0 - Placeholder.
  • netstandard2.0 - Placeholder.

The new version of the package will have to target:

  • net462 - With an actual implementation.
  • net47 - Type forward again.
  • net471 - Placeholder.
  • netcoreapp2.0 - Placeholder.
  • netstandard2.0 - Placeholder.

@carlossanlop carlossanlop self-assigned this Nov 22, 2024
@carlossanlop carlossanlop requested a review from a team as a code owner November 22, 2024 23:17
@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 26, 2024

Any idea why for net47 we have a type forward assembly instead of just a placeholder file like with the other TFMs?

@carlossanlop
Copy link
Member Author

Any idea why for net47 we have a type forward assembly instead of just a placeholder file like with the other TFMs?

System.ValueTuple is part of the shared framework in net461 and net47, but not in all others.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 26, 2024

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:

image

https://apisof.net/catalog/f25ce1f51db08b3f9b38d40c4b569e02

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 26, 2024

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.

@ViktorHofer
Copy link
Member

@carlossanlop I was able to fix the build. The IsPlaceholderTargetFramework condition and the public key token were incorrect.

Note that the build in general is currently broken: #192 (comment)

Comment on lines +17 to +18
<AssemblyVersion Condition="'$(IsPackable)' == 'true'">4.0.4.0</AssemblyVersion>
Copy link
Member

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?

Copy link
Member

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...

Copy link
Member

@ViktorHofer ViktorHofer Jan 9, 2025

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.

@ViktorHofer ViktorHofer merged commit 631a4e6 into dotnet:main Jan 9, 2025
5 checks passed
@ViktorHofer
Copy link
Member

Merging in. The remaining open discussion might result in a follow-up change but not necessarily.

@carlossanlop carlossanlop deleted the BuildSystemValueTuple branch January 9, 2025 22:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants