Added TryParse to StandardFormat #33687

AlexejLiebenthal · 2018-11-25T15:52:57Z

Fixes issue #32950

Removed StandardFormat.Parse(string) overload in src/System.Memory/ref/System.Memory.cs
Added StandardFormat.TryParse in src/System.Memory/ref/System.Memory.cs
Marked Obsolete StandardFormat.Parse(string) overload in src/Common/src/CoreLib/System/Buffers/StandardFormat.cs
Added TryParse in src/Common/src/CoreLib/System/Buffers/StandardFormat.cs
Added UnitTests for TryParse in src/System.Memory/tests/ParsersAndFormatters/StandardFormatTests.cs

Need help for this. I didn't get it exactly how all these parts work together.
Like I see, there is src/System.Memory/ref/System.Memory.cs, where the "abstraction" is defined and the src/Common/src/CoreLib/System/Buffers/StandardFormat.cs, but this file has no *.sln or *.csproj. But as you can see regardless of the changes I made the build fails.

src/Common/src/CoreLib/System/Buffers/StandardFormat.cs

stephentoub · 2018-11-26T13:31:02Z

cc: @ahsonkhan

ahsonkhan

Some nits, but I believe the change should be made in the coreclr repo first (which get mirrored to corefx). We can expose the new API from System.Memory ref and add the test here (i.e. corefx).

src/Common/src/CoreLib/System/Buffers/StandardFormat.cs

src/System.Memory/tests/ParsersAndFormatters/StandardFormatTests.cs

src/System.Memory/ref/System.Memory.cs

ahsonkhan · 2018-11-26T21:22:54Z

Need help for this. I didn't get it exactly how all these parts work together.

Yep, no worries. It is a bit tricky since the implementation is in CoreLib.

Like I see, there is src/System.Memory/ref/System.Memory.cs, where the "abstraction" is defined and the src/Common/src/CoreLib/System/Buffers/StandardFormat.cs, but this file has no *.sln or *.csproj. But as you can see regardless of the changes I made the build fails.

The file is included within CoreLib as part of System.Private.CoreLib (coming from coreclr), but exposed from System.Memory reference assembly:

corefx/src/Common/src/CoreLib/System.Private.CoreLib.Shared.projitems

Line 55 in 5aab1d3

    
           <Compile Include="$(MSBuildThisFileDirectory)System\Buffers\StandardFormat.cs" />

Here are the steps I would follow:

Open a PR in coreclr with your source changes to StandardFormat.cs and wait for it to get merged.
Change this PR to only expose the new API in the System.Memory reference assembly and the new tests you added.
Once the PR in coreclr is merged, your changes will get mirorred into corefx automatically (a PR will be submitted automatically to mirror your changes from coreclr to this repo in src/Common/src/CoreLib/System/Buffers/StandardFormat.cs).
Your PR in corefx along with the tests won't pass until we consume a new coreclr build which has your changes within this repo. Once that happens, the CI should be green and we can merge this PR.

To test your changes locally:

Build coreclr locally (with your changes from 1 above).
Build corefx pointing to your local build of coreclr (for example: build.cmd -- /p:CoreCLROverridePath=d:\git\coreclr\bin\Product\Windows_NT.x64.Release\).
Make your changes in corefx (as specified in 2 above), and then run your test. If they pass, they should pass once we consume coreclr into corefx.

See this for more details: https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md#testing-with-private-coreclr-bits

Feel free to ask any questions you may have and I will try to help.

ahsonkhan · 2018-11-26T21:32:00Z

As an example, look at these two PRs that recently added a new API to CoreLib which was exposed from System.Runtime:

And here is the auto-gen mirror PR:

Mirror changes from dotnet/corert,coreclr #33245

The only difference here is that the API you are adding is being exposed from System.Memory.

AlexejLiebenthal · 2018-11-26T22:04:53Z

@ahsonkhan Thank you very much for your help and constructive feedback :) Now the things getting much clearer for me.
I already thought that this has something to do with coreclr.
Hope I can finish my first PR to corefx tomorrow :)

ahsonkhan · 2018-11-26T22:33:30Z

Now the things getting much clearer for me.
I already thought that this has something to do with coreclr.
Hope I can finish my first PR to corefx tomorrow :)

Sounds good. This doc provides more context that could be useful to you:
https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/changing-corelib.md

…changes to ref and tests

AlexejLiebenthal · 2018-11-30T09:09:04Z

@dotnet-bot test this please

AlexejLiebenthal · 2018-12-01T20:22:01Z

@dotnet-bot test this please

AlexejLiebenthal · 2018-12-03T22:48:54Z

