* * *

Author Topic: Process.pp - ugly code for RunCommand  (Read 712 times)

kris

  • Jr. Member
  • **
  • Posts: 54
Process.pp - ugly code for RunCommand
« on: October 13, 2017, 05:36:39 am »
I think that RunCommand function in Process.pp unit is ugly. The main problem is that it has plenty of unnecessary redundancy making the code longer than it needs to be and hard to read.

I propose a new code layout for that function along with cleaned private __RunCommand replacing the original InternalRunCommand:

Code: Pascal  [Select]
  1.   Function ReadInputPipe(const Pipe: TInputPipeStream; var BytesRead,OutputLen: integer; var OutputString: string): Boolean;
  2.    var
  3.      available : integer;
  4.      NumBytes  : integer;
  5.    begin
  6.      Result := False; available := 0;
  7.      if(BytesRead<0)then BytesRead := 0;
  8.      if(OutputLen<0)then OutputLen := 0;
  9.      repeat
  10.        if(Pipe=nil)then exit;
  11.        if(Pipe.NumBytesAvailable>0)
  12.          then available := Pipe.NumBytesAvailable
  13.          else break;
  14.        if(BytesRead+available>OutputLen)then
  15.          begin
  16.            OutputLen := BytesRead+READ_BYTES;
  17.            SetLength(OutputString,OutputLen);
  18.          end;
  19.        try
  20.          try
  21.            NumBytes := Pipe.Read(OutputString[1+BytesRead], available);
  22.            if(NumBytes>0)
  23.              then Inc(BytesRead, NumBytes);
  24.          except
  25.            Result := False;
  26.          end;
  27.        finally
  28.          Result := True;
  29.        end;
  30.      until false;
  31.    end;
  32.  
  33.   Function __Runcommand(var P: TProcess; var Stdout, Stderr: string; var ExitStatus:integer): integer;
  34.    var
  35.      bytesread_stdout : integer;
  36.      outputlen_stdout : integer;
  37.      bytesread_stderr : integer;
  38.      outputlen_stderr : integer;
  39.    begin
  40.      Result := -1; if(P=nil)then exit;
  41.      try
  42.        try
  43.          P.Options        := [poUsePipes];
  44.          bytesread_stdout := 0;
  45.          outputlen_stdout := 0;
  46.          bytesread_stderr := 0;
  47.          outputlen_stderr := 0;
  48.          P.Execute;
  49.          while(P.Running)do
  50.            begin
  51.              if(not ReadInputPipe(P.Output, bytesread_stdout, outputlen_stdout, Stdout))and
  52.                (not ReadInputPipe(P.Stderr, bytesread_stderr, outputlen_stderr, Stderr))
  53.                 then sleep(100);
  54.            end;
  55.        //Get remaining output after end of execution:
  56.          ReadInputPipe(P.Output, bytesread_stdout, outputlen_stdout, Stdout);
  57.          ReadInputPipe(P.Stderr, bytesread_stderr, outputlen_stderr, Stderr);
  58.          ExitStatus := P.ExitStatus;
  59.          Result     := 0; // we came to here, document that.
  60.        except
  61.           Result := 1;
  62.         end;
  63.      finally
  64.        FreeAndNil(P);
  65.        SetLength(Stdout,bytesread_stdout);
  66.        SetLength(Stderr,bytesread_stderr);
  67.      end;
  68.    end;
  69.  
  70.   Function RunCommand(const ExeName:string;const Commands:array of string;var Stdout,StdErr:string): Boolean;
  71.    var
  72.      ExitStatus,i : integer;
  73.      P            : TProcess;
  74.   begin
  75.     P := TProcess.Create(nil);
  76.     P.Executable := ExeName;
  77.     if high(commands)>=0
  78.        then for i:=low(Commands) to high(Commands)
  79.                 do P.Parameters.Add(Commands[i]);
  80.     Result := (__RunCommand(P,StdOut,StdErr,ExitStatus)=0) and (ExitStatus=0);
  81.   end;
  82.  

The original RunCommand ignores the output to StdErr unless you use flags for redirecting it into stdout. So it can be overloaded so you can choose to ignore or not to ignore stderr. I'd like to propose this code cleanup to the official release, but not sure where I should submit it.

Leledumbo

  • Hero Member
  • *****
  • Posts: 7743
  • Programming + Glam Metal + Tae Kwon Do = Me
