Update the S.T.E.W configurations to explicitly target a versioned TFM (nc3.0). by ahsonkhan · Pull Request #42069 · dotnet/corefx

ahsonkhan · 2019-10-23T22:04:01Z

The assets within the package on 3.0 were:
ns1.0, 2.0, 2.1

With this change (in master) we are adding a new asset:
ns1.0, 2.0, 2.1, nc3.0

Without this change, the new asset was targeting nc5.0 which isn't what we want.

Addresses #41933 (comment)

cc @ericstj, @ViktorHofer, @Anipik

(nc3.0).

ahsonkhan · 2019-10-23T22:13:16Z

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

    <RootNamespace>System.Text.Encodings.Web</RootNamespace>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
-    <Configurations>netcoreapp-Debug;netcoreapp-Release;netstandard-Debug;netstandard-Release;netstandard2.1-Debug;netstandard2.1-Release</Configurations>
+    <Configurations>netcoreapp3.0-Debug;netcoreapp3.0-Release;netstandard-Debug;netstandard-Release;netstandard2.1-Debug;netstandard2.1-Release</Configurations>


Continuing to use TargetsNetCoreApp within item group conditions is fine, even with this change, correct?

corefx/Directory.Build.props

Line 149 in 3b6ec2b

<TargetsNetCoreApp Condition="$(TargetFramework.StartsWith('netcoreapp'))">true</TargetsNetCoreApp>

What about using the SDK constant in source for if/defs? Should these be changed to TFM specific (i.e. NETCOREAPP3_0)? I don't see any other use of that in the repo, but maybe this is the first instance. I see older TFMS though.

corefx/src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/TextEncoder.cs

Lines 13 to 16 in bdd3397

#if NETCOREAPP

using System.Runtime.Intrinsics;

using System.Runtime.Intrinsics.X86;

#endif

Yes using TargetsNetCoreApp is fine. On the defines: in master I believe we use the SDK pattern where netcoreapp means any version, so these should be fine. Double check the IL to be sure (or test for the difference).

Double check the IL to be sure (or test for the difference).

Yep, that's what I am doing to verify. I could instead use a custom constant, like we do in S.T.Json (that has the added benefit of being in sync with what we are doing in the release/3.1 for S.T.E.Web).

corefx/src/System.Text.Json/src/System.Text.Json.csproj

Line 9 in bdd3397

<DefineConstants Condition="'$(TargetsNETStandard)' != 'true' AND '$(TargetsNetFx)' != 'true'">$(DefineConstants);BUILDING_INBOX_LIBRARY</DefineConstants>

Edit: Yep, works as expected.

ahsonkhan · 2019-10-24T05:40:34Z

src/System.Text.Encodings.Web/src/Configurations.props

  <PropertyGroup>
    <BuildConfigurations>
-      netcoreapp;
+      netcoreapp3.0;


Some of the references that S.T.E.W depends on don't have a config for netcoreapp3.0.
System.Diagnostics.Debug
System.Memory
System.Runtime
etc.

Should we also add the versioned TFM configs to all those (along side the version-less ones) or do we need to add package configurations here, instead?

corefx/src/System.Diagnostics.Debug/src/Configurations.props

Lines 4 to 5 in bd18c12

netcoreapp-Unix;

netcoreapp-Windows_NT;

corefx/src/System.Memory/src/Configurations.props

Lines 4 to 5 in bd18c12

netcoreapp-Windows_NT;

netcoreapp-Unix;

That's causing the CI failures:

##[error].dotnet\sdk\3.0.100\Microsoft.Common.CurrentVersion.targets(2106,5): error MSB3245: Could not resolve this reference. Could not locate the assembly "System.Runtime". Check to make sure the assembly exists on disk. If this reference is required by your code, you may get compilation errors.

build -allconfigurations succeeds but targeting individual frameworks (particularly the default - netcoreapp) fails.

This library is inbox as well so you will need a version-less build configuration but you will need to exclude it from the packages configuration so that it targets the latest.

So you do something like:

<PackageConfigurations> netcoreapp3.0; netstandard2.1; netstandard; </PackageConfigurations> <BuildConfigurations> $(PackageConfigurations); netcoreapp; </BuildConfigurations>

So you do something like

Thanks for the suggestion, @safern. That's precisely what I tried next, following precedence of other similar packages like Threading.Channels:

corefx/src/System.Threading.Channels/src/Configurations.props

Lines 2 to 12 in bd18c12

<PropertyGroup>

<PackageConfigurations>

netstandard1.3;

netstandard;

netcoreapp3.0;

</PackageConfigurations>

<BuildConfigurations>

$(PackageConfigurations);

netcoreapp;

