Copy File step refactor by kikimych · Pull Request #348 · vsrad/radeon-asm-tools

kikimych · 2023-02-21T14:27:33Z

Copy File step can now copy directories

johnny-keker

Interface is unusable.

johnny-keker · 2023-02-22T19:00:45Z

RadeonAsmDebugger.sln

-		{B7222544-B13F-48C8-85E3-6BFD3D44702A}.Debug|Any CPU.ActiveCfg = Debug|Any CPU
-		{B7222544-B13F-48C8-85E3-6BFD3D44702A}.Debug|Any CPU.Build.0 = Debug|Any CPU
-		{B7222544-B13F-48C8-85E3-6BFD3D44702A}.Release|Any CPU.ActiveCfg = Release|Any CPU


Changes in this file don't make sense. We simply moved this 3 lines 4 lines further. Please revert

johnny-keker · 2023-02-22T19:01:14Z

VSRAD.Build/VSRAD.Build.csproj

    <ErrorReport>prompt</ErrorReport>
    <WarningLevel>4</WarningLevel>
    <CodeAnalysisRuleSet />
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>


Why do we need this property?

johnny-keker · 2023-02-22T19:01:31Z

VSRAD.Build/VSRAD.Build.csproj

    <RemoveDir Directories="$(ProjectDir)..\$(ConfigurationName)"/>
  </Target>
-</Project>
+</Project>


Why do we need this line in diff?

johnny-keker · 2023-02-22T19:03:54Z

VSRAD.DebugServer/IPC/StreamExtensions.cs

+                    var sendSize = Math.Min(bytesToSend, BUFFER_SIZE);
+
+                    var readCount = reader.Read(buffer, 0, Convert.ToInt32(sendSize));
+
+                    if (readCount == 0) throw new IOException("SendFileAsync file read error");
+
+                    writer.Write(buffer, 0, readCount);
+
+                    bytesToSend -= readCount;


Codestyle: why emptyline between every line of code?

johnny-keker · 2023-02-22T19:04:46Z

VSRAD.DebugServer/SharedUtils/FileMetadata.cs

+        public string relativePath_ { get; set; }
+
+        public DateTime lastWriteTimeUtc_ { get; set; }
+
+        public bool isDirectory_ { get; set; }


Codestyle: what case is this and why is it chosen?

I believe we use PascalCase for public properties.

johnny-keker · 2023-02-22T19:05:53Z

VSRAD.DebugServer/SharedUtils/FileMetadata.cs

