Include NetCoreAppCurrent configs in packages #53439

ViktorHofer · 2021-05-28T17:36:45Z

The NetCoreAppCurrent configurations were omitted from packages to avoid
ever growing packages. Now that we adhere to the support policy for
packages we don't need to exclude them anymore as the policy defines a
baseline for .NETCoreApp configurations.

This will remove an artificial difference when source building the
repository and also make it so that partner repositories which don't
depend on a transport package like windowsdesktop (winforms) receive
the very latest assets that are included in the shared framework as
well.

The NetCoreAppCurrent configurations were omitted from packages to avoid ever growing packages. Now that we adhere to the support policy for packages we don't need to exclude them anymore as the policy defines a baseline for .NETCoreApp configurations. This will remove an artificial difference when source building the repository and also make it so that partner repositories which don't depend on a transport package like windowsdesktop (winforms) receive the very latest assets that are included in the shared framework as well.

ghost · 2021-05-28T17:36:48Z

Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details

The NetCoreAppCurrent configurations were omitted from packages to avoid
ever growing packages. Now that we adhere to the support policy for
packages we don't need to exclude them anymore as the policy defines a
baseline for .NETCoreApp configurations.

This will remove an artificial difference when source building the
repository and also make it so that partner repositories which don't
depend on a transport package like windowsdesktop (winforms) receive
the very latest assets that are included in the shared framework as
well.

Author:	ViktorHofer
Assignees:	ViktorHofer
Labels:	`area-Infrastructure-libraries`
Milestone:	-

...crosoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj

ericstj

I'm OK with this. Given the size of this and potential impact be on the lookout for changes downstream. I don't expect anything to break but you never know given this is shipping a lot of configurations which may never have been seen before.

ViktorHofer · 2021-05-31T16:40:27Z

@CertifiedRice was the approval intentional?

ViktorHofer · 2021-05-31T17:04:33Z

I had to update the packageindex as a few assemblies were missing a net6.0 entry which caused unnecessary package references (even for inbox libs).

ViktorHofer · 2021-06-01T14:22:41Z

@ericstj @joperezr can you please help me figure out how to react to the package system errors? It looks like the browser-wasm RID is trying to be restored on netcoreapp2.0 and above .NETCoreApp tfms but restore can't find the runtime packs for it. Simliar for ilumos, ios and others. Wondering if this is just the first time that we are packaging one of the new RIDs into a package?

ericstj · 2021-06-01T15:44:51Z

I believe this is an issue with how the test-tuples are calculated. cc @Anipik

Previously we had a set of known TFMs, and for each TFM we had a set of RIDs we knew it supported.

Looks like now the same set of RIDs are applied to all TFMs:
https://github.com/dotnet/arcade/blob/c7d6bd607715f334cda90e01967bb0c02dee09be/src/Microsoft.DotNet.PackageTesting/GetCompatiblePackageTargetFrameworks.cs#L37-L38
So it's applying newer RIDs to older TFMs, thus these errors when validating.

That task should be changed to only apply those RIDs to the TFM of the asset they apply, or compatible TFMs (to catch a mistake where RID-specific was used in early framework, but dropped later: package would still resolve RID specific on newer framework).

ericstj · 2021-06-01T15:52:59Z

src/libraries/System.Data.Odbc/src/System.Data.Odbc.csproj

@@ -2,7 +2,6 @@
  <PropertyGroup>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <TargetFrameworks>$(NetCoreAppCurrent)-windows;$(NetCoreAppCurrent)-FreeBSD;$(NetCoreAppCurrent)-illumos;$(NetCoreAppCurrent)-Solaris;$(NetCoreAppCurrent)-Linux;$(NetCoreAppCurrent)-OSX;$(NetCoreAppCurrent)-iOS;$(NetCoreAppCurrent)-tvOS;$(NetCoreAppCurrent);netcoreapp3.1-windows;netcoreapp3.1-FreeBSD;netcoreapp3.1-Linux;netcoreapp3.1-OSX;netcoreapp3.1;netstandard2.0;net461-windows</TargetFrameworks>