Re: Process.pp - ugly code for RunCommand
« Reply #1 on: October 13, 2017, 05:48:52 am »
The bugtracker also serves for code improvement, post your svn diff against trunk there.

kris

  • Jr. Member
  • **
  • Posts: 54
Re: Process.pp - ugly code for RunCommand
« Reply #2 on: October 13, 2017, 06:07:32 am »
I didn't alter anything in the process.pp, I wrote my own cleaned version of RunCommand in a separate unit so the only way I can share diff to you is via a hosted online diff viewer:
https://www.diffchecker.com/m4wnijjz


Leledumbo

  • Hero Member
  • *****
  • Posts: 7743
  • Programming + Glam Metal + Tae Kwon Do = Me
Re: Process.pp - ugly code for RunCommand
« Reply #3 on: October 13, 2017, 08:06:14 am »
There's no other way if you want your code to be integrated in fpc, unless someone is willing enough to do it in behalf of you, which I'm not.

kris

  • Jr. Member
  • **
  • Posts: 54
Re: Process.pp - ugly code for RunCommand
« Reply #4 on: October 13, 2017, 09:10:35 am »
no problem, I am willing to do it, just pls explain exactly what I need to do.
I modify the original process.pp file, and then what? I upload here? Or I upload link to online hosted diff view of the unmodified and modified files?

kris

  • Jr. Member
  • **
  • Posts: 54
Re: Process.pp - ugly code for RunCommand
« Reply #5 on: October 13, 2017, 09:25:10 am »
process.pp diff: https://www.diffchecker.com/NGTrZrVd

and attached is the modified process.pp file.

Anything else I should do, please explain steps.

Thaddy

  • Hero Member
  • *****
  • Posts: 4779
Re: Process.pp - ugly code for RunCommand
« Reply #6 on: October 13, 2017, 10:19:40 am »
As usual (Leledumbo you know better...) Platform please... I am afraid this is not cross-platform after a little test.I may be wrong.
But plz always give the platform and compiler version. So what you can do is test against other platforms.

There is no sane way to debug these kind of things unless you give us code+platform+fpcversion
"Logically, no number of positive outcomes at the level of experimental testing can confirm a scientific theory, but a single counterexample is logically decisive."

kris

  • Jr. Member
  • **
  • Posts: 54
Re: Process.pp - ugly code for RunCommand
« Reply #7 on: October 13, 2017, 10:25:49 am »
I think it should be platform independent because I did not make ANY changes within any IFDEF blocks. And the changes I did make are basically reorganizing code to remove redundancy. Nothing really that would break anything platform-specific.

My platform is Ubuntu Linux x86_64, fpc3.0.0


ccrause

  • Jr. Member
  • **
  • Posts: 78
Re: Process.pp - ugly code for RunCommand
« Reply #8 on: October 13, 2017, 10:48:51 am »
Since 3.0.2 the parameters for RunCommand(InDir) changed a bit and now also include a parameter Options.  Refer to this wiki entry on how to submit patches, this will make it easier for developers to integrate user contributions. Note that patches based on recent trunk versions are preferred.

kris

  • Jr. Member
  • **
  • Posts: 54
Re: Process.pp - ugly code for RunCommand
« Reply #9 on: October 13, 2017, 10:56:27 am »
alright thanks.

Thaddy

  • Hero Member
  • *****
  • Posts: 4779
Re: Process.pp - ugly code for RunCommand
« Reply #10 on: October 13, 2017, 11:01:37 am »
Note that patches based on recent trunk versions are preferred.
Correct. It also means you have to test your patch against the most recent trunk.
"Logically, no number of positive outcomes at the level of experimental testing can confirm a scientific theory, but a single counterexample is logically decisive."

marcov

  • Global Moderator
  • Hero Member
  • *****
  • Posts: 5875
Re: Process.pp - ugly code for RunCommand
« Reply #11 on: October 13, 2017, 11:14:49 am »
Don't bother. If I have time,  I'll go more with the spirit, and extract the common code, also because the patch has a totally different indentation style.

The idea was anyway to clean up internal* somehow and make it a method of tprocess, so that other people can easily make basic variants.
« Last Edit: October 13, 2017, 02:40:49 pm by marcov »

 

Recent

Get Lazarus at SourceForge.net. Fast, secure and Free Open Source software downloads Open Hub project report for Lazarus