+namespace VSRAD.DebugServer.SharedUtils
+{
+
+    public class FileMetadata// : IEquatable<FileMetadata>


So IEquatable should not be implemented?

johnny-keker · 2023-02-22T19:06:26Z

VSRAD.DebugServer/VSRAD.DebugServer.csproj

       <Version>$([System.Text.RegularExpressions.Regex]::Match($([System.Text.RegularExpressions.Regex]::Match($([System.IO.File]::ReadAllText("$(MSBuildProjectDirectory)\..\VSRAD.Package\source.extension.vsixmanifest")), `Version=\"[0-9,.]+\" Language=`).ToString()),`[0-9,.]+`).ToString())</Version>
    </PropertyGroup>
-    <Message Importance="High" Text="Debug server version: $(Version)"/>
+    <Message Importance="High" Text="Debug server version: $(Version)" />


We do not need this line in diff.

johnny-keker · 2023-02-22T19:20:00Z

Interface is unusable.

upd: fixed in 034f908

johnny-keker

Project is not building in Release configuration

johnny-keker · 2023-02-22T19:40:05Z

Project is not building in Release configuration

Looks like we need <AllowUnsafeBlocks>true</AllowUnsafeBlocks> in both Build and DebugServer

However, jumping ahead, I don't like using unsafe code at all. As far as I can see, we can refactor code to get rid of it. But I'm still investigating it

johnny-keker · 2023-02-22T19:42:28Z

For example, EqualStartingCharacterCount can be rewritten in C# way, using Zip from Linq and simple foreach iterator.

johnny-keker · 2023-02-22T19:56:29Z

@kikimych please consider f93be37

I believe this is much more laconic and simple implementation of same (am I right?) function without using unsafe code

johnny-keker

⚠ Show stopper ⚠

Current manual approach to paths handling doesn't work for Windows host -> Linux remote situation. Next image illustrates problem, Windows host is on the left, Linux remote on the right

Personally I am not very fond of whole PathExtension.cs thing, I'll try to come out with better suggestion ASAP

johnny-keker · 2023-02-22T21:33:32Z

VSRAD.DebugServer/IPC/StreamExtensions.cs

+                case SendFileCommand _:
+                    var send_command = (SendFileCommand)command;
+                    var sendPath = Path.Combine(send_command.SrcPath, send_command.Metadata.relativePath_);
+                    return await SendFileAsync(writer, sendPath, send_command.UseCompression).ConfigureAwait(false);
+                case GetFileCommand _:
+                    var receive_command = (GetFileCommand)command;
+                    var receivePath = Path.Combine(receive_command.DstPath, receive_command.Metadata.relativePath_);
+                    return await ReceiveFileAsync(writer, receivePath, receive_command.UseCompression);


In this chunk of code switch already does conversion for us, however we discard it using _, only to do it again manually in the next line. Also, we do not use snake case anywhere in project (as far as I know)

I suggest rewriting this switch like this:

case SendFileCommand sf: var sendPath = Path.Combine(sf.SrcPath, sf.Metadata.relativePath_); return await SendFileAsync(writer, sendPath, sf.UseCompression).ConfigureAwait(false); case GetFileCommand gf: var receivePath = Path.Combine(gf.DstPath, gf.Metadata.relativePath_); return await ReceiveFileAsync(writer, receivePath, gf.UseCompression); default: break;

johnny-keker · 2023-02-22T23:24:47Z

VSRAD.DebugServer/SharedUtils/FileMetadata.cs

+        public String ToString()
+        {
+            var props = this.GetType().GetProperties();
+            var sb = new StringBuilder();
+            foreach (var p in props)
+            {
+                sb.AppendLine(p.Name + ": " + p.GetValue(this, null));
+            }
+            return sb.ToString();
+        }


Do we really need to use reflection to iterate over 3 properties?

Also, readability can be improved. I would suggest maybe something more like

public override string ToString() => string.Join(Environment.NewLine, new string[] { "Metadata = [", $"\tRelative Path : {relativePath_}", $"\tLast Write Time UTC : {lastWriteTimeUtc_}", $"\tIs Directory : {isDirectory_}", "]" });

johnny-keker · 2023-02-22T23:31:26Z

VSRAD.DebugServer/IPC/Responses.cs

+        private static XmlSerializer getFormatter()
+        {
+            return new XmlSerializer(typeof(List<FileMetadata>));
+        }


Do we really don't have other options to serialize a List of FileMetadata but using XmlSerializer?

It seems off to me, since we never used such format before anywhere in project.

Also, this declaration clearly suggests that it was originated in some Java code, in C# we use PascalCase in such places.

johnny-keker

Checked new version with server on Ubuntu. Old problem (#348 (review)) is not resolved, and now I have a new problem when new directory path contain full windows path from host

johnny-keker · 2023-04-20T12:56:57Z

VSRAD.DebugServer/IPC/Responses.cs

+        private static XmlSerializer GetFormatter()
+        {
+            return new XmlSerializer(typeof(List<FileMetadata>));
+        }


Same as #348 (comment)

Do we really need XmlSerializer?

johnny-keker · 2023-04-24T17:41:50Z

VSRAD.Package/Server/ActionRunner.cs

+                {
+                    var rootPath = Path.Combine(_environment.LocalWorkDir, step.SourcePath);
+                    var localInfos = new List<FileMetadata>();
+                    foreach (var info in PathExtension.TraverseFileInfoTree(rootPath))


This function will return an empty list in case we specified path of the particular file, and we won't send nothing. Expected behavior would be sending one specified file.

Client handles server ststuses for file copy commands Block length now has same size on both connection sides in compressed file transfers

kikimych added 4 commits February 13, 2023 16:54

Added CheckOutdatedFiles and ListFiles command

6cf8093

Added PutDirectory, SendFile, GetFile commands

1e024e4

Added recursive file copy support

3db9c60

Added network traffic compression

b30a08d

johnny-keker requested changes Feb 22, 2023

View reviewed changes

kikimych force-pushed the copy_directory branch from 034f908 to 53f3b2a Compare February 27, 2023 16:11

kikimych force-pushed the copy_directory branch 2 times, most recently from c7ba63e to efc0e20 Compare March 6, 2023 18:04

johnny-keker requested changes Apr 20, 2023

View reviewed changes

Replaced IterateFilesystemInfo with custom tree traverser

e70eb36

This comment was marked as outdated.

Sign in to view

kikimych force-pushed the copy_directory branch from efc0e20 to e70eb36 Compare April 21, 2023 17:58

johnny-keker requested changes Apr 24, 2023

View reviewed changes

Reworked copy file status handling

2e2f3ea

Client handles server ststuses for file copy commands Block length now has same size on both connection sides in compressed file transfers

Comments

Conversation

kikimych commented Feb 21, 2023

Uh oh!

johnny-keker left a comment

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!

johnny-keker Feb 22, 2023 • 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!

johnny-keker commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

johnny-keker left a comment

Choose a reason for hiding this comment

Uh oh!

johnny-keker commented Feb 22, 2023

Uh oh!

johnny-keker commented Feb 22, 2023

Uh oh!

johnny-keker commented Feb 22, 2023

Uh oh!

johnny-keker left a comment

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!

johnny-keker left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

johnny-keker Feb 22, 2023 •

edited

Loading

johnny-keker commented Feb 22, 2023 •

edited

Loading