Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15299004:
Add Process.shell and Process.runShell. (Closed)
|
Created:
7 years, 7 months ago by Anders Johnsen Modified:
7 years, 7 months ago CC:
reviews_dartlang.org, kustermann, ricow1, floitsch, Lasse Reichstein Nielsen Visibility:
Public. |
DescriptionAdd Process.shell and Process.runShell.
BUG=
R=sgjesse@google.com
Committed: https://code.google.com/p/dart/source/detail?r=23007
Patch Set 1 #
Total comments: 20
Patch Set 2 : Remove Process.shell and clean up impl. #
Total comments: 2
Patch Set 3 : Comment fix. #Patch Set 4 : Fix Windows impl. #
Total comments: 2
Messages
Total messages: 15 (0 generated)
CL for early API feedback. I have not yet tested on Windows.
Wouldn't this be better in a pub package? otherwise I like.
On 2013/05/17 14:13:23, floitsch wrote: > Wouldn't this be better in a pub package? > otherwise I like. I'm not sure if this should be in 'dart:io' or not. Do you have any use cases for this?
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:101: This method needs a comment. Should there be a way of specifying the actual shell command to use? Or should we just document the search strategy used. See comments below. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:108: I am not sure we should have both runShell and shell. The do almost the same. To be consistent I think when something returns ProcessResult it should be called runXXX. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:115: static String _getShellCommand() { Maybe we should do more extensive search of the shell to use. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:117: return "cmd"; Should we lookup %COMSPEC% in the Windows environment? Should it default to "%WINDIR%\system32\cmd.exe"? https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:119: return "sh"; Should we lookup $SHELL in the environment? Should it default to /bin/sh?
On 2013/05/17 14:38:45, kustermann wrote: > On 2013/05/17 14:13:23, floitsch wrote: > > Wouldn't this be better in a pub package? > > otherwise I like. > > I'm not sure if this should be in 'dart:io' or not. > Do you have any use cases for this? This is mostly based on a feature request from https://code.google.com/p/dart/issues/detail?id=1705.
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:133: commandLine = "$commandLine '$arg'"; Quadratic time string concatenation. Use a StringBuffer.
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:119: return "sh"; On 2013/05/17 14:48:56, Søren Gjesse wrote: > Should we lookup $SHELL in the environment? Should it default to /bin/sh? I don't really care what it defaults to, but I do think we should lookup $SHELL, I would assume that bash is used if SHELL is set to it https://codereview.chromium.org/15299004/diff/1/tests/standalone/io/process_s... File tests/standalone/io/process_shell_test.dart (right): https://codereview.chromium.org/15299004/diff/1/tests/standalone/io/process_s... tests/standalone/io/process_shell_test.dart:38: var command = options.executable; var command = "${options.executable} $path $args"; ?
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:132: arg = arg.replaceAll("'", "'\"'\"'"); You could use a multiline string here: """'"'"'""" Not confusing at all :)
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:109: static Future<ProcessResult> shell(String command, What is this method doing? https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:119: return "sh"; On 2013/05/21 07:26:34, ricow1 wrote: > On 2013/05/17 14:48:56, Søren Gjesse wrote: > > Should we lookup $SHELL in the environment? Should it default to /bin/sh? > I don't really care what it defaults to, but I do think we should lookup $SHELL, > I would assume that bash is used if SHELL is set to it The issue here is that different shells might interpret their arguments differently (zsh, bash, csh ..). As soon as we do '-c' we assume that the shell supports it. In addition to that, we assume that all shells interpret the arguments the same way (e.g. is "ls **/*dart" supported?). IMHO the safest way to do it is "/bin/sh -c"
https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:101: On 2013/05/17 14:48:56, Søren Gjesse wrote: > This method needs a comment. > > Should there be a way of specifying the actual shell command to use? Or should > we just document the search strategy used. See comments below. Done. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:108: On 2013/05/17 14:48:56, Søren Gjesse wrote: > I am not sure we should have both runShell and shell. The do almost the same. To > be consistent I think when something returns ProcessResult it should be called > runXXX. Done. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:109: static Future<ProcessResult> shell(String command, On 2013/05/21 07:50:00, kustermann wrote: > What is this method doing? It's gone. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:115: static String _getShellCommand() { On 2013/05/17 14:48:56, Søren Gjesse wrote: > Maybe we should do more extensive search of the shell to use. Done. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:117: return "cmd"; On 2013/05/17 14:48:56, Søren Gjesse wrote: > Should we lookup %COMSPEC% in the Windows environment? Should it default to > "%WINDIR%\system32\cmd.exe"? Done. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:119: return "sh"; On 2013/05/21 07:50:00, kustermann wrote: > On 2013/05/21 07:26:34, ricow1 wrote: > > On 2013/05/17 14:48:56, Søren Gjesse wrote: > > > Should we lookup $SHELL in the environment? Should it default to /bin/sh? > > I don't really care what it defaults to, but I do think we should lookup > $SHELL, > > I would assume that bash is used if SHELL is set to it > The issue here is that different shells might interpret their arguments > differently (zsh, bash, csh ..). > As soon as we do '-c' we assume that the shell supports it. In addition to that, > we assume that all shells interpret the arguments the same way (e.g. is "ls > **/*dart" supported?). > IMHO the safest way to do it is "/bin/sh -c" Done. https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:132: arg = arg.replaceAll("'", "'\"'\"'"); On 2013/05/21 07:31:51, Lasse Reichstein Nielsen wrote: > You could use a multiline string here: > """'"'"'""" > Not confusing at all :) Hehe, I prefer not ;) https://codereview.chromium.org/15299004/diff/1/sdk/lib/io/process.dart#newco... sdk/lib/io/process.dart:133: commandLine = "$commandLine '$arg'"; On 2013/05/21 07:13:42, Lasse Reichstein Nielsen wrote: > Quadratic time string concatenation. Use a StringBuffer. Done. https://codereview.chromium.org/15299004/diff/1/tests/standalone/io/process_s... File tests/standalone/io/process_shell_test.dart (right): https://codereview.chromium.org/15299004/diff/1/tests/standalone/io/process_s... tests/standalone/io/process_shell_test.dart:38: var command = options.executable; On 2013/05/21 07:26:34, ricow1 wrote: > var command = "${options.executable} $path $args"; > ? Done.
lgtm https://codereview.chromium.org/15299004/diff/13001/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/13001/sdk/lib/io/process.dart#n... sdk/lib/io/process.dart:106: * On UNIX systems, [:/bin/sh:] is used to execute the [executable]. On UNIX -> Linux and Mac OS
https://codereview.chromium.org/15299004/diff/13001/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/13001/sdk/lib/io/process.dart#n... sdk/lib/io/process.dart:106: * On UNIX systems, [:/bin/sh:] is used to execute the [executable]. On On 2013/05/22 07:50:39, Søren Gjesse wrote: > UNIX -> Linux and Mac OS Done.
Message was sent while issue was closed.
Committed patchset #4 manually as r23007 (presubmit successful).
Message was sent while issue was closed.
Sorry for the late reply. https://codereview.chromium.org/15299004/diff/21001/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/21001/sdk/lib/io/process.dart#n... sdk/lib/io/process.dart:145: arg = arg.replaceAll("'", "'\"'\"'"); I think there is something fishy with this replacement. arg' --converts-to--> arg'"'"' arg\' --converts-to--> arg\'"'"' Is this what we want? (I'm also unsure how cmd.exe treats such strings.)
Message was sent while issue was closed.
https://codereview.chromium.org/15299004/diff/21001/sdk/lib/io/process.dart File sdk/lib/io/process.dart (right): https://codereview.chromium.org/15299004/diff/21001/sdk/lib/io/process.dart#n... sdk/lib/io/process.dart:145: arg = arg.replaceAll("'", "'\"'\"'"); On 2013/05/22 11:54:26, kustermann wrote: > I think there is something fishy with this replacement. > arg' --converts-to--> arg'"'"' > arg\' --converts-to--> arg\'"'"' > Is this what we want? > > (I'm also unsure how cmd.exe treats such strings.) See https://codereview.chromium.org/15743002 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
