Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#2189 closed defect (fixed)

PyGRASS Module does not work on Windows

Reported by: annakrat Owned by: grass-dev@…
Priority: normal Milestone: 7.0.0
Component: Python Version: svn-trunk
Keywords: pygrass Cc:
CPU: Unspecified Platform: MSWindows 8

Description

PyGRASS Module class is not working on Windows. It can't find the command:

import grass.pygrass.modules as pymod
region = pymod.Module("g.region")

GrassError: 'Error running: `g.region --interface-description`.'

If I enter the entire path of the command, it works:

egion = pymod.Module(r"C:\OSGeo4W_dev\apps\grass\grass-7.0.svn\bin\g.region")
region.description
Manages the boundary definitions for the geographic region.

This ticket is related to #2067 and #2188.

Maybe PyGRASS could be a separate component in trac?

Attachments (1)

module.diff (2.1 KB) - added by zarch 6 years ago.
Patch to use Popen from grass.script.core

Download all attachments as: .zip

Change History (7)

comment:1 in reply to:  description ; Changed 6 years ago by zarch

Replying to annakrat:

PyGRASS Module class is not working on Windows. It can't find the command:

import grass.pygrass.modules as pymod
region = pymod.Module("g.region")

GrassError: 'Error running: `g.region --interface-description`.'

If I enter the entire path of the command, it works:


May be it is only a PATH problem? are you able to run the command from the terminal?, I'm just using:

(http://trac.osgeo.org/grass/browser/grass/trunk/lib/python/pygrass/modules/interface/module.py#L243)

# call the command with --interface-description
get_cmd_xml = subprocess.Popen([cmd, "--interface-description"],
                               stdout=subprocess.PIPE)


So from my understanding also:

core.run_command("g.region", "p")

Should not find the command. Someone has an idea why this is happening? Both are using the same python class, and both avoid to set any special environmental variable...


Maybe PyGRASS could be a separate component in trac?

For me it's fine.

comment:2 in reply to:  1 ; Changed 6 years ago by annakrat

Replying to zarch:

Replying to annakrat:

PyGRASS Module class is not working on Windows. It can't find the command:

import grass.pygrass.modules as pymod
region = pymod.Module("g.region")

GrassError: 'Error running: `g.region --interface-description`.'

If I enter the entire path of the command, it works:


May be it is only a PATH problem? are you able to run the command from the terminal?, I'm just using:

(http://trac.osgeo.org/grass/browser/grass/trunk/lib/python/pygrass/modules/interface/module.py#L243)

# call the command with --interface-description
get_cmd_xml = subprocess.Popen([cmd, "--interface-description"],
                               stdout=subprocess.PIPE)


So from my understanding also:

core.run_command("g.region", "p")

Should not find the command. Someone has an idea why this is happening? Both are using the same python class, and both avoid to set any special environmental variable...

run_command works. The difference is passing shell=True in Popen (as here) and adding this helps. Python documentation warns against using this but we are using it anyway in script.py. I added it in r58902. I haven't checked if there are other places in pygrass where it is needed.

Changed 6 years ago by zarch

Attachment: module.diff added

Patch to use Popen from grass.script.core

comment:3 Changed 6 years ago by zarch

Replying to annakrat:

Should not find the command. Someone has an idea why this is happening? Both are using the same python class, and both avoid to set any special environmental variable...

run_command works. The difference is passing shell=True in Popen

Thanks, I didn't notice that we were using a modify version of Popen!

I added it in r58902. I haven't checked if there are other places in pygrass where it is needed.

May be I should just use the grass.script.core.Popen class instead. I would like to reduce duplicated code, especially these tricky parts that are platform dependent, do you mind If I revert the commit r58902, and apply the attached changes?

comment:4 in reply to:  3 ; Changed 6 years ago by annakrat

Replying to zarch:

Replying to annakrat:

Should not find the command. Someone has an idea why this is happening? Both are using the same python class, and both avoid to set any special environmental variable...

run_command works. The difference is passing shell=True in Popen

Thanks, I didn't notice that we were using a modify version of Popen!

I added it in r58902. I haven't checked if there are other places in pygrass where it is needed.

May be I should just use the grass.script.core.Popen class instead. I would like to reduce duplicated code, especially these tricky parts that are platform dependent, do you mind If I revert the commit r58902, and apply the attached changes?

Sure, that's probably a good idea.

comment:5 in reply to:  4 Changed 6 years ago by zarch

Resolution: fixed
Status: newclosed

Replying to annakrat:

Replying to zarch:

May be I should just use the grass.script.core.Popen class instead. I would like to reduce duplicated code, especially these tricky parts that are platform dependent, do you mind If I revert the commit r58902, and apply the attached changes?

Sure, that's probably a good idea.


ok, done in r58932. Anyway thank you for your help, you've found the solution of the problem! :-)

I've tested on Win7 and it seems to work properly to me, so I'm closing this ticket as fixed.

comment:6 in reply to:  2 Changed 6 years ago by glynn

Replying to annakrat:

Python documentation warns against using this but we are using it anyway in script.py.

The warning in the Python documentation is misleading. It's not that shell=True is a problem by itself. The problem is shell=True in combination with passing the command as a string rather than a list, where the string is dynamically generated.

On Unix, passing the command as a string practically requires shell=True (passing a string with shell=False will treat the entire string as the path to an executable, which will be executed without arguments).

As the documentation notes, this combination risks an injection attack. If a chunk of text which is supposed to be a single argument or part of an argument contains shell metcharacters (spaces, quotes, etc), it can end up supplying additional arguments or even additional commands to be executed. Avoiding the shell and passing the command as a list avoids this problem.

On Windows, the fundamental execution primitive (CreateProcess) takes a string rather than a list of strings. A list passed to subprocess.Popen() is always converted to a string by "inverting" the rules by which MSVCRT parses the string back to an argument list.

There, the only difference between shell=False and shell=True is that the latter prepends "cmd.exe /c " to the resulting string (actually, if the COMSPEC environment variable is set, its value is used in place of "cmd.exe"). This is the same behaviour as MSVCRT's system() function. The end result is that shell=True allows you to execute scripts (including batch files), documents, etc, whereas shell=False only works for binary executables (execution of scripts is implemented by the shell, rather than by the kernel as is the case on Unix).

Note: See TracTickets for help on using tickets.