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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

carlossanlop
Copy link
Member

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.
  • 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
<PackageDesktopAsRef>false</PackageDesktopAsRef>
<TargetFrameworks>netstandard2.0;$(NetFrameworkMinimum);net47;netcoreapp2.0</TargetFrameworks>
<AssemblyKey>Open</AssemblyKey>
<IsPlaceholderTargetFramework
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the first migrated package where I see netstandard2.0 as a placeholder. So the conditions here and in the Choose/When items were more complex than usual.

<VersionPrefix>4.5.0</VersionPrefix>
<VersionPrefix Condition="'$(IsPackable)' == 'true'">4.6.0</VersionPrefix>
<AssemblyVersion Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">4.0.3.0</AssemblyVersion>
<AssemblyVersion Condition="'$(IsPackable)' == 'true' and $([MSBuild]::GetTargetFrameworkIdentifier('$(TargetFramework)')) == '.NETFramework'">4.0.4.0</AssemblyVersion>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a comment here like in System.Memory and System.Threading.Tasks.Extensions indicating that the AssemblyVersion should not go above a specific number? I forgot where to get that limit from. Should it be the first version where this assembly was part of the shared framework (3.0)?

@@ -1,128 +0,0 @@
// Licensed to the .NET Foundation under one or more agreements.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file was deleted because:

  • It was only meant for netcoreapp, which we treat as a placeholder.
  • It was using BinaryFormatter.

<RunApiCompat Condition="'$(TargetGroup)' != 'netcoreapp' AND '$(TargetGroup)' != 'uap'">false</RunApiCompat>
<!-- We've explicitly authored a reference facade so we do not need to use the desktop implementation as ref -->
<PackageDesktopAsRef>false</PackageDesktopAsRef>
<TargetFrameworks>netstandard2.0;$(NetFrameworkMinimum);net47;netcoreapp2.0</TargetFrameworks>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are ApiCompat failures due to having NetFrameworkMinimum and net47, but I'm unsure how to resolve them:


API compatibility errors between 'lib/net47/System.ValueTuple.dll' (system.valuetuple.4.5.0.nupkg)
 and 'lib/net462/System.ValueTuple.dll' (System.ValueTuple.4.6.0-ci.24572.1.nupkg): 
CP0003: [Baseline] lib/net47/System.ValueTuple.dll assembly public key token 'cc7b13ffcd2ddd51'
 does not match with lib/net462/System.ValueTuple.dll '31bf3856ad364e35'. 


<PropertyGroup>
<TargetFrameworks>netstandard2.0;$(NetFrameworkMinimum);net47;netcoreapp2.0</TargetFrameworks>
<AssemblyKey>Open</AssemblyKey>
<IsPlaceholderTargetFramework
Condition="$([MSBuild]::GetTargetFrameworkIdentifier('$ (TargetFramework)')) == '.NETCoreApp'
or ($([MSBuild]::IsTargetFrameworkCompatible('$(TargetFramework)', 'netstandard2.0'))
and '$(TargetFramework)' != '$(NetFrameworkMinimum)')">true</IsPlaceholderTargetFramework>
and '$(TargetFramework)' != 'net462')">true</IsPlaceholderTargetFramework>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant that it would be better to remove the conditions from L7+L8 completely so that you only have the != net462 check.

@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?

</ItemGroup>

<ItemGroup>
<PackageReference Include="System.ValueTuple" />
</ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Delete the empty ItemGroup.

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

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.

2 participants