</BuildConfigurations>

</PropertyGroup>

The netcoreapp3.0 package config still causes the failure though. I don't know if its because of contention caused by netstandard2.1 and netcoreapp3.0.

Let me retry that to verify.

Awesome, that worked! I wasn't updating the Configurations property in the csproj locally to include netcoreapp which is why I was seeing the failure earlier.

safern · 2019-10-24T06:56:13Z

src/System.Text.Encodings.Web/src/System/Text/Encodings/Web/DefaultJavaScriptEncoder.cs

 using System.Text.Unicode;

-#if NETCOREAPP
+#if BUILDING_INBOX_LIBRARY


You could do
#if !NETSTANDARD
...
#endif

And then that means anything but any version of netstandard.

Any particular reason why one is considered better than the other? #if NETCOREAPP, vs #if !NETSTANDARD, vs #if CUSTOM_CONSTANT?

Would using constants like NETSTANDARD work in release/3.1?

BUILDING_INBOX_LIBRARY is incorrect. You want this for the package build of Netcoreapp. I would recommend #if netcoreapp if that works in both master and release. I know that works in master: versionless define indicates any, as it does with the SDK. I think we still had the differing behavior in release, so a dedicated define may be appropriate. How about USE_INTRINSICS or something like that in release?

Yes, the SDK constants do work in the release/3.x branches as well.

I would recommend #if netcoreapp if that works in both master and release.

Any difference between lower-case and upper-case? I left it as upper-case since that's what is being used across the repo.

Lower-case is what we used to define but we removed that some weeks ago, upper-case are the ones the SDK define based on the TFM.

…uild config

…brary" This reverts commit 8648cf6.

ahsonkhan · 2019-10-24T23:33:41Z

The change is ready for review. Let me know if anything else needs to be addressed.

…M (nc3.0). (dotnet/corefx#42069) * Update the S.T.E.W configurations to explicitly target a versioned TFM (nc3.0). * Use custom defined constant like S.T.Json - Building_Inbox_Library * Update config.props to use package config and leave netcoreapp as a build config * Add back netcoreapp specific config to the src csproj * Revert "Use custom defined constant like S.T.Json - Building_Inbox_Library" This reverts commit dotnet/corefx@8648cf6. * Remove extra new line. Commit migrated from dotnet/corefx@0954a97

Update the S.T.E.W configurations to explicitly target a versioned TFM

faadac4

(nc3.0).

ahsonkhan added the area-System.Text.Encodings.Web label Oct 23, 2019

ahsonkhan added this to the 5.0 milestone Oct 23, 2019

ahsonkhan commented Oct 23, 2019

View reviewed changes

Use custom defined constant like S.T.Json - Building_Inbox_Library

8648cf6

ahsonkhan commented Oct 24, 2019

View reviewed changes

ahsonkhan requested review from GrabYourPitchforks and tarekgh October 24, 2019 05:48

ahsonkhan mentioned this pull request Oct 24, 2019

Optimize FindFirstCharToEncode for JavaScriptEncoder.Default and Relaxed using Sse2 intrinsics. #41933

Merged

safern reviewed Oct 24, 2019

View reviewed changes

ahsonkhan and others added 4 commits October 24, 2019 02:23

Update config.props to use package config and leave netcoreapp as a b…

563725a

…uild config

Add back netcoreapp specific config to the src csproj

87b013c

Revert "Use custom defined constant like S.T.Json - Building_Inbox_Li…

1a7a1eb

…brary" This reverts commit 8648cf6.

Remove extra new line.

4c58ba9

ericstj approved these changes Oct 24, 2019

View reviewed changes

safern approved these changes Oct 25, 2019

View reviewed changes

safern merged commit 0954a97 into dotnet:master Oct 25, 2019

ahsonkhan deleted the FixPackaging branch October 25, 2019 03:33

	#if NETCOREAPP
	using System.Runtime.Intrinsics;
	using System.Runtime.Intrinsics.X86;
	#endif

	<PropertyGroup>
	<PackageConfigurations>
	netstandard1.3;
	netstandard;
	netcoreapp3.0;
	</PackageConfigurations>
	<BuildConfigurations>
	$(PackageConfigurations);
	netcoreapp;
	</BuildConfigurations>
	</PropertyGroup>

Conversation

ahsonkhan commented Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ahsonkhan Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan Oct 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Oct 24, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahsonkhan commented Oct 23, 2019 •

edited

Loading

ahsonkhan Oct 23, 2019 •

edited

Loading

ahsonkhan Oct 23, 2019 •

edited

Loading

ahsonkhan Oct 24, 2019 •

edited

Loading

ahsonkhan Oct 24, 2019 •

edited

Loading