Allow Utf8String package to work on netstandard 2.0 #33357

eerhardt · 2020-03-08T22:48:27Z

Index and Range APIs are netcoreapp3.0+ only. Includes System.Text.Rune for netstandard2.0, since that is currently only netcoreapp3.0.

Getting the full build working again.

Get the package building.

Dotnet-GitSync-Bot · 2020-03-08T22:48:52Z

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

…c with cancellation.

src/libraries/System.Private.CoreLib/src/System/Index.cs

stephentoub · 2020-03-09T13:14:22Z

src/libraries/System.Private.CoreLib/src/System/Range.cs

@@ -47,12 +52,17 @@ public Range(Index start, Index end)
        /// <summary>Returns the hash code for this instance.</summary>
        public override int GetHashCode()
        {
+#if SYSTEM_PRIVATE_CORELIB || NETCOREAPP


In some other places where we've had these one-off experimental downlevel packages, we've instead chosen to just duplicate/specialize the source, with dedicated files for that package, rather than complicating shared code with lots of ifdefs that we'll likely end up removing in the near future. Might want to consider that with this, too.

I've simplified the ifdefs in this file, so it looks much better now. I can split the GetHashCode and ToString out into specialized files if you still think it is worth it.

src/libraries/System.Private.CoreLib/src/System/Range.cs

stephentoub · 2020-03-09T13:18:16Z

src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs

@@ -20,6 +20,10 @@ namespace System.Text
    [DebuggerDisplay("{DebuggerDisplay,nq}")]
    public readonly struct Rune : IComparable<Rune>, IEquatable<Rune>
    {
+        private const char HIGH_SURROGATE_START = '\ud800';
+        private const char LOW_SURROGATE_START = '\udc00';
+        private const int HIGH_SURROGATE_RANGE = 0x3FF;


Can we use PascalCase for such consts?

I can. I copied this from existing code in CharUnicodeInfo. Do you think I should change all the existing occurrences to PascalCase as well?

https://github.com/dotnet/runtime/search?l=C%23&q=HIGH_SURROGATE_START

src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs

src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.cs

eng/referenceFromRuntime.targets

src/libraries/System.Utf8String.Experimental/tests/System/Utf8SpanTests.Searching.TestData.cs

src/libraries/System.Utf8String.Experimental/ref/System.Utf8String.Experimental.csproj

GrabYourPitchforks

I haven't gotten through it all yet but wanted to give some preliminary feedback. Will take another stab tomorrow.

Thanks so much for getting this PR out!

src/libraries/System.Private.CoreLib/src/System/Text/Rune.cs

src/libraries/System.Private.CoreLib/src/System/Index.cs

src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Conversion.cs

src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.Searching.cs

src/libraries/System.Utf8String.Experimental/src/System/Runtime/Intrinsics/Intrinsics.Shims.cs

eerhardt · 2020-03-11T02:36:01Z

I believe I have addressed all existing feedback. Please take a look and let me know if you have any more.

eerhardt · 2020-03-12T21:18:18Z

I'm removing the "new-api-needs-documentation" label on this PR since there is no new API being introduced.

src/libraries/System.Private.CoreLib/src/System/Text/Utf8Span.cs

src/libraries/System.Private.CoreLib/src/System/Text/Utf8StringComparer.cs

src/libraries/System.Private.CoreLib/src/System/Utf8String.Construction.cs

src/libraries/System.Utf8String.Experimental/src/System/Net/Http/Utf8StringContent.cs

src/libraries/System.Utf8String.Experimental/src/System/Utf8String.Portable.cs

src/libraries/System.Utf8String.Experimental/tests/System/ReflectionTests.netfx.cs

src/libraries/System.Utf8String.Experimental/tests/System/Utf8SpanTests.Conversion.cs

src/libraries/System.Utf8String.Experimental/tests/System/Utf8StringTests.Ctor.cs

src/libraries/System.Utf8String.Experimental/ref/System.Utf8String.Experimental.csproj

…ide of CoreLib.

src/libraries/System.Private.CoreLib/src/System/Utf8String.Construction.cs

jkotas · 2020-03-16T15:10:06Z

src/libraries/System.Utf8String.Experimental/ref/System.Utf8String.Experimental.csproj

  <PropertyGroup>
    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
    <!-- disable warnings about obsolete APIs -->
    <NoWarn>$(NoWarn);0809;0618</NoWarn>
-    <TargetFrameworks>$(NetCoreAppCurrent)</TargetFrameworks>
+    <TargetFrameworks>netstandard2.0;netstandard2.1;netcoreapp3.0;$(NetCoreAppCurrent)</TargetFrameworks>
    <Nullable>enable</Nullable>
  </PropertyGroup>
  <ItemGroup Condition="'$(IsPrerelease)' != 'false'">


Should the whole project be disabled when IsPrerelease=true? None of this should be building in release branches.

I guess I assumed that's what this line was intending to do.

@GrabYourPitchforks @joperezr @safern - thoughts on how to do this?

Should the whole project be disabled when IsPrerelease=true? None of this should be building in release branches.

Really? You don't even want to ship previews?

Here are the set of "modes" that folks usually wish to fall into:

Build, ship, and stabilize along with the rest of the product.

Build, ship, but do not stabilize along with the rest of the product.

Build, but do not ship nor stabilize along with the rest of the product.

Let me know which of these fits the best, or if you have different requirements than what is discussed here.

I think we agreed that this should be built in release branches as we still want to ship the experimental packages to nuget but they won't be marked as stable... so to me this condition is no longer needed as we control the package versioning through other properties like:

runtime/Directory.Build.props

Lines 75 to 76 in 73268c7

<SuppressFinalPackageVersion Condition="'$(SuppressFinalPackageVersion)' == '' and $(MSBuildProjectName.Contains('Experimental'))">true</SuppressFinalPackageVersion>

<IsShippingAssembly Condition="$(MSBuildProjectName.Contains('Experimental'))">false</IsShippingAssembly>

cc: @mmitche @danmosemsft

I didn't have @ericstj reply when I was responding, so basically what we do now using the properties above is number 2.

Build, ship, but do not stabilize along with the rest of the product.

We have not been shipping experimental packages to nuget.org. For example, System.Runtime.Intrinsics.Experimental was not on nuget.org for 3.0. People who wanted to use the experimental packages had no problem with using them from the daily feeds.

That was a bug that I introduced in 3.0 and fixed in 5.0.

dotnet/corefx#41513

I had a discussion with @danmosemsft @ericstj and @GrabYourPitchforks and we agreed we wanted them in NuGet to raise visibility and usage of the packages, but they would only ship non-stable versions until they're marked as non-experimental.

If we want to not have them in NuGet, IsPrerelease is not the way to go anyway, we would need to tweak the IsShippingPackage property to set it to false for these packages.

I agree that publishing experimental packages to nuget.org sends the wrong message. A developer can easily get confused about what is supported.

Keeping experimental packages off of nuget.org, and forcing users to opt-in by using a different package feed, makes it even more clear that this package isn't intended for production usage.

I recall the agreement being similar to what Jan said: no building in release branches, no servicing. I'm impartial as to whether or not we publish to nuget.org. Our readme already gives the correct acquisition URL for the install-package command.

Opened #33636

Yeah, I discussed this earlier with @eerhardt and it makes sense we should keep them off nuget as it's inevitable they will be perceived as supported.

eerhardt added 18 commits March 6, 2020 13:51

Update ref assembly for building netstandard2.0.

b5fe79a

Index and Range APIs are netcoreapp3.0+ only. Includes System.Text.Rune for netstandard2.0, since that is currently only netcoreapp3.0.

Move Utf8String code from coreclr to libraries

9a2168d

Get Rune working on netstandard2.0

f296c30

Get Utf8Span compiling on netstandard2.0.

aa1e4c8

Get Utf8String building on netstandard2.0

a52f01b

Get Utf8StringContent building on netstandard2.0 again.

ee19225

Compile Utf8String/Span.Conversion.cs for netstandard.

8193573

Build Utf8String Comparison and Enumeration for netstandard.

4e2615a

Enable Index and Range on netstandard2.0

cea9703

Everything building

97b89d0

Moving code around so it lines up with the ref assemblies.

414c890

Getting the full build working again.

Build for netcoreapp3.0.

f5b7269

Get the package building.

Get tests building on netfx

6c6fd97

Fix up merge

545e3d2

Get Utf8String constructors working on netstandard.

5963e60

Get some tests running on net472

f6b917c

More tests running on netfx

7c6e17d

Get all tests running on netfx.

5c2aba5

Dotnet-GitSync-Bot added area-System.Runtime new-api-needs-documentation labels Mar 8, 2020

Fix build for netcoreapp3.0, which doesn't have SerializeToStreamAsyn…

7daf535

…c with cancellation.

eerhardt force-pushed the Utf8StringPackaging branch from 14fc1d9 to 7daf535 Compare March 9, 2020 02:25

stephentoub reviewed Mar 9, 2020

View reviewed changes

eerhardt added 2 commits March 9, 2020 15:01

Add netstandard2.1 support to Utf8String.Experimental.

889c9a2

Clean up to prepare for review.

b418ed2

eerhardt marked this pull request as ready for review March 9, 2020 22:46

eerhardt requested a review from GrabYourPitchforks March 9, 2020 22:46

eerhardt commented Mar 9, 2020

View reviewed changes

eng/referenceFromRuntime.targets Outdated Show resolved Hide resolved

eerhardt commented Mar 9, 2020

View reviewed changes

src/libraries/System.Utf8String.Experimental/tests/System/Utf8SpanTests.Searching.TestData.cs Outdated Show resolved Hide resolved

joperezr reviewed Mar 9, 2020

View reviewed changes

src/libraries/System.Utf8String.Experimental/ref/System.Utf8String.Experimental.csproj Show resolved Hide resolved

GrabYourPitchforks reviewed Mar 9, 2020

View reviewed changes

eerhardt mentioned this pull request Mar 10, 2020

DesignTimeBuild fails for System.Utf8String.Experimental src project #33427

Closed

Address PR feedback

aafb1d8

Make Intrinsics IsSupported be const fields.

735317d

eerhardt force-pushed the Utf8StringPackaging branch from 93900e1 to 735317d Compare March 11, 2020 02:47

eerhardt removed the new-api-needs-documentation label Mar 12, 2020

GrabYourPitchforks approved these changes Mar 13, 2020

View reviewed changes

safern reviewed Mar 13, 2020

View reviewed changes

src/libraries/System.Utf8String.Experimental/ref/System.Utf8String.Experimental.csproj Show resolved Hide resolved

Address PR feedback

6cef31b

eerhardt mentioned this pull request Mar 13, 2020

Take advantage of nint and nuint in Utf8String code #33575

Closed

Ensure nint and nuint are set correctly when building Utf8String outs…

d2d5441

…ide of CoreLib.

GrabYourPitchforks approved these changes Mar 14, 2020

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Utf8String.Construction.cs Outdated Show resolved Hide resolved

Fix bug in construction from long UTF16 string.

9e682d1

eerhardt merged commit 98d9e7e into dotnet:master Mar 14, 2020

eerhardt deleted the Utf8StringPackaging branch March 14, 2020 15:43

jkotas reviewed Mar 16, 2020

View reviewed changes

jkotas mentioned this pull request Mar 16, 2020

Publishing of experimental packages #33636

Closed

eerhardt mentioned this pull request Apr 17, 2020

Meaning of negative index in System.Numerics.Tensors #35075

Closed

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow Utf8String package to work on netstandard 2.0 #33357

Allow Utf8String package to work on netstandard 2.0 #33357

eerhardt commented Mar 8, 2020 •

edited

Loading

Dotnet-GitSync-Bot commented Mar 8, 2020

stephentoub Mar 9, 2020

eerhardt Mar 9, 2020

stephentoub Mar 9, 2020

eerhardt Mar 9, 2020

GrabYourPitchforks left a comment

eerhardt commented Mar 11, 2020

eerhardt commented Mar 12, 2020

jkotas Mar 16, 2020

eerhardt Mar 16, 2020

ericstj Mar 16, 2020

safern Mar 16, 2020

safern Mar 16, 2020

safern Mar 16, 2020

eerhardt Mar 16, 2020

GrabYourPitchforks Mar 16, 2020

jkotas Mar 16, 2020

danmoseley Mar 16, 2020

	<SuppressFinalPackageVersion Condition="'$(SuppressFinalPackageVersion)' == '' and $(MSBuildProjectName.Contains('Experimental'))">true</SuppressFinalPackageVersion>
	<IsShippingAssembly Condition="$(MSBuildProjectName.Contains('Experimental'))">false</IsShippingAssembly>

Allow Utf8String package to work on netstandard 2.0 #33357

Allow Utf8String package to work on netstandard 2.0 #33357

Conversation

eerhardt commented Mar 8, 2020 • edited Loading

Dotnet-GitSync-Bot commented Mar 8, 2020

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

GrabYourPitchforks left a comment

Choose a reason for hiding this comment

eerhardt commented Mar 11, 2020

eerhardt commented Mar 12, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Mar 8, 2020 •

edited

Loading