[WIP] Update core-setup local tools to use S.T.Json for netstandard2.0 instead by ahsonkhan · Pull Request #5010 · dotnet/core-setup

ahsonkhan · 2019-01-19T14:32:14Z

Initial attempt to port tools-local to use the System.Text.Json source package for TFM netstandard2.0. Note: There are some API changes going through so this is still WIP. We have the JsonReader and JsonWriter available atm so only using the components that are ready.

The System.Text.Json binary is in-box on .NET Core 3.0.

I added TODOs to use the JsonDocument and Deserializer once they are available. Using the Utf8Reader in these scenarios is less than ideal and requires a lot of code change.

This PR is mainly to experiment to gather feedback and showcase the capabilities. Hence, marked as no merge.

We can't remove the reference to Newtonsoft since the tools are targeting net46 on Unix.

Is it possible to drop support for all but netstandard2.0? Otherwise, we have to conditionally include the source package.

See https://github.com/dotnet/corefx/blob/master/src/System.Text.Json/source_package/README.md

Related PR: #5009

cc @joshfree, @bartonjs, @steveharter, @eerhardt, @KrzysztofCwalina, @JamesNK, @glennc, @ericstj

of Newtonsoft.

ahsonkhan · 2019-01-19T14:33:13Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+                    string jsonFileText = projectFileReader.ReadToEnd();
+
+                    // TODO: Replace the use of the Utf8JsonReader with the Deserializer once it is available.
+                    ConfigJson configJson = GetConfigJson(jsonFileText);


This isn't a great way to deserialize a POCO object. I used the JsonReader just as an experiment.

ahsonkhan · 2019-01-19T14:34:58Z

tools-local/tasks/ChangeEntryPointLibraryName.Netstandard2_0.cs

+
+        private void ReadWritePropertyName(string propertyName, ref Utf8JsonReader reader, ref Utf8JsonWriter writer)
+        {
+            if (propertyName == "targets")


We need to use the JsonDocument here. The order in which the "targets" property and the "libraries" property show up could affect the end results here if we use the Reader and Writer. This relies on "targets" coming up before "libraries".

https://dotnet.myget.org/feed/dotnet-core/package/nuget/Microsoft.Bcl.Json.Sources says JsonDocument is available now (4.6.0-preview.19073.1 is bigger than the previous version)

stephentoub · 2019-01-23T17:38:45Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+                if (!File.Exists(ConfigJsonFile))
+                {
+                    throw new FileNotFoundException($"Expected file {ConfigJsonFile} was not found.");
+                }


Won't File.OpenText throw the same exception, and without the inherent race condition? You're just trying to provide a better exception message in some cases?

Originally written this way: https://github.com/dotnet/core-setup/pull/2462/files#diff-0ebce9e49dc50f19e82e0d3ceada8568R43

stephentoub · 2019-01-23T17:39:22Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+
+                // Open the Config Json and read the values into the model
+                TextReader projectFileReader = File.OpenText(ConfigJsonFile);
+                if (projectFileReader != null)


This won't ever be null.

Copy/pasted from the origin impl. Will fix there.

stephentoub · 2019-01-23T17:40:09Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+                }
+
+                // Open the Config Json and read the values into the model
+                TextReader projectFileReader = File.OpenText(ConfigJsonFile);


This should be disposed of when you're done with it; otherwise you're leaving the file open until it's eventually closed by a finalizer.

Most of these exist in the current implementation and I didn't modify most of it. I am going to remove BuildFPMToolPreReqs.Netstandard2_0.cs since we should wait for the Deserializer anyway, but will fix the issues you mentioned here.

stephentoub · 2019-01-23T17:44:00Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+                    string jsonFileText = projectFileReader.ReadToEnd();
+
+                    // TODO: Replace the use of the Utf8JsonReader with the Deserializer once it is available.
+                    ConfigJson configJson = GetConfigJson(jsonFileText);


So we're opening the file, reading all of its contents into a string (which will involve converting from UTF8 to UTF16 if it's saved as UTF8), and then turning around and encoding that string into a UTF8 byte[]? Isn't the point of having a UTF8-based JSON reader to avoid that? Or it simply doesn't matter perf-wise here and this is what's cleanest?

I was going to ask the same question, but then I realized that the reader does encoding-sniffing. So if someone saves the file as UTF-16 this would still successfully read it.

If we're willing to say "the file is UTF-8" (or, more likely, "the file is 7-bit ASCII") then this can be simplified a lot.

stephentoub · 2019-01-23T17:45:58Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+            while (reader.Read() && reader.TokenType == JsonTokenType.PropertyName)
+            {
+                string propertyName = reader.GetStringValue().ToLower();
+                if (propertyName == Maintainer_Name_String)


Does perf not matter here? Or could we instead compare span to span rather than materializing the string? And if we stick with strings, could the comparison be done as ordinal ignore case rather than lower casing the property name?

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

bartonjs · 2019-01-23T17:51:40Z

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs

+    /// <summary>
+    /// Model classes for reading and storing the JSON. 
+    /// </summary>
+    public class ConfigJson


As with the other PR; there's a lot of duplication between this file and the alternate for this file.

eerhardt · 2019-01-23T17:53:01Z

This PR is mainly to experiment to gather feedback and showcase the capabilities. Hence, marked as no merge.

Is the intention to ever put this code in? I don't see a valid reason for complicating this code.

dleeapho · 2019-08-07T21:25:34Z

Closing as stale.

ahsonkhan · 2019-08-08T04:39:06Z

Sounds good. Will re-visit post 3.0.

Update core-setup local tools to use S.T.Json for netstandard2.0 instead

402646a

of Newtonsoft.

ahsonkhan added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Jan 19, 2019

ahsonkhan commented Jan 19, 2019

View reviewed changes

ahsonkhan mentioned this pull request Jan 19, 2019

Update M.E.DependencyModel to use S.T.Json source for netstandard2.0 #5009

Merged

joshfree requested review from bartonjs, davidfowl, rynowak, stephentoub and steveharter January 22, 2019 18:45

joshfree added this to the 3.0 milestone Jan 22, 2019

joshfree assigned ahsonkhan Jan 22, 2019

stephentoub reviewed Jan 23, 2019

View reviewed changes

tools-local/tasks/BuildFPMToolPreReqs.Netstandard2_0.cs Outdated Show resolved Hide resolved

bartonjs reviewed Jan 23, 2019

View reviewed changes

ahsonkhan added 2 commits January 24, 2019 00:31

Update to use newer source package.

0590866

Remove reader/writer based BuildFPMToolPreReqs

c3ab51c

ahsonkhan mentioned this pull request Jan 30, 2019

Seed the porting guide for going from Json.NET to System.Text.Json. dotnet/corefx#34944

Merged

dleeapho closed this Aug 7, 2019

ahsonkhan mentioned this pull request Jan 30, 2020

Should M.E.DependencyModel only target netstandard2.0+ going forward? dotnet/runtime#3425

Closed

Conversation

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

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

stephentoub Jan 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eerhardt commented Jan 23, 2019

Uh oh!

dleeapho commented Aug 7, 2019

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ahsonkhan commented Jan 19, 2019 •

edited

Loading

stephentoub Jan 23, 2019 •

edited

Loading

ahsonkhan commented Aug 8, 2019 •

edited

Loading