-    <ExcludeCurrentNetCoreAppFromPackage>true</ExcludeCurrentNetCoreAppFromPackage>


It's really unusual for us to have this many RID-specific assets in a package. It was bad before, but this makes it worse.
Anything we can refactor to make this simpler?

Opened #53900 to discuss this in more detail.

Anipik · 2021-06-01T16:14:54Z

I think it will need to if we don't want to give folks different behavior. Once we start removing the "special" characteristic of NetCoreAppCurrent then folks will no longer try to catch differences between it and other TargetFrameworks: that's a key benefit we're getting here.

@ericstj @ViktorHofer can you elaborate on the advantages of doing this ?

Requiring accumulation of TargetFrameworks will result in some churn to the codebase, modifying conditions, ifdefs, etc. It will make turning the crank on versioning in the repo a bit harder (hopefully tool-able).

probably with the correct patterns, we should be able to reduce it. However, we should try to make the policy concrete and hopefully not change it as we spent a lot of time during last releases as well to automate the previous process.

Anipik · 2021-06-01T16:21:20Z

That task should be changed to only apply those RIDs to the TFM of the asset they apply, or compatible TFMs (to catch a mistake where RID-specific was used in early framework, but dropped later: package would still resolve RID specific on newer framework).

i can try to fix this later today.

ericstj · 2021-06-01T16:27:42Z

src/libraries/System.Text.Encodings.Web/src/System.Text.Encodings.Web.csproj

@@ -2,7 +2,6 @@
  <PropertyGroup>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <TargetFrameworks>$(NetCoreAppCurrent);$(NetCoreAppCurrent)-Browser;netcoreapp3.1;netstandard2.0;net461</TargetFrameworks>


Looks like we are including browser here just for this little ifdef:

runtime/src/libraries/System.Text.Encodings.Web/src/System/Text/Encodings/Web/OptimizedInboxTextEncoder.Ascii.cs

Lines 25 to 35 in 85fde05

#if NETCOREAPP

#if !TARGET_BROWSER

[FieldOffset(0)] // ensure same offset with AsBytes field

internal Vector128<byte> AsVector;

#else

// This member shouldn't be accessed from browser-based code paths.

// All call sites should be trimmed away, which will also trim this member

// and the type hierarchy it links to.

internal Vector128<byte> AsVector => throw new PlatformNotSupportedException();

#endif

#endif

Doesn't seem worth it to me. Also I don't like us having PNSE's buried in a library that has nothing to do with platform-specific functionality (Encoding). Shouldn't we have some runtime feature flag that can detect if the platform supports this path and avoid even going down here? @eerhardt @GrabYourPitchforks

Shouldn't we have some runtime feature flag that can detect

The issue is the [StructLayout(LayoutKind.Explicit)] attribute on this type. If the linker sees a used struct with that attribute, it can't trim any of the fields on it - since you are defining a StructLayout, you are probably using it in a P/Invoke, so trimming fields would break the P/Invoke.

Since the field can't be trimmed, it means that Vector128<T> can't be trimmed in browser-wasm.

FYI - @vitek-karas @MichalStrehovsky @marek-safar. Looks related to dotnet/linker#1309. There is some discussion there about this exact scenario.

since you are defining a StructLayout, you are probably using it in a P/Invoke, so trimming fields would break the P/Invoke.

I believe @GrabYourPitchforks did this so that he could have a union structure where he could access the same bytes as a Vector as well. Is there a way to annotate that it's safe to trim it? Alternatively the linker could identify that this is a union, where it's safe to trim a field so long as the size of the struct remains the same.

I believe @GrabYourPitchforks did this so that he could have a union structure where he could access the same bytes as a Vector as well.

Can this use Unsafe.As or similar at the call site instead of explicit Vector128 inside the struct?

Can this use Unsafe.As or similar at the call site instead of explicit Vector128 inside the struct?

