PyGRASS Module does not work on Windows

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

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?

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

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

[zarch]

Patch to use Popen from grass.script.core

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

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

[zarch]

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.

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