grass.script: Split the command string using shell-like syntax on the win32 platform same as on POSIX-compliant platforms #1908

tmszi · 2021-10-01T07:29:13Z

Closes #1063.

To Reproduce
Steps to reproduce the behavior:

Launch GRASS GIS
Launch wx0 monitor d.mon start=wx0
Try display some vector map with SQL query (where parameter), don't forget escaping MS Windows Command Propmt special characters < > ( ) & | , ; " with ^ carret symbol please, e.g. d.vect map=census where="cat ^> 10 AND cat ^< 20"

Additional context

More info is in commit message.

hellik · 2021-10-01T10:58:52Z

issue confirmed

hellik · 2021-10-01T15:41:25Z

don't forget escaping MS Windows Command Propmt >special characters < > ( ) & | , ; " with ^ carret symbol >please, e.g. d.vect map=census where="cat ^> 10 AND >cat ^< 20"

But why is

cat > 10

working without escaping?

tmszi · 2021-10-01T16:53:21Z

don't forget escaping MS Windows Command Propmt >special characters < > ( ) & | , ; " with ^ carret symbol >please, e.g. d.vect map=census where="cat ^> 10 AND >cat ^< 20"

But why is

cat > 10

working without escaping?

Yes you are right, because where=cat > 10 is in Python splitted as where=cat (and > 10 is stripped, because Command Prompt interpreted > as redirection, and in your current directory you can see file with name 10) , and therefore d.vect display all categories (not cat > 10). You can verify this on the wx0 monitor, but better example is where=cat < 10.

Example how d.vect census where=cat > 10 command from Command Prompt is internally splitted in the Python (check where= parameter):

['d.vect', 'map=census', 'layer=1', 'display=shape', 'type=point,line,area,face', 'where=cat', 'color=0:29:57', 'fill_color=0:103:204', 'width=0', 'width_scale=1', 'icon=basic/x', 'size=5', 'icon_area=legend/area', 'icon_line=legend/line', 'label_layer=1', 'label_color=red', 'label_bgcolor=none', 'label_bcolor=none', 'label_size=8', 'xref=left', 'yref=center']

wenzeslaus

Simpler code, aligned behavior across platforms (GUI Console is not system terminal), so I general yes.

I would appreciate more explanatory description (that is the final commit message and possibly also the PR description). Although I think I got the idea, I don't really get from the description what are the desired, current, and failing cases.

… Windows Same as on POSIX-compliant platforms. * e.g. run 'd.vect map=census where="cat > 10 AND cat < 20"' with where parameter from Command Prompt CMD/emulator terminal: non-POSIX platform ``` >>> import shlex >>> shlex.split('where="cat > 10 AND cat < 20"', posix=False) ['where="cat', '>', '10', 'AND', 'cat', '<', '20"'] ``` POSIX-compliant platform ``` >>> import shlex >>> shlex.split('where="cat > 10 AND cat < 20"', posix=True) ['where=cat > 10 AND cat < 20'] ``` Under OS MS Windows 'd.vect map=census where="cat > 10 AND cat < 20"' command runned from Command Prompt CMD require use ^ carret symbol for escaping special characters < > ( ) & | , ; ". e.g. 'd.vect map=census where="cat ^> 10 AND cat ^< 20"'

tmszi · 2023-09-27T07:41:32Z

Rebase 0772c39.

This is a significant fix and should be merged.

wenzeslaus

I'm for merging this, but I'm still not quite able to put all the pieces together.

When it happens and how the carret fits into this? The issue happens when using d.mon in terminal. What happens with d.vect in Console in GUI? Where is this function used and what strings it processes?

Is the function documentation still valid? The PR title would suggest it is not. The implementation has slightly different behavior for Windows but without explanation.

A test would at least clarify the intended behavior which would help clarifying the intention further.

I'm sorry for all the questions, but I guess insufficient documentation is what lead to the bad usage of this function in the first place.

