[GRASS-dev] Re: [GRASSGUI] Re: [grass-addons] r659 - trunk/grassaddons/gui/gui_modules

Glynn Clements glynn at gclements.plus.com
Thu May 17 11:56:35 EDT 2007


Martin Landa wrote:

> I am not quite sure if this patch OK...
> 
> -           self.module = subprocess.Popen(self.cmd, shell=True,
> +           cmdList = cmd.split(' ')
> +           self.module = subprocess.Popen(args=cmdList, shell=False,
>                                            stdin=subprocess.PIPE,
>                                            stdout=subprocess.PIPE,
>                                            stderr=subprocess.PIPE,

Nope. In fact, it's a good example of why the interface is wrong
(think about how it will behave if you have a space in an argument).

The problem isn't the implementation, it's the interface.

Let me put it another way: which of the following is correct?

1.	int main(char *cmd)
2.	int main(int argc, char **argv)

The correct answer is #2. A command isn't a string, it's a list of
strings, and that is the interface which you need to provide.

Taking a list of strings, combining them to form a single string, then
splitting the list into multiple strings will introduce errors, i.e. 
the list you end up with won't always be the original list.

The existing interface splits them according to the shell's syntax,
which has several problems; not least of which is that /bin/sh and
cmd.exe have radically different syntax, meaning that you would often
need to write separate Unix/Windows versions of any code which calls
cmd.Command().

You're proposed patch is worse, in that it becomes entirely impossible
to pass an argument which contains a space (these are quite common for
filenames on Windows).

The only realistic solution is not to combine them in the first place. 
IOW, the cmd parameter should be a list, and Popen should be called
as:

	self.module = subprocess.Popen(args=self.cmd,

You then need to change everything which calls cmd.Command() to pass a
list, not a string.

-- 
Glynn Clements <glynn at gclements.plus.com>




More information about the grass-dev mailing list