@dotnet-bot test Linux x64 Release Build
@dotnet-bot test NETFX x86 Release Build
@dotnet-bot test Packaging All Configurations x64 Debug Build
@dotnet-bot test UWP CoreCLR x64 Debug Build
@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

ahsonkhan · 2018-12-06T11:06:40Z

@dotnet-bot test this please

AlexejLiebenthal · 2018-12-06T18:22:06Z

@dotnet-bot test Linux x64 Release Build

@ahsonkhan Do you know what is going wrong here?
It seems like netfx build can't find the TryParse method/doesn't use the implementation of coreclr/System.Buffers:
error CS0117: 'StandardFormat' does not contain a definition for 'TryParse'

AlexejLiebenthal · 2018-12-06T18:25:39Z

@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

ahsonkhan · 2018-12-06T19:35:50Z

Do you know what is going wrong here?
It seems like netfx build can't find the TryParse method/doesn't use the implementation of coreclr/System.Buffers

Yes, this should be fixed. Here is the cause/context:

We used to build System.Memory (ref/src) for netstandard as well as netcoreapp (.NET Core 2.1):

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

Lines 4 to 7 in 448ba28

    
           <PackageConfigurations> 
        
             netstandard1.1; 
        
             netstandard; 
        
           </PackageConfigurations>

We now only build for netcoreapp (no longer netstandard, since the OOB API surface is frozen):

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

Lines 4 to 10 in 3839c4a

    
           <BuildConfigurations> 
        
             uap-Windows_NT; 
        
             netcoreapp-Windows_NT; 
        
             netcoreappaot-Windows_NT; 
        
             netcoreappaot-WebAssembly; 
        
             netcoreapp-Unix; 
        
           </BuildConfigurations>

Update System.Memory as inbox & expose LastIndexOf #30897
https://github.com/dotnet/corefx/issues/30960

The tests still run on netstandard as well, and expect an API that doesn't exist (for Netfx, since it was only added on .NET Core 3.0):
- corefx/src/System.Memory/tests/Configurations.props
  
  Line 6 in 3839c4a
  
  netstandard;

The fix:

Remove the netstandard configuration from the tests since the implementation doesn't build for it anymore.

If you are up for it, the fix shouldn't be too difficult. There might be some if/defs or netstandard specific test code you would have to remove. Let me know if you need any help or get blocked.

You can test locally by building and testing while targeting netfx (https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/developer-guide.md):
At repo root (only required once): build -framework netfx
At test project root: dotnet msbuild /t:BuildAndTest /p:TargetGroup=netfx

Notice the failure occurs only when building the test project:

03:23:24 ParsersAndFormatters\StandardFormatTests.cs(69,42): error CS0117: 'StandardFormat' does not contain a definition for 'TryParse' [D:\j\workspace\windows-TGrou---2a8f9c29\src\System.Memory\tests\System.Memory.Tests.csproj]

AlexejLiebenthal · 2018-12-06T20:40:57Z

Oh I was not so far away. I already opened this file corefx/src/System.Memory/src/Configurations.props earlier and played a bit around with adding netfx with no success to the build configuration.
I will try to fix it till end of the week.

Btw: Wasn't the idea to make this API a netstandard API?

And thank you again :)

AlexejLiebenthal · 2018-12-10T20:50:43Z

@ahsonkhan Stuck again :-/

After removing netstandard from corefx/src/System.Memory/tests/Configurations.props I now get a another build error (C:\Program Files\dotnet\sdk\2.1.500\Sdks\Microsoft.NET.Sdk\targets\Microsoft.NET.TargetFrameworkInference.targets(96,5): error NETSDK1013: The TargetFramework value '' was not recognized. It may be misspelled. If not, then the TargetFrameworkIdentifier and/or TargetFrameworkVersion properties must be specified explicitly.) when I executed dotnet msbuild /p:TargetGroup=netfx /t:BuildAndTest

Also with no success building with /p:ArchGroup=x86 and some combinations of dotnet msbuild /p:TargetGroup=netfx [/p:ArchGroup=x86 | /p:TargetFrameworkVersion=v4.7.2 | /p:TargetFrameworkIdentifier=.NETFramework] /t:BuildAndTest

ahsonkhan · 2018-12-10T21:11:14Z

when I executed dotnet msbuild /p:TargetGroup=netfx /t:BuildAndTest

Once you have removed netstandard from corefx/src/System.Memory/tests/Configurations.props, you cannot run tests targeting netfx since the package/library (along with tests) no longer supports netfx.

So, it failing to build/run for that configuration is expected, and isn't something that needs to be fixed. You can only pass in TargetGroups that are supported (i.e netcoreapp and uap).

AlexejLiebenthal · 2018-12-10T21:57:44Z

