output r.out.xyz

[pvanbosgeo]

According to the help file,

separator=character

Field separator Special characters: newline, space, comma, tab Default: |

However, in the output it seems a space rather than | is used by default.

[pvanbosgeo]

In addition, the separator | is not accepted at all. Using it will result in an output without separator.

[hcho]

I confirmed this issue. Related to r57905. Not sure why the pipe character was disabled.

[hcho]

Fixed the usage and displays a warning when | is used. (r60569)

annakrat: Is the | issue on Windows only with python scripts and not with binary modules? What exactly is the problem?

[glynn]

Replying to hcho:

I confirmed this issue. Related to r57905. Not sure why the pipe character was disabled.

r60591 reverts r57905 and r60569. Vertical bar ("pipe") character is again allowed, and is the default.

[annakrat]

Replying to hcho:

Fixed the usage and displays a warning when | is used. (r60569)

annakrat: Is the | issue on Windows only with python scripts and not with binary modules? What exactly is the problem?

Here is the problem a user had with running r.out.xyz:

​http://lists.osgeo.org/pipermail/grass-user/2013-October/069019.html

I remember having these problems with pipe as separator when calling a module from python script on Windows. It seemed that parsing of the command failed and it tried to evaluate whatever was after pipe as another command. So I think, reverting might break it again but we can see tomorrow if this issue is still there.

[hcho]

I didn't try on Windows, but I think this is a problem with wxGUI, not with r.out.xyz, so we should fix the wxGUI console. We cannot work around this issue for all other modules that have a separator option. Maybe, double quotes around a pipe?

[pvanbosgeo]

May I offer a suggestion as an user? Disabling hitherto default parameter settings are fairly important as they will break user-made scripts. It did for me, and it wasn't easy to find out why they didn't work anymore. More so because the | was still listed as the default in the help files.

So, whenever such changes are made, it would be good to change it in the help file and/or put them on a list like ​http://trac.osgeo.org/grass/wiki/Grass7/NewFeatures (but perhaps a separate list for the development version, don't know).

I am fully aware that time may be an impediment (I am very grateful for all the time you are already putting into improving the software), and I know that by using the development version I run the risk of things breaking, but still, if possible such changes in the help file could be helpful and also make it easier to spot potential bugs, etc.

[pvanbosgeo]

As an additional comment, wouldn't it be possible to simply change the default, but keeping the | as an option? I for one always declare all parameters in my scripts, even if I am using the default settings, precisely to avoid problems if ever the default is changed. Of course, that doesn't help when some options are removed altogether.

[annakrat]

Replying to pvanbosgeo:

May I offer a suggestion as an user? Disabling hitherto default parameter settings are fairly important as they will break user-made scripts. It did for me, and it wasn't easy to find out why they didn't work anymore. More so because the | was still listed as the default in the help files.

Sorry for that but the intention wasn't to disable it but to fix it (although it was more a workaround than fixing). Now I see what was wrong, I expected r.stats, the underlying module, to have | as default separator parameter but it has space instead (btw is there any reason for this inconsistency?). My intention was to not include | in the parameters with which r.stats is called (because that was causing problem) and instead make r.stats use its default separator.

[pvanbosgeo]

Thanks for the explanation and apologies for my wrong interpretation

[annakrat]

Replying to hcho:

I didn't try on Windows, but I think this is a problem with wxGUI, not with r.out.xyz, so we should fix the wxGUI console. We cannot work around this issue for all other modules that have a separator option. Maybe, double quotes around a pipe?

It's not problem of wxGUI but maybe of Python scripting library. Today's version of r.out.xyz on Windows gives this error when you specify | or you leave default:

The syntax of the command is incorrect.

Adding quotes around pipe (for example in _make_val function in grass.core) works but results in "|" as a separator (3 characters) which is wrong. So, now we have r.out.xyz broken on Windows. Any ideas?

[wenzeslaus]

Replying to annakrat:

Any ideas?

• Use separator=pipe instead of separator=|, similarly to separator=space.
• Fix G7:r.stats, the default separator for all other modules is a pipe. (And than apply again the patch.)
• Another idea would be to use some other separator by default which would discourage usage of pipe by MS Windows users (which anyway must be very confused from that character, I would say). A space or comma could be much natural separators (e.g., considering putting the data to some spreadsheet or computational environment such as R). This does not solve the actual problem but it might completely avoid it on MS Windows and user of other systems can still use the pipe if they want.

[hcho]

separator=pipe is added in r60614. G_OPT_F_SEP defaults to "pipe", not to "|". I think it should fix the problem.

[glynn]

Replying to annakrat:

It's not problem of wxGUI but maybe of Python scripting library. Today's version of r.out.xyz on Windows gives this error when you specify | or you leave default:

The syntax of the command is incorrect.

How are you running the script?

If it isn't via cmd.exe in a console window, with parameters provided on the command line, then that's the first thing to try. Any problems when executing via wxGUI or using the parameter dialogs should be assumed to be problems with those features until proven otherwise.

Adding quotes around pipe (for example in _make_val function in grass.core) works but results in "|" as a separator (3 characters) which is wrong. So, now we have r.out.xyz broken on Windows. Any ideas?

If a command is executed via e.g. grass.run_command(), Python's subprocess.Popen() provides the necessary quoting.

[glynn]

Replying to hcho:

separator=pipe is added in r60614. G_OPT_F_SEP defaults to "pipe", not to "|". I think it should fix the problem.

It adds a few new ones, namely that Python scripts (which don't use G_option_to_separator()) are now trying to use the literal string "pipe" as a separator. E.g.

$ echo "170.510125 -45.868537" | m.proj -i input=-
WARNING: Invalid field separator, using 'p'
WARNING: Invalid field separator, using 'p'
-4813902.92p-9497864.79p0.00

G_OPT_F_SEP is used in r.out.xyz, v.in.lines, r.tileset and m.proj.

r.out.xyz shouldn't be a problem, as the option is simply being passed through to r.stats.

The other three all use the option value.

v.in.lines and m.proj explicitly understand space, tab and comma (although I'm not sure that they interpret tab correctly; they appear to treat it as a synonym for space).

r.tileset doesn't understand any of the names, interpreting the value literally.

Also, none of this changes the fact that there appears to be a bug somewhere. There shouldn't be any problems with using a literal vertical bar character. If there are such problems, they should be fixed, not simply worked around so that we can pretend there isn't a problem.

While there are valid reasons for supporting "separator=pipe" (e.g. not forcing users to understand how quoting/escaping works in their preferred shell), if it results in the original problem being forgotten about, it may need to be reverted until the problem is fixed.

[hcho]

Replying to glynn:

It adds a few new ones, namely that Python scripts (which don't use G_option_to_separator()) are now trying to use the literal string "pipe" as a separator. E.g.

$ echo "170.510125 -45.868537" | m.proj -i input=-
WARNING: Invalid field separator, using 'p'
WARNING: Invalid field separator, using 'p'
-4813902.92p-9497864.79p0.00

G_OPT_F_SEP is used in r.out.xyz, v.in.lines, r.tileset and m.proj.

r.out.xyz shouldn't be a problem, as the option is simply being passed through to r.stats.

The other three all use the option value.

v.in.lines and m.proj explicitly understand space, tab and comma (although I'm not sure that they interpret tab correctly; they appear to treat it as a synonym for space).

r.tileset doesn't understand any of the names, interpreting the value literally.

I overlooked that some scripts use answers literally. Their default answer just happened to be "|", not "comma", or some other strings, so we didn't notice this issue so far.

Also, none of this changes the fact that there appears to be a bug somewhere. There shouldn't be any problems with using a literal vertical bar character. If there are such problems, they should be fixed, not simply worked around so that we can pretend there isn't a problem.

I agree if it can be fixed. E.g., the user can use "|", not "pipe" in the wxGUI console. The backtick issue was worked around because it couldn't be fixed (?) in Windows.

While there are valid reasons for supporting "separator=pipe" (e.g. not forcing users to understand how quoting/escaping works in their preferred shell),

That very reason was why I chose to add "pipe" and as a side effect, this issue was partially "taken care of".

if it results in the original problem being forgotten about, it may need to be reverted until the problem is fixed.

What about implementing grass.option_to_separator and keeping this ticket open until the original issue is fixed?

[annakrat]

Replying to glynn:

Replying to annakrat:

It's not problem of wxGUI but maybe of Python scripting library. Today's version of r.out.xyz on Windows gives this error when you specify | or you leave default:

The syntax of the command is incorrect.

How are you running the script?

Sorry I forgot to mention it, I tried to run it from wxGUI and terminal window and the error is the same. So I repeat it again, it's not the problem of wxGUI.

If a command is executed via e.g. grass.run_command(), Python's subprocess.Popen() provides the necessary quoting.

Apparently not.

[hcho]

I replied to the last email, but it doesn't appear here. Reposting..

Check r60634. ^{|& had to be escaped with} . I tested it on Windows 7 and it works.

[hcho]

ah! trac is eating carets. |& and caret need to be escaped with 3 carets

[hcho]

FYI, I tried "option=value" and option="value" before subprocess.Popen, but neither worked. It looks like we cannot avoid 7 carets.

[wenzeslaus]

Replying to hcho:

I replied to the last email, but it doesn't appear here. Reposting..

Things from here are send also to grass-dev but from grass-dev nothing goes here.

Check r60634. ^{|& had to be escaped with} . I tested it on Windows 7 and it works.

Check r60634. ^|& had to be escaped with ^^^. I tested it on Windows 7 and it works.

You need to use backtics `something` or tree curly brackets {{{something}}}.

Can you provide your testing examples? I can try to create a test from them.

[hcho]

r.out.xyz input=a sep=`~!@#$%^&*()-_=+[{]}\|;:,<.>/?"'"'"'

produces:

0.5`~!@#$%^&*()-_=+[{]}\|;:,<.>/?'"0.5`~!@#$%^&*()-_=+[{]}\|;:,<.>/?'"1

a is a single pixel map.

Single/double quotes were escaped by the other characters because shlex.split raises a "No closing quotation" error.

[glynn]

Replying to hcho:

The backtick issue was worked around because it couldn't be fixed (?) in Windows.

"Bactick issue"? Can you elaborate?

Backticks are a shell construct, so they shouldn't be applicable to GRASS 7, where use of the shell should have been almost entirely eliminated.

A vertical bar shouldn't be an issue for similar reasons. If it is an issue, likely candidates would be passing a string to subprocess.Popen() (or some interface built upon it) rather than a list, or possibly something like shlex in the GUI.

if it results in the original problem being forgotten about, it may need to be reverted until the problem is fixed.

What about implementing grass.option_to_separator and keeping this ticket open until the original issue is fixed?

That would be useful, but I'd suggest having the default value for G_OPT_F_SEP as "|" (literal vertical bar) rather than "pipe" until this gets resolved.

That may require someone who can reproduce the problem to do some debugging to determine exactly where this is going wrong.

I can't do it right now, as I've recently had to replace my Windows box and the new one doesn't have any development tools or libraries on it yet.

[glynn]

Replying to annakrat:

If a command is executed via e.g. grass.run_command(), Python's subprocess.Popen() provides the necessary quoting.

Apparently not.

Er, sort of. It quotes it, but the shell interprets | (and < and >) even when quoted.

>>> subprocess.call(["args.exe","hello","world","|"])
argc = 4
argv[ 0] = 'args.exe'
argv[ 1] = 'hello'
argv[ 2] = 'world'
argv[ 3] = '|'
0
>>> subprocess.call(["args.exe","hello","world","|"],shell=True)
The syntax of the command is incorrect.
1

Without shell=True, the command must be an exe or batch file:

>>> subprocess.call(["args.exe","hello","world"])
argc = 3
argv[ 0] = 'args.exe'
argv[ 1] = 'hello'
argv[ 2] = 'world'
0
>>> subprocess.call(["args.bat","hello","world"])

C:\Users\Glynn>args.exe hello world
argc = 3
argv[ 0] = 'args.exe'
argv[ 1] = 'hello'
argv[ 2] = 'world'
0
>>> subprocess.call(["args.py","hello","world"])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "C:\Program Files (x86)\Python27\lib\subprocess.py", line 493, in call
    return Popen(*popenargs, **kwargs).wait()
  File "C:\Program Files (x86)\Python27\lib\subprocess.py", line 679, in __init__
    errread, errwrite)
  File "C:\Program Files (x86)\Python27\lib\subprocess.py", line 893, in _execute_child
    startupinfo)
WindowsError: [Error 193] %1 is not a valid Win32 application

Currently, grass.Popen() sets shell=True for Windows, so that executing scripts works.

It's looking as if we'll need to change that, and require the use of batch files for executing scripts (however, I think that will have its own problems if any of the arguments contain |, < or >).

[hcho]

Replying to glynn:

Replying to annakrat:

If a command is executed via e.g. grass.run_command(), Python's subprocess.Popen() provides the necessary quoting.

Apparently not.

Er, sort of. It quotes it, but the shell interprets | (and < and >) even when quoted.

Quoting didn't help. I had to escape special characters with ^^^ in r60634. It looks like subprocess.Popen(shell=True) calls cmd.exe twice, so double escaping was needed. E.g., to escape |, ^^^|} needs to be fed into cmd.exe, but since cmd.exe is called twice, three ^ becomes six ^ and | becomes ^|. So we need ^^^^^^^|. I tested r60634 on Windows 7 with trunk and it worked.

Now back to G_OPT_F_SEP, since we fixed |, showing | or pipe as default is a preference only.

Regarding the backtick issue, it was not exactly an issue because cmd.exe does not support backticks anyway. I talked about g.mlist output=, which was implemented recently. There was no way to support backticks, so we had to implement output=.

[hcho]

More recent version: r60639

[neteler]

Replying to glynn:

Replying to hcho:

The backtick issue was worked around because it couldn't be fixed (?) in Windows.

"Bactick issue"? Can you elaborate?

Backticks are a shell construct, so they shouldn't be applicable to GRASS 7, where use of the shell should have been almost entirely eliminated.

For a (solved) issue, see ticket #1724.

[glynn]

Replying to hcho:

Er, sort of. It quotes it, but the shell interprets | (and < and >) even when quoted.

Quoting didn't help. I had to escape special characters with ^^^ in r60634.

r60634 appears to relate to wxGUI, which won't have any effect upon running the script normally. Also, it appears to apply the escaping regardless of whether shell=True or shell=False.

It looks like subprocess.Popen(shell=True) calls cmd.exe twice, so double escaping was needed.

Setting shell=True prepends "cmd.exe /c " to the command string. If the command is a .bat file, that would probably result in additional parsing.

E.g., to escape |, ^^^|} needs to be fed into cmd.exe, but since cmd.exe is called twice, three ^ becomes six ^ and | becomes ^|. So we need ^^^^^^^|. I tested r60634 on Windows 7 with trunk and it worked.

I think that the only robust solution here is to remove the shell from the equation altogether. The quoting required for MSVCRT is ugly enough, and adding another layer of quoting for cmd.exe makes it even worse. And there may be issues other than quoting waiting to be discovered (cmd.exe isn't particularly well documented, compared to e.g. the bash manual which runs to around 80 pages).

Now back to G_OPT_F_SEP, since we fixed |, showing | or pipe as default is a preference only.

No, we haven't fixed | yet. I think that modifying grass.Popen() not to use shell=True should fix it, but I don't know what relies upon the existing behaviour, or what to replace it with.

Regarding the backtick issue, it was not exactly an issue because cmd.exe does not support backticks anyway.

It does in the "for" command, which is sometimes used solely for its backtick feature; but that's a fairly ugly hack, and not something which should be suggested to users.

[hcho]

Replying to glynn:

I think that the only robust solution here is to remove the shell from the equation altogether. The quoting required for MSVCRT is ugly enough, and adding another layer of quoting for cmd.exe makes it even worse. And there may be issues other than quoting waiting to be discovered (cmd.exe isn't particularly well documented, compared to e.g. the bash manual which runs to around 80 pages).

Now back to G_OPT_F_SEP, since we fixed |, showing | or pipe as default is a preference only.

No, we haven't fixed | yet. I think that modifying grass.Popen() not to use shell=True should fix it, but I don't know what relies upon the existing behaviour, or what to replace it with.

Setting shell=False alone doesn't fix this problem. We still need ^^^| (4 less carets). Also, we lose useful cmd.exe commands (e.g., cd, dir, type, ...).

[glynn]

Replying to hcho:

Setting shell=False alone doesn't fix this problem. We still need ^^^| (4 less carets).

Why? The caret-escape mechanism is specific to cmd.exe, which is no longer used. The escaping needed to ensure that the command's "argv" array has the correct values is performed by the subprocess module (and doesn't involve carets).

Also, we lose useful cmd.exe commands (e.g., cd, dir, type, ...).

A Python script shouldn't be executing such commands, but should be using Python's own functionality. In particular, executing a "cd" command in a child process is pointless, as its effect is limited to the child process itself.

In any case r60679 removes all of the Popen() hackery from grass.script.core.py. grass.script.Popen() is now effectively an alias for subprocess.Popen().

Execution of (e.g.) Python scripts from within Python scripts will require either specifying the interpreter explicitly, or the use of a batch file, or specifying shell=True (and providing any necessary quoting).

Ultimately, it's impossible to quote correctly when shell=True, as it uses %COMSPEC% (if it's set) as the interpreter, and there's no guarantee that it will use exactly the same quoting rules as cmd.exe.

Technically, similar issues apply when shell=False, as it's up to the executable to parse the command line into arguments. Fortunately, the parsing rules are now documented and widely followed.

[wenzeslaus]

Now relates to ​discussion ([​http://osgeo-org.1560.x6.nabble.com/Re-GRASS-SVN-r60679-grass-trunk-lib-python-script-td5143645i20.html nabble) for r60679 because "shell=True" is used as a part of the proposed solution.

[martinl]

Still an issue?

[martinl]

Not reproducable in G76