wenzeslaus · 2023-09-27T17:19:48Z

A test for this could go into python/grass/script/tests and it is a great candidate for using pytest.

tmszi · 2023-09-27T19:55:32Z

When it happens and how the carret fits into this? The issue happens when using d.mon in terminal. What happens with d.vect in Console in GUI? Where is this function used and what strings it processes?

First read issue #1063 carefully please.

MS Windows platform Command Prompt CMD:

Display some vector map e.g. census and use where parameter:

A. Default behavior:

I did not find anywhere in the d.vect documentation manual that it is necessary to escape special characters < > ( ) & | , ; " with the ^ carret character when command is runned from CMD only. That's why I update doc of d.vect module.

Without carret symbol (d.vect module fail with error message):

# Launch display
d.mon start=wx0

# Display some vector map e.g. census and use where parameter
C:\Users\User>d.vect census where="cat < 10"
The system cannot find the path specified.

With carret char (d.vect module fail again with error message):

C:\Users\User>d.vect census where="cat ^< 10"
WARNING: Illegal filename <10">. Character <"> not allowed.
ERROR: Vector map <10"> not found

C:\Users\User>Syntax error: No closing quotationTraceback (most recent call last):
  File "C:\Program Files\GRASS GIS 8.4\Python39\lib\site-packages\wx\core.py", line 2346, in Notify
    self.notify()
  File "C:\Program Files\GRASS GIS 8.4\gui\wxpython\mapdisp\main.py", line 628, in watcher
    self.mapDisplay.GetMap().GetLayersFromCmdFile()
  File "C:\Program Files\GRASS GIS 8.4\gui\wxpython\mapdisp\main.py", line 191, in GetLayersFromCmdFile
    ltype = utils.command2ltype[cmd[0]]
IndexError: list index out of range

Internaly is commandline parsed in Python with following function (under OS MS Windows is used parameter posix=False, which is the source of incorrect parsing of the where parameter of d.vect module):

grass/python/grass/script/utils.py

Lines 308 to 310 in 5deae3a

    
           def split(s): 
        
               """!Platform specific shlex.split""" 
        
               return shlex.split(s, posix=(sys.platform != "win32"))

parsed result list:

['d.vect', 'map="census"', 'layer="1"', 'display="shape"', 'type="point,line,area,face"', 'where="cat', '<', '10"', 'color="0:29:57"', 'fill_color="0:103:204"', 'width=0', 'width_scale=1', 'icon="basic/x"', 'size=5', 'icon_area="legend/area"', 'icon_line="legend/line"', 'label_layer="1"', 'label_color="red"', 'label_bgcolor="none"', 'label_bcolor="none"', 'label_size=8', 'xref="left"', 'yref="center"']

Look at how is parsed where param: 'where="cat', '<', '10"' which is incorrect.

If function return shlex.split(s, posix=(sys.platform != "win32")) param posix argument is replaced with True on OS MS Windows return shlex.split(s.replace("\\", r"\\")) parsed commandline d.vect module where param argument is parsed correctly as 'where=cat < 10'

Expected behavior (solved with this PR):

C:\Users\User>d.vect census where="cat ^< 10"
d.vect complete.

MS Windows platform wxGUI console is not affected with changes in this PR and reported issue.

Is such an explanation sufficient to understand the problem and solve it?

Is the function documentation still valid? The PR title would suggest it is not. The implementation has slightly different behavior for Windows but without explanation.

Yes I agree func doc is not valid and explanation on MS Windows platform is needed too.

A test would at least clarify the intended behavior which would help clarifying the intention further.

if possible, I will add a test.

wenzeslaus · 2023-09-27T20:37:01Z

Thank you so much. With your new description, it is clear to me now. I see that the caret symbol is really a side issue here as the issue is always passing display commands as posix somewhere in d.mon the rendering machinery while expecting them as platform specific. In this context, I'm now curious why the backslashes need to be doubled on Windows but don't on Linux. You comment also clarifies why the split is not relevant to v.extract.