🤦‍♂️

* Added TryParse to StandardFormat * Removed Obsolete from StandardFormat.Parse(string) * checked out the non changed StandardFormat.cs and made the suggested changes to ref and tests * Removed netstandard from BuildConfigurations

* Added TryParse to StandardFormat * Removed Obsolete from StandardFormat.Parse(string) * checked out the non changed StandardFormat.cs and made the suggested changes to ref and tests * Removed netstandard from BuildConfigurations Commit migrated from dotnet/corefx@c4eb0f0

Added TryParse to StandardFormat

08a69cb

AlexejLiebenthal force-pushed the features/add-tryparse-to-standardformat branch from 7601574 to 08a69cb Compare November 25, 2018 17:32

stephentoub reviewed Nov 26, 2018

View reviewed changes

src/Common/src/CoreLib/System/Buffers/StandardFormat.cs Outdated Show resolved Hide resolved

Removed Obsolete from StandardFormat.Parse(string)

6e86731

ahsonkhan added the area-System.Memory label Nov 26, 2018

ahsonkhan reviewed Nov 26, 2018

View reviewed changes

ahsonkhan mentioned this pull request Nov 26, 2018

Update documentation for changing CoreLib dotnet/coreclr#21201

Merged

checked out the non changed StandardFormat.cs and made the suggested …

7ef0ccb

…changes to ref and tests

AlexejLiebenthal mentioned this pull request Nov 27, 2018

Added TryParse to StandardFormat dotnet/coreclr#21216

Merged

ahsonkhan approved these changes Nov 28, 2018

View reviewed changes

stephentoub approved these changes Nov 28, 2018

View reviewed changes

ahsonkhan mentioned this pull request Nov 30, 2018

MetadataLoadContext feature #33201

Merged

Removed netstandard from BuildConfigurations

20e047b

ahsonkhan approved these changes Dec 11, 2018

View reviewed changes

ahsonkhan merged commit c4eb0f0 into dotnet:master Dec 11, 2018

AlexejLiebenthal deleted the features/add-tryparse-to-standardformat branch December 11, 2018 06:19

karelz added this to the 3.0 milestone Dec 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TryParse to StandardFormat #33687

Added TryParse to StandardFormat #33687

AlexejLiebenthal commented Nov 25, 2018 •

edited

Loading

stephentoub commented Nov 26, 2018

ahsonkhan left a comment

ahsonkhan commented Nov 26, 2018

ahsonkhan commented Nov 26, 2018

AlexejLiebenthal commented Nov 26, 2018

ahsonkhan commented Nov 26, 2018 •

edited by jkotas

Loading

AlexejLiebenthal commented Nov 30, 2018

AlexejLiebenthal commented Dec 1, 2018

AlexejLiebenthal commented Dec 3, 2018

ahsonkhan commented Dec 6, 2018

AlexejLiebenthal commented Dec 6, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 6, 2018

ahsonkhan commented Dec 6, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 6, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 10, 2018 •

edited

Loading

ahsonkhan commented Dec 10, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 10, 2018

Added TryParse to StandardFormat #33687

Added TryParse to StandardFormat #33687

Conversation

AlexejLiebenthal commented Nov 25, 2018 • edited Loading

stephentoub commented Nov 26, 2018

ahsonkhan left a comment

Choose a reason for hiding this comment

ahsonkhan commented Nov 26, 2018

ahsonkhan commented Nov 26, 2018

AlexejLiebenthal commented Nov 26, 2018

ahsonkhan commented Nov 26, 2018 • edited by jkotas Loading

AlexejLiebenthal commented Nov 30, 2018

AlexejLiebenthal commented Dec 1, 2018

AlexejLiebenthal commented Dec 3, 2018

ahsonkhan commented Dec 6, 2018

AlexejLiebenthal commented Dec 6, 2018 • edited Loading

AlexejLiebenthal commented Dec 6, 2018

ahsonkhan commented Dec 6, 2018 • edited Loading

AlexejLiebenthal commented Dec 6, 2018 • edited Loading

AlexejLiebenthal commented Dec 10, 2018 • edited Loading

ahsonkhan commented Dec 10, 2018 • edited Loading

AlexejLiebenthal commented Dec 10, 2018

AlexejLiebenthal commented Nov 25, 2018 •

edited

Loading

ahsonkhan commented Nov 26, 2018 •

edited by jkotas

Loading

AlexejLiebenthal commented Dec 6, 2018 •

edited

Loading

ahsonkhan commented Dec 6, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 6, 2018 •

edited

Loading

AlexejLiebenthal commented Dec 10, 2018 •

edited

Loading

ahsonkhan commented Dec 10, 2018 •

edited

Loading