AzCopyCore updates #2171

KrzysztofCwalina · 2018-03-20T23:32:34Z

updated the sample to new packages and runtime
refactored HTTP code into a separate wrapper for IDuplexPipe
small resulting renames

ahsonkhan · 2018-03-21T00:17:17Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

-    <PackageReference Include="System.Text.Http.Parser" Version="$(CoreFxLabPackageVersion)" />
-    <PackageReference Include="System.Text.Utf8String" Version="$(CoreFxLabPackageVersion)" />
-    <PackageReference Include="System.IO.Pipelines" Version="$(CoreFxLabPackageVersion)" />
+    <PackageReference Include="System.Azure.Experimental" Version="0.1.0-preview2-180320-2" />


Update and use the CoreFxLabPackageVersion property to make it easier to update in the future.

Yeah, but there is a trade off here. I think it's good that we have some samples that validate how our customers are using and demonstrate how they should be using our packages.

I don't understand the trade off here. We have all these package version numbers here, and they point to the same version. We have a property in this csproj called CoreFxLabPackageVersion. I was just suggesting we use that so that it becomes a single point of edit in the future, if we ever want to upgrade them again. Note, this is still isolated from the rest of the repo and would still demonstrate the customer's use of our packages.

ahsonkhan · 2018-03-21T00:17:37Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

-    <PackageReference Include="System.Memory" Version="$(CoreFxPackageVersion)" />
-    <PackageReference Include="System.Threading.Tasks.Extensions" Version="$(CoreFxPackageVersion)" />
+    <PackageReference Include="Microsoft.DotNet.ILCompiler" Version="1.0.0-alpha-26320-02" />
+    <PackageReference Include="System.Buffers.ReaderWriter" Version="0.1.0-preview2-180320-2" />


nit: Move this down to the other ItemGroup with the rest of the corefxlab packages

ahsonkhan · 2018-03-21T00:17:46Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

+    <PackageReference Include="System.Text.Http" Version="0.1.0-preview2-180320-2" />
+    <PackageReference Include="System.Text.Http.Parser" Version="0.1.0-preview2-180320-2" />
+    <PackageReference Include="System.Text.Utf8String" Version="0.1.0-preview2-180320-2" />
+    <PackageReference Include="System.IO.Pipelines" Version="4.5.0-preview3-26319-04" />


nit: Move this up to the other ItemGroup with the rest of the CoreFX packages

ahsonkhan · 2018-03-21T00:18:04Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

+    <PackageReference Include="Microsoft.DotNet.ILCompiler" Version="1.0.0-alpha-26320-02" />
+    <PackageReference Include="System.Buffers.ReaderWriter" Version="0.1.0-preview2-180320-2" />
+    <PackageReference Include="System.Memory" Version="4.5.0-preview3-26319-04" />
+    <PackageReference Include="System.Threading.Tasks.Extensions" Version="4.5.0-preview3-26319-04" />


Update and use the CoreFxPackageVersion property to make it easier to update in the future.

As I said above, I like how the sample shows VS experience that our customers will get.

I don't know if someone would go and manually update each individual referenced packages using VS/NuGet package manager. Once they are added (which can be done by using VS/NuGet package manager), it would make sense to create a property to update the versions, no?

ahsonkhan · 2018-03-21T00:18:39Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

@@ -3,7 +3,9 @@
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>netcoreapp2.1</TargetFramework>
+	<!--


Why is this commented out? This should be the same as https://github.com/dotnet/corefxlab/blob/master/SharedRuntimeVersion.txt (or import the common.props file).

It's @terrajobst's fault. :-) I removed it now.

ahsonkhan · 2018-03-21T00:19:00Z

samples/AzCopyCore/AzCopyCore/AzCopyCore.csproj

-    <PackageReference Include="System.Text.Utf8String" Version="$(CoreFxLabPackageVersion)" />
-    <PackageReference Include="System.IO.Pipelines" Version="$(CoreFxLabPackageVersion)" />
+    <PackageReference Include="System.Azure.Experimental" Version="0.1.0-preview2-180320-2" />
+    <PackageReference Include="System.Buffers.Experimental" Version="0.1.0-preview2-180320-2" />


Can these be project references instead of package references?

Same as above: I think samples should show customer experience that does not require cloning the whole repo.

Agreed. In that case, package references make sense. Thanks.

ahsonkhan · 2018-03-21T00:19:30Z

samples/AzCopyCore/AzCopyCore/Program.cs


        var options = new CommandLine(args);
        ReadOnlySpan<char> source = options.GetSpan("/Source:");
        ReadOnlySpan<char> destination = options.GetSpan("/Dest:");

+        long before = GC.GetAllocatedBytesForCurrentThread();
+        var sw = Stopwatch.StartNew();


nit: use of var

ahsonkhan · 2018-03-21T00:20:18Z

samples/AzCopyCore/AzCopyCore/Program.cs

        {
-            client.Log = Log;
+            //var files = new Files(directoryPath);


clean up commented out code

ahsonkhan · 2018-03-21T00:20:37Z

samples/AzCopyCore/AzCopyCore/Program.cs

-            keyBytes = options["/DestKey:"].ComputeKeyBytes();
+            ReadOnlySpan<char> key = options["/DestKey:"];
+            var str = key.ToString();
+            keyBytes = key.ComputeKeyBytes();


Why is this change necessary?

leftover from debugging. I removed now. thanks.

FYI, I just confirmed, debugging works as expected and shows the string. So, hopefully, these types of changes won't be necessary anymore.

Refactored HTTP Out

5fe27d1

KrzysztofCwalina merged commit ea28ffe into dotnet:master Mar 20, 2018

ahsonkhan reviewed Mar 21, 2018

View reviewed changes

KrzysztofCwalina added a commit to KrzysztofCwalina/corefxlab that referenced this pull request Mar 22, 2018

Refactored HTTP Out (dotnet#2171)

bd90f22

KrzysztofCwalina deleted the SocketClient branch March 22, 2018 17:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AzCopyCore updates #2171

AzCopyCore updates #2171

KrzysztofCwalina commented Mar 20, 2018

ahsonkhan Mar 21, 2018 •

edited

Loading

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018 •

edited

Loading

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

KrzysztofCwalina Mar 21, 2018

ahsonkhan Mar 21, 2018

AzCopyCore updates #2171

AzCopyCore updates #2171

Conversation

KrzysztofCwalina commented Mar 20, 2018

ahsonkhan Mar 21, 2018 • edited Loading

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

ahsonkhan Mar 21, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Mar 21, 2018 •

edited

Loading

ahsonkhan Mar 21, 2018 •

edited

Loading