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

Michael,

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

yes, I think so.

Martin

Post by Michael Barton
Michael

__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics & Complexity
Arizona State University
phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

--
Martin Landa <***@gmail.com> * http://gama.fsv.cvut.cz/~landa *

Michael Barton

2007-11-14 22:56:50 UTC

Seems like a good idea, especially for updating in the future. For all new
commands, I'll follow this syntax. As we all have time, we can go back and
change the existing code. It shouldn't be a huge task.

Michael

Post by Martin Landa
Michael,

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

yes, I think so.
Martin

Post by Michael Barton
Michael

Author: landa
Date: 2007-05-16 11:09:42 +0200 (Wed, 16 May 2007)
New Revision: 659
trunk/grassaddons/gui/gui_modules/mapdisp.py
fixing 'Zoom to saved region'
Modified: trunk/grassaddons/gui/gui_modules/mapdisp.py
===================================================================
--- trunk/grassaddons/gui/gui_modules/mapdisp.py 2007-05-15 23:55:41 UTC
(rev
658)
+++ trunk/grassaddons/gui/gui_modules/mapdisp.py 2007-05-16 09:09:42 UTC
(rev
659)
@@ -851,21 +851,21 @@
zoomreg = {}
dlg = SavedRegion(self, wx.ID_ANY, "Zoom to saved region extents",
- pos=wx.DefaultPosition, size=wx.DefaultSize,
- style=wx.DEFAULT_DIALOG_STYLE,
- loadsave='load')
+ pos=wx.DefaultPosition, size=wx.DefaultSize,
+ style=wx.DEFAULT_DIALOG_STYLE,
+ loadsave='load')
dlg.Destroy()
return
wind = dlg.wind
- cmd = "g.region -ugp region=%s" % wind
+ cmdString = "g.region -ugp region=%s" % wind
- p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE,
stderr=PIPE,
close_fds=True)
+ p = cmd.Command (cmdString)
- output = p.stdout.read().split('\n')
+ output = p.module_stdout.read().split('\n')
line = line.strip()
if '=' in line: key,val = line.split('=')
@@ -877,14 +877,6 @@
self.ZoomHistory(self.Map.region['n'],self.Map.region['s'],self.Map.region['
e'
],self.Map.region['w'])
self.UpdateMap()
- print >> sys.stderr, "Child was terminated by signal",
p.stdout
- #print >> sys.stderr, p.stdout
- pass
- print >> sys.stderr, "Execution failed:", e
-
dlg.Destroy()
_______________________________________________
grass-commit-addons mailing list
http://grass.itc.it/mailman/listinfo/grass-commit-addons

__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics & Complexity
Arizona State University
phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

Glynn Clements

2007-11-14 22:56:50 UTC

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

+ cmdString = "g.region -ugp region=%s" % wind
+ p = cmd.Command (cmdString)

This interface is broken, and needs to be replaced.

self.module = subprocess.Popen(self.cmd, shell=True,
^^^^^^^^^^

Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong.

Just how clearly do I have to say this before it sinks in:

DO NOT USE THE SHELL

Using the shell means problems with spaces, quotes and other shell
metacharacters. There is no reason to use it. So don't.

--
Glynn Clements <***@gclements.plus.com>

Martin Landa

2007-11-14 22:56:50 UTC

Glynn,

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

+ cmdString = "g.region -ugp region=%s" % wind
+ p = cmd.Command (cmdString)

This interface is broken, and needs to be replaced.
self.module = subprocess.Popen(self.cmd, shell=True,
^^^^^^^^^^
Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong.
DO NOT USE THE SHELL
Using the shell means problems with spaces, quotes and other shell
metacharacters. There is no reason to use it. So don't.

OK, then we need to fix Command class... (I will look at it tomorrow)

Martin

Post by Glynn Clements
--
_______________________________________________
grassgui mailing list
http://grass.itc.it/mailman/listinfo/grassgui

--
Martin Landa <***@gmail.com> * http://gama.fsv.cvut.cz/~landa *

Michael Barton

2007-11-14 22:56:50 UTC

The good thing about using the command class is that if we get it right
THERE, then it will be right throughout the GUI, and we won't have to change
each instance that we call a GRASS command.

This was the gist of my post. Martin is doing the cmd.py module and can fix
it so that it doesn't use SHELL.

Michael

Post by Martin Landa
Glynn,

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

+ cmdString = "g.region -ugp region=%s" % wind
+ p = cmd.Command (cmdString)

This interface is broken, and needs to be replaced.
self.module = subprocess.Popen(self.cmd, shell=True,
^^^^^^^^^^
Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong.
DO NOT USE THE SHELL
Using the shell means problems with spaces, quotes and other shell
metacharacters. There is no reason to use it. So don't.

OK, then we need to fix Command class... (I will look at it tomorrow)
Martin

Post by Glynn Clements
--
_______________________________________________
grassgui mailing list
http://grass.itc.it/mailman/listinfo/grassgui

__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics and Complexity
Arizona State University

phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

Martin Landa

2007-11-14 22:56:50 UTC

Hi,

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,

Martin

Post by Michael Barton
The good thing about using the command class is that if we get it right
THERE, then it will be right throughout the GUI, and we won't have to change
each instance that we call a GRASS command.
This was the gist of my post. Martin is doing the cmd.py module and can fix
it so that it doesn't use SHELL.
Michael

Post by Martin Landa
Glynn,

Post by Michael Barton
Are you thinking that we should change all calls to GRASS commands to use
the new cmd module?

+ cmdString = "g.region -ugp region=%s" % wind
+ p = cmd.Command (cmdString)

This interface is broken, and needs to be replaced.
self.module = subprocess.Popen(self.cmd, shell=True,
^^^^^^^^^^
Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong. Wrong.
DO NOT USE THE SHELL
Using the shell means problems with spaces, quotes and other shell
metacharacters. There is no reason to use it. So don't.

OK, then we need to fix Command class... (I will look at it tomorrow)
Martin

Post by Glynn Clements
--
_______________________________________________
grassgui mailing list
http://grass.itc.it/mailman/listinfo/grassgui

__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics and Complexity
Arizona State University
phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

--
Martin Landa <***@gmail.com> * http://gama.fsv.cvut.cz/~landa *

Glynn Clements

2007-11-14 22:56:50 UTC

Post by Martin Landa
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 <***@gclements.plus.com>

Michael Barton

2007-11-14 22:56:50 UTC

This is helpful. But I will wait to make any changes until this is worked
out--since it looks like we will also need to redefine the original command
string (i.e., to make it a proper list) prior to calling cmd.Command.

Michael

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
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

2007-11-14 22:56:50 UTC

Post by Michael Barton
This is helpful. But I will wait to make any changes until this is worked
out--since it looks like we will also need to redefine the original command
string (i.e., to make it a proper list) prior to calling cmd.Command.

I've fixed cmd.Command itself and all of the straightforward uses.

AFAICT, GUI.parseCommand() is probably still broken:

menuform.py:1075: cmdlst = cmd.split(' ')

--
Glynn Clements <***@gclements.plus.com>

Michael Barton

2007-11-14 22:56:50 UTC

I've fixed cmd.Command itself and all of the straightforward uses.

So we'll need to send it a list consisting of [command+flags, option=val,
option=val,...]. Right?

Post by Glynn Clements
menuform.py:1075: cmdlst = cmd.split(' ')

This actually might be fine. It looks like it is only checking to see if
there is a space in the command string and returning an error message is
there is. It making sure that the only thing being passed on to menuform is
a simple GRASS command without arguments.

Michael

__________________________________________
Michael Barton, Professor of Anthropology
School of Human Evolution & Social Change
Center for Social Dynamics and Complexity
Arizona State University

phone: 480-965-6213
fax: 480-965-7671
www: http://www.public.asu.edu/~cmbarton

Glynn Clements

2007-11-14 22:56:50 UTC

I've fixed cmd.Command itself and all of the straightforward uses.

So we'll need to send it a list consisting of [command+flags, option=val,
option=val,...]. Right?

Example:

dbDescribe = cmd.Command (cmd = ["db.describe", "-c",
"table=%s" % self.parent.tablename,
"driver=%s" % self.parent.driver,
"database=%s" % self.parent.database])

Post by Glynn Clements
menuform.py:1075: cmdlst = cmd.split(' ')

Ah; okay.

--
Glynn Clements <***@gclements.plus.com>

Michael Barton

2007-11-14 22:56:50 UTC

I've fixed cmd.Command itself and all of the straightforward uses.

So we'll need to send it a list consisting of [command+flags, option=val,
option=val,...]. Right?

dbDescribe = cmd.Command (cmd = ["db.describe", "-c",
"table=%s" % self.parent.tablename,
"driver=%s" % self.parent.driver,
"database=%s" % self.parent.database])

Thanks. This is a good example.

Michael

Post by Glynn Clements
menuform.py:1075: cmdlst = cmd.split(' ')

Ah; okay.

Martin Landa

2007-11-14 22:56:50 UTC

Glynn,

sorry I was some days out of the office, anyway thanks for fixing it!
And also thanks for explanation.

Martin

I've fixed cmd.Command itself and all of the straightforward uses.

So we'll need to send it a list consisting of [command+flags, option=val,
option=val,...]. Right?

dbDescribe = cmd.Command (cmd = ["db.describe", "-c",
"table=%s" % self.parent.tablename,
"driver=%s" % self.parent.driver,
"database=%s" % self.parent.database])

Thanks. This is a good example.
Michael