I agree with the addition of discussion of the caret symbol into the documentation of d.vect. It applies to v.extract as well as anything else, though, because it is a CMD thing, right? But that's an issue for later.

tmszi · 2023-09-30T04:39:37Z

I'm now curious why the backslashes need to be doubled on Windows but don't on Linux.

During my current testing, on the MS Windows 11, I found that this is not necessary, but I don't know reason why I added it (PR is from 2021). Originally I tested PR on MS Windows 10.

You comment also clarifies why the split is not relevant to v.extract

v.extract module doesn't use Python shlex.split() func.

python/grass/script/utils.py

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

petrasovaa · 2023-10-03T18:14:08Z

Just to give an overview where this is used:

./scripts/d.redraw/d.redraw.py:27:from grass.script.utils import split

./gui/wxpython/mapdisp/main.py:149:                    if "=" in utils.split(dWhatCmd)[1]:
./gui/wxpython/mapdisp/main.py:150:                        maps = utils.split(dWhatCmd)[1].split("=")[1].split(",")
./gui/wxpython/mapdisp/main.py:152:                        maps = utils.split(dWhatCmd)[1].split(",")
./gui/wxpython/mapdisp/main.py:184:                cmd = utils.split(line.strip())
./gui/wxpython/core/gconsole.py:473:        :param command: command given as a list (produced e.g. by utils.split())
./gui/wxpython/core/gconsole.py:475:        e.g. by utils.split()) and key 'cmdString' with original cmd
./gui/wxpython/gmodeler/dialogs.py:292:            cmd = utils.split(str(line))
./gui/wxpython/gui_core/prompt.py:133:            cmd = utils.split(str(cmdString))
./gui/wxpython/gui_core/prompt.py:135:            cmd = utils.split(EncodeString((cmdString)))
./gui/wxpython/gui_core/prompt.py:393:            splitted = utils.split(str(entry))
./gui/wxpython/gui_core/forms.py:3243:        cmd = utils.split(sys.argv[1])
./gui/wxpython/gui_core/menu.py:106:                cmd = utils.split(str(command))
./gui/wxpython/gui_core/menu.py:108:                cmd = utils.split(EncodeString((command)))

…ith-where-param-on-ms-win-command-prompt

landam · 2023-11-21T08:13:14Z

I am able to reproduce this issue on Windows in wxGUI Python tab:

from grass.script.utils import split
split('d.vect census where="cat < 10"')
['d.vect', 'census', 'where="cat', '<', '10"']
import shlex
shlex.split('d.vect census where="cat < 10"')
['d.vect', 'census', 'where=cat < 10']

… on the win32 platform same as on POSIX-compliant platforms (OSGeo#1908) e.g. run 'd.vect map=census where="cat > 10 AND cat < 20"' with where parameter from Command Prompt CMD/emulator terminal: non-POSIX platform ``` >>> import shlex >>> shlex.split('where="cat > 10 AND cat < 20"', posix=False) ['where="cat', '>', '10', 'AND', 'cat', '<', '20"'] ``` POSIX-compliant platform ``` >>> import shlex >>> shlex.split('where="cat > 10 AND cat < 20"', posix=True) ['where=cat > 10 AND cat < 20'] ``` Under OS MS Windows 'd.vect map=census where="cat > 10 AND cat < 20"' command runned from Command Prompt CMD require use ^ carret symbol for escaping special characters < > ( ) & | , ; ". e.g. 'd.vect map=census where="cat ^> 10 AND cat ^< 20"' --------- Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

tmszi added bug Something isn't working backport_needed windows Microsoft Windows specific Python Related code is in Python labels Oct 1, 2021

tmszi requested a review from hellik October 1, 2021 07:29

neteler added this to the 8.0.1 milestone Dec 9, 2021

tmszi modified the milestones: 8.0.1, 8.2.0 Feb 20, 2022

wenzeslaus reviewed Mar 15, 2022

View reviewed changes

wenzeslaus modified the milestones: 8.2.0, 8.2.1 Apr 26, 2022

neteler modified the milestones: 8.2.1, 8.2.2 Jan 22, 2023

wenzeslaus added backport to 8.3 and removed backport_needed labels Jun 6, 2023

wenzeslaus modified the milestones: 8.2.2, 8.3.1 Jun 6, 2023

tmszi added 4 commits September 27, 2023 08:10

Fix Flake8 formatting error

82d5fa5

Replace ^ carret symbol is unnecessary

3db7568

Fix missing right parenthesis

ef4fa22

tmszi force-pushed the fix-launch-d_vect-with-where-param-on-ms-win-command-prompt branch from 0772c39 to ef4fa22 Compare September 27, 2023 06:19

wenzeslaus reviewed Sep 27, 2023

View reviewed changes

Update split() func doc

6568bd1

petrasovaa reviewed Oct 2, 2023

View reviewed changes

python/grass/script/utils.py Outdated Show resolved Hide resolved

Incorporate suggestion

8aee0cb

Co-authored-by: Anna Petrasova <kratochanna@gmail.com>

landam modified the milestones: 8.3.1, 8.3.2 Oct 25, 2023

Merge remote-tracking branch 'upstream/main' into fix-launch-d_vect-w…

5cb02a9

…ith-where-param-on-ms-win-command-prompt

landam assigned tmszi Nov 21, 2023

landam approved these changes Nov 21, 2023

View reviewed changes

landam modified the milestones: 8.3.2, 8.4.0 Nov 21, 2023

tmszi merged commit 816c25f into OSGeo:main Nov 21, 2023
17 checks passed

tmszi deleted the fix-launch-d_vect-with-where-param-on-ms-win-command-prompt branch November 21, 2023 19:09

tmszi removed the backport to 8.3 label Nov 21, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

grass.script: Split the command string using shell-like syntax on the win32 platform same as on POSIX-compliant platforms #1908

grass.script: Split the command string using shell-like syntax on the win32 platform same as on POSIX-compliant platforms #1908

tmszi commented Oct 1, 2021 •

edited

Loading

hellik commented Oct 1, 2021

hellik commented Oct 1, 2021

tmszi commented Oct 1, 2021 •

edited

Loading

wenzeslaus left a comment

tmszi commented Sep 27, 2023

wenzeslaus left a comment

wenzeslaus commented Sep 27, 2023

tmszi commented Sep 27, 2023

wenzeslaus commented Sep 27, 2023

tmszi commented Sep 30, 2023

petrasovaa commented Oct 3, 2023

landam commented Nov 21, 2023 •

edited

Loading

grass.script: Split the command string using shell-like syntax on the win32 platform same as on POSIX-compliant platforms #1908

grass.script: Split the command string using shell-like syntax on the win32 platform same as on POSIX-compliant platforms #1908

Conversation

tmszi commented Oct 1, 2021 • edited Loading

hellik commented Oct 1, 2021

hellik commented Oct 1, 2021

tmszi commented Oct 1, 2021 • edited Loading

wenzeslaus left a comment

Choose a reason for hiding this comment

tmszi commented Sep 27, 2023

wenzeslaus left a comment

Choose a reason for hiding this comment

wenzeslaus commented Sep 27, 2023

tmszi commented Sep 27, 2023

wenzeslaus commented Sep 27, 2023

tmszi commented Sep 30, 2023

petrasovaa commented Oct 3, 2023

landam commented Nov 21, 2023 • edited Loading

tmszi commented Oct 1, 2021 •

edited

Loading

tmszi commented Oct 1, 2021 •

edited

Loading

landam commented Nov 21, 2023 •

edited

Loading