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

GRASS GIS GRASS GIS developer
1

Martin,

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

Michael

On 5/16/07 2:09 AM, "landa@grass.itc.it" <landa@grass.itc.it> wrote:

Author: landa
Date: 2007-05-16 11:09:42 +0200 (Wed, 16 May 2007)
New Revision: 659

Modified:
   trunk/grassaddons/gui/gui_modules/mapdisp.py
Log:
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')

         if dlg.ShowModal() == wx.ID_CANCEL:
             dlg.Destroy()
             return

         wind = dlg.wind
- cmd = "g.region -ugp region=%s" % wind
+ cmdString = "g.region -ugp region=%s" % wind

- try:
- p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE,
close_fds=True)
+ p = cmd.Command (cmdString)

- output = p.stdout.read().split('\n')
+ if p.returncode == 0:
+ output = p.module_stdout.read().split('\n')
             for line in output:
                 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()

- if p.stdout < 0:
- print >> sys.stderr, "Child was terminated by signal",
p.stdout
- elif p.stdout > 0:
- #print >> sys.stderr, p.stdout
- pass
- except OSError, e:
- print >> sys.stderr, "Execution failed:", e
-
         dlg.Destroy()

     def SaveDisplayRegion(self, event):

_______________________________________________
grass-commit-addons mailing list
grass-commit-addons@grass.itc.it
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

2

Michael,

2007/5/16, Michael Barton <michael.barton@asu.edu>:

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

yes, I think so.

Martin

Michael

On 5/16/07 2:09 AM, "landa@grass.itc.it" <landa@grass.itc.it> wrote:

> Author: landa
> Date: 2007-05-16 11:09:42 +0200 (Wed, 16 May 2007)
> New Revision: 659
>
> Modified:
> trunk/grassaddons/gui/gui_modules/mapdisp.py
> Log:
> 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')
>
> if dlg.ShowModal() == wx.ID_CANCEL:
> dlg.Destroy()
> return
>
> wind = dlg.wind
> - cmd = "g.region -ugp region=%s" % wind
> + cmdString = "g.region -ugp region=%s" % wind
>
> - try:
> - p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE, stderr=PIPE,
> close_fds=True)
> + p = cmd.Command (cmdString)
>
> - output = p.stdout.read().split('\n')
> + if p.returncode == 0:
> + output = p.module_stdout.read().split('\n')
> for line in output:
> 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()
>
> - if p.stdout < 0:
> - print >> sys.stderr, "Child was terminated by signal",
> p.stdout
> - elif p.stdout > 0:
> - #print >> sys.stderr, p.stdout
> - pass
> - except OSError, e:
> - print >> sys.stderr, "Execution failed:", e
> -
> dlg.Destroy()
>
> def SaveDisplayRegion(self, event):
>
> _______________________________________________
> grass-commit-addons mailing list
> grass-commit-addons@grass.itc.it
> 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

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

3

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

On 5/16/07 8:32 AM, "Martin Landa" <landa.martin@gmail.com> wrote:

Michael,

2007/5/16, Michael Barton <michael.barton@asu.edu>:

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

yes, I think so.

Martin

Michael

On 5/16/07 2:09 AM, "landa@grass.itc.it" <landa@grass.itc.it> wrote:

Author: landa
Date: 2007-05-16 11:09:42 +0200 (Wed, 16 May 2007)
New Revision: 659

Modified:
   trunk/grassaddons/gui/gui_modules/mapdisp.py
Log:
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')

         if dlg.ShowModal() == wx.ID_CANCEL:
             dlg.Destroy()
             return

         wind = dlg.wind
- cmd = "g.region -ugp region=%s" % wind
+ cmdString = "g.region -ugp region=%s" % wind

- try:
- p = Popen(cmd, shell=True, stdin=PIPE, stdout=PIPE,
stderr=PIPE,
close_fds=True)
+ p = cmd.Command (cmdString)

- output = p.stdout.read().split('\n')
+ if p.returncode == 0:
+ output = p.module_stdout.read().split('\n')
             for line in output:
                 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()

- if p.stdout < 0:
- print >> sys.stderr, "Child was terminated by signal",
p.stdout
- elif p.stdout > 0:
- #print >> sys.stderr, p.stdout
- pass
- except OSError, e:
- print >> sys.stderr, "Execution failed:", e
-
         dlg.Destroy()

     def SaveDisplayRegion(self, event):

_______________________________________________
grass-commit-addons mailing list
grass-commit-addons@grass.itc.it
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

__________________________________________
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

4

Michael Barton wrote:

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 <glynn@gclements.plus.com>

5

Glynn,

2007/5/16, Glynn Clements <glynn@gclements.plus.com>:

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

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

Martin

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

_______________________________________________
grassgui mailing list
grassgui@grass.itc.it
http://grass.itc.it/mailman/listinfo/grassgui

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

6

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

On 5/16/07 10:15 AM, "Martin Landa" <landa.martin@gmail.com> wrote:

Glynn,

2007/5/16, Glynn Clements <glynn@gclements.plus.com>:

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.

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

Martin

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

_______________________________________________
grassgui mailing list
grassgui@grass.itc.it
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

7

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

2007/5/16, Michael Barton <michael.barton@asu.edu>:

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

On 5/16/07 10:15 AM, "Martin Landa" <landa.martin@gmail.com> wrote:

> Glynn,
>
> 2007/5/16, Glynn Clements <glynn@gclements.plus.com>:
>
>>> 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.
>
> OK, then we need to fix Command class... (I will look at it tomorrow)
>
> Martin
>
>> --
>> Glynn Clements <glynn@gclements.plus.com>
>>
>> _______________________________________________
>> grassgui mailing list
>> grassgui@grass.itc.it
>> 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 <landa.martin@gmail.com> * http://gama.fsv.cvut.cz/~landa *

8

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@gclements.plus.com>

9

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

On 5/17/07 8:56 AM, "Glynn Clements" <glynn@gclements.plus.com> wrote:

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.

__________________________________________
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

10

Michael Barton wrote:

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 <glynn@gclements.plus.com>

11

On 5/17/07 3:17 PM, "Glynn Clements" <glynn@gclements.plus.com> wrote:

Michael Barton wrote:

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.

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

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

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

12

Michael Barton wrote:

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

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

> AFAICT, GUI.parseCommand() is probably still broken:
>
> 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.

Ah; okay.

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

13

On 5/17/07 6:30 PM, "Glynn Clements" <glynn@gclements.plus.com> wrote:

Michael Barton wrote:

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.

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

Thanks. This is a good example.

Michael

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

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.

Ah; okay.

__________________________________________
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

14

Glynn,

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

Martin

2007/5/18, Michael Barton <michael.barton@asu.edu>:

On 5/17/07 6:30 PM, "Glynn Clements" <glynn@gclements.plus.com> wrote:

>
> Michael Barton wrote:
>
>>>> 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.
>>
>> 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])
>

Thanks. This is a good example.

Michael

>
>>> AFAICT, GUI.parseCommand() is probably still broken:
>>>
>>> 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.
>
> Ah; okay.

__________________________________________
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 <landa.martin@gmail.com> * http://gama.fsv.cvut.cz/~landa *