The presence of Vector128 changes the alignment requirement of the struct so dropping the field and using Unsafe.As would likely result in unaligned reads.

Looks like the problem was that the presence of Vector128 regresses the WASM size by ~4kB compressed. Vector128 is more fat than it needs to be. I assume it's caused by Equals/GetHashCode/ToString having too many dependencies.

I opened #53900 to discuss this in more detail.

ViktorHofer · 2021-06-01T17:03:29Z

@ericstj @ViktorHofer can you elaborate on the advantages of doing this ?

Shipping the very latest and greatest .NETCoreApp asset (which in some cases also ships via the shared framework) guarantees that when the package evolves, optimizations that only apply to the latest version of .NETCoreApp aren't lost.

probably with the correct patterns, we should be able to reduce it. However, we should try to make the policy concrete and hopefully not change it as we spent a lot of time during last releases as well to automate the previous process.

I agree with you on that. Using the right patterns reduces churn by ~90% but the remaining 10% will need manual action:

TFM updates (could be automated by deferring to a property which specifies the NetCoreAppPrevious tfms, i.e. for net7: net6.0 and for net8 net6.0;net7.0).
References or conditions that apply to a previous not built tfm anymore. We want to clean this up with every release to not add on top of each other.

FWIW, the convergence of ref and source project would definitely help with keeping tfms in sync and reducing the number of conditions that apply to specific tfms.

I agree on that we either need documentation or make versioning of packages trivial to mimic.

ViktorHofer · 2021-06-01T17:04:01Z

i can try to fix this later today.

Thanks a lot @Anipik. I will meanwhile label this PR as blocked.

joperezr · 2021-06-01T21:33:43Z

Is there a design doc or discussion on what the plan is going forward for assets in terms of support? My questions are mostly around the following scenario: library System.Foo has configs (netstandard2.0;netcoreapp3.1,netcoreappcurrent) in the package, and then netcoreapp3.1 goes out of support. Does that mean that we drop netcoreapp3.1 asset from the package? do we add a new configuration for the dropped config +1?

ViktorHofer · 2021-06-01T22:01:05Z

Is there a design doc or discussion on what the plan is going forward for assets in terms of support?

Yes, see @GrabYourPitchforks's design doc which isn't public yet: https://microsoft.sharepoint.com/:w:/r/teams/DotNetTeam/_layouts/15/Doc.aspx?sourcedoc=%7B0176D4ED-B6F9-4C9D-95E2-DDE2BCC00688%7D&file=Addressing%20dotnet%20OOB%20servicing%20pitfalls.docx&action=default&mobileredirect=true&cid=92b19bfc-c3b6-41aa-93ab-67d2208b5946. Note that the relevant section just enumerates the package policy that is already in place but which just wasn't enforced until recently.

To provide you an example of what's going to happen for NET7 and NET8 see the following:

When main targets NET7, packages will drop the netcoreapp3.1 and net5.0 assets and contain the minimum supported .NETCoreApp configuration net6.0 (LTS) and net7.0 (Current).
When main targets NET8, .NETCoreApp packages assets won't be dropped as net6.0 (LTS) will still be supported when NET8 ships but net8.0 (Current) will be added.
When main targets NET9, net6.0 and net7.0 will be dropped, net8.0 (LTS) will be kept and net9.0 (Current) will be added.

By only including tfms which are supported by the "current band" (i.e. NET6) an infinite growth in package size is prevented. In the past NetCoreAppCurrent configurations were excluded from packages as there was no lower boundary that was enforced. Until just recently we even carried portable-* and netcore4/5 assets along even though those are out of support for quite some time.

ViktorHofer · 2021-06-08T08:01:22Z

Encountered conflict between 'Platform:System.Formats.Asn1.dll' and 'CopyLocal:C:\h\w\B0C609B1\w\AC1C095C\e\.nuget\system.formats.asn1\6.0.0-ci\lib\net6.0\System.Formats.Asn1.dll'. Choosing 'CopyLocal:C:\h\w\B0C609B1\w\AC1C095C\e\.nuget\system.formats.asn1\6.0.0-ci\lib\net6.0\System.Formats.Asn1.dll' because file version '42.42.42.42424' is greater than '6.0.21.20104'.

During ResolvePackageFileConflicts we are feeding in the shipping SDK (currently 6.0 Preview 3) with valid assembly file versions. In non official builds, the file versions are set to 42.42.42.42424, mostly for convenience. Because of that, conflict resolution prefers the copylocal asset from the nuget package instead of the inbox asset.

Opened #53854 as well to discuss this.

ViktorHofer · 2021-06-08T17:49:31Z

src/libraries/pkg/test/packageTest.targets

+          Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and
+                      '$(DisableVerifyNotDependsOnNetStandardTest)' != 'true'">


@joperezr just FYI, VerifyNotDependsOnNetStandard wasn't running for the last year as the internal properties that it depended on were removed in the SDK.

safern · 2021-06-08T20:47:12Z

To provide you an example of what's going to happen for NET7 and NET8 see the following:

I guess for packages that support NS2.0 or NS2.1, we are going to add targets that error when an out of support tfm is used to restore the package?

ViktorHofer · 2021-06-08T20:49:02Z

I guess for packages that support NS2.0 or NS2.1, we are going to add targets that error when an out of support tfm is used to restore the package?

Yes and that's already implemented (and automated). See 293d472 for more details.

ViktorHofer · 2021-06-08T21:35:51Z

Merging as CI seems to take very long today. The previous run was green and the allconfigurations leg (which is the only code path affected by the last commit) already passed.

safern · 2021-06-08T21:54:52Z

Yes and that's already implemented (and automated). See 293d472 for more details.

Awesome, thanks for sharing as I missed it because it happened while I was gone.

ViktorHofer added the area-Infrastructure-libraries label May 28, 2021

ViktorHofer requested review from ericstj and Anipik May 28, 2021 17:36

ViktorHofer self-assigned this May 28, 2021

ViktorHofer requested review from eiriktsarpalis, jozkee, layomia and steveharter as code owners May 28, 2021 17:36

ericstj reviewed May 28, 2021

View reviewed changes

...crosoft.Extensions.Logging.Abstractions/src/Microsoft.Extensions.Logging.Abstractions.csproj Show resolved Hide resolved

ericstj approved these changes May 28, 2021

View reviewed changes

CertifiedRice approved these changes May 29, 2021

View reviewed changes

Update packageindex for 6.0.0

9b64140

ViktorHofer added 2 commits June 1, 2021 08:25

Update packaging package version

57f292c

Update Microsoft.DotNet.PackageTesting version

2651a44

ViktorHofer mentioned this pull request Jun 1, 2021

Intrinsicify (Sse, Axv2, Arm64, wasm) JsonReaderHelper.IndexOfOrLessThan #41097

Closed

ViktorHofer removed request for eiriktsarpalis, steveharter, layomia and jozkee June 1, 2021 14:23

ericstj reviewed Jun 1, 2021

View reviewed changes

Anipik approved these changes Jun 1, 2021

View reviewed changes

ericstj reviewed Jun 1, 2021

View reviewed changes

ViktorHofer added the blocked Issue/PR is blocked on something - see comments label Jun 1, 2021

ViktorHofer mentioned this pull request Jun 1, 2021

Add special case for Xamarin assets to nuget precedence rules dotnet/designs#222

Closed

ericstj mentioned this pull request Jun 2, 2021

Change package testing to use RuntimeTargets rather than RID-specific restore #53575

Merged

Merge branch 'main' into DontExcludeNetCoreAppCurrentFromPkg

ec08894

ViktorHofer removed the blocked Issue/PR is blocked on something - see comments label Jun 8, 2021

Use targetingpack for package testing

8be7c3f

ViktorHofer mentioned this pull request Jun 8, 2021

Helix package testing uses LKG shared framework instead of live built #53854

Closed

ViktorHofer added 2 commits June 8, 2021 16:09

Fixes

ef8434f

Fix escaping

99c7033

ViktorHofer commented Jun 8, 2021

View reviewed changes

Fix package validation for non exposed inbox libs

54ac36f

ViktorHofer mentioned this pull request Jun 8, 2021

Use NuGet.config for restore during package testing #53837

Closed

ViktorHofer mentioned this pull request Jun 8, 2021

Review NetCoreAppCurrent libraries configurations which are platform specific #53900

Open

ViktorHofer merged commit 0377558 into dotnet:main Jun 8, 2021

ViktorHofer deleted the DontExcludeNetCoreAppCurrentFromPkg branch June 8, 2021 21:36

This was referenced Jun 9, 2021

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53948

Closed

Pipeline dependency issue: Artifact CoreCLRProduct bits missing. #53842

Closed

ViktorHofer mentioned this pull request Jun 23, 2021

Add NetCoreAppCurrent configuration to libs (Phase 1) #54544

Merged

ghost locked as resolved and limited conversation to collaborators Jul 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include NetCoreAppCurrent configs in packages #53439

Include NetCoreAppCurrent configs in packages #53439

ViktorHofer commented May 28, 2021 •

edited

Loading

ghost commented May 28, 2021

ericstj left a comment

ViktorHofer commented May 31, 2021

ViktorHofer commented May 31, 2021

ViktorHofer commented Jun 1, 2021

ericstj commented Jun 1, 2021 •

edited

Loading

ericstj Jun 1, 2021

ViktorHofer Jun 8, 2021

Anipik commented Jun 1, 2021

Anipik commented Jun 1, 2021

ericstj Jun 1, 2021

eerhardt Jun 1, 2021

eerhardt Jun 1, 2021

ericstj Jun 1, 2021

marek-safar Jun 2, 2021

MichalStrehovsky Jun 3, 2021

ViktorHofer Jun 8, 2021

ViktorHofer commented Jun 1, 2021

ViktorHofer commented Jun 1, 2021

joperezr commented Jun 1, 2021

ViktorHofer commented Jun 1, 2021

ViktorHofer commented Jun 8, 2021

ViktorHofer Jun 8, 2021

safern commented Jun 8, 2021

ViktorHofer commented Jun 8, 2021

ViktorHofer commented Jun 8, 2021

safern commented Jun 8, 2021

	#if NETCOREAPP
	#if !TARGET_BROWSER
	[FieldOffset(0)] // ensure same offset with AsBytes field
	internal Vector128<byte> AsVector;
	#else
	// This member shouldn't be accessed from browser-based code paths.
	// All call sites should be trimmed away, which will also trim this member
	// and the type hierarchy it links to.
	internal Vector128<byte> AsVector => throw new PlatformNotSupportedException();
	#endif
	#endif

		Condition="'$(TargetFrameworkIdentifier)' == '.NETFramework' and
		'$(DisableVerifyNotDependsOnNetStandardTest)' != 'true'">

Include NetCoreAppCurrent configs in packages #53439

Include NetCoreAppCurrent configs in packages #53439

Conversation

ViktorHofer commented May 28, 2021 • edited Loading

ghost commented May 28, 2021

ericstj left a comment

Choose a reason for hiding this comment

ViktorHofer commented May 31, 2021

ViktorHofer commented May 31, 2021

ViktorHofer commented Jun 1, 2021

ericstj commented Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anipik commented Jun 1, 2021

Anipik commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Jun 1, 2021

ViktorHofer commented Jun 1, 2021

joperezr commented Jun 1, 2021

ViktorHofer commented Jun 1, 2021

ViktorHofer commented Jun 8, 2021

Choose a reason for hiding this comment

safern commented Jun 8, 2021

ViktorHofer commented Jun 8, 2021

ViktorHofer commented Jun 8, 2021

safern commented Jun 8, 2021

ViktorHofer commented May 28, 2021 •

edited

Loading

ericstj commented Jun 1, 2021 •

edited

Loading