Opened 10 years ago

Closed 10 years ago

#637 closed defect (fixed)

Problems with paths in the TCL/TK Windows GUI

Reported by: marmai Owned by: marisn
Priority: critical Milestone: 6.4.0
Component: Tcl/Tk Version: 6.4.0 RCs
Keywords: Tcl Window Gui path, wingrass Cc: grass-dev@…
CPU: Unspecified Platform: MSWindows 2K

Description

Within the tcltk-GUI it is not possible to select a map within the project: When clicking 'Add vector layer', then on the icon in the lower part of the GIS Manager (tcltk-GUI) 'vector map to display'. Within the following pop-up window there are no maps listed. With the 'g.list type=vect' command entered in the GIS.m window, all vector maps are listed as expected. And it is also possible to show the maps by entering the names of the maps in the 'Vector map'- field in the lower part of the GIS Manager. Just selecting with the icon is not possible.

Furthermore another path related problem with the tcltk-GUI: Using the 'Show attribute data' button within the 'GIS Manager' window the following command is sent to be executed:

db.select table=veg_buffer_1_dissolve database=D:\daten\GISDataBase/demolocation/PERMANENT/dbf/ driver=dbf

For this command I get the following error in the output:

BMI-DBF driver error: Cannot create dbf database: D:datenGISDataBase/demolocation/PERMANENT/dbf/ Unable to open database <D:datenGISDataBase/demolocation/PERMANENT/dbf/>

Notice: the \ are gone!

By correcting the command manually (backslash to slash) to: db.select table=veg_buffer_1_dissolve database=D:/daten/GISDataBase/demolocation/PERMANENT/dbf/ driver=dbf everything works fine.

All was tested with the GRASS6.4.0RC3 and GRASS6.4svn-2 (OSGeo4W installer)

Change History (20)

comment:1 in reply to:  description Changed 10 years ago by hamish

Keywords: wingrass added
Priority: normalcritical

Replying to marmai:

Within the tcltk-GUI it is not possible to select a map within the project: When clicking 'Add vector layer', then on the icon in the lower part of the GIS Manager (tcltk-GUI) 'vector map to display'. Within the following pop-up window there are no maps listed. With the 'g.list type=vect' command entered in the GIS.m window, all vector maps are listed as expected. And it is also possible to show the maps by entering the names of the maps in the 'Vector map'- field in the lower part of the GIS Manager. Just selecting with the icon is not possible.

confirmed with the latest wingrass installer (Colin's stand alone version is much newer than the current osgeo4w build)

this is a close relative of bug #593.

Furthermore another path related problem with the tcltk-GUI: Using the 'Show attribute data' button within the 'GIS Manager' window the following command is sent to be executed:

db.select table=veg_buffer_1_dissolve database=D:\daten\GISDataBase/demolocation/PERMANENT/dbf/ driver=dbf

For this command I get the following error in the output:

BMI-DBF driver error: Cannot create dbf database: D:datenGISDataBase/demolocation/PERMANENT/dbf/ Unable to open database <D:datenGISDataBase/demolocation/PERMANENT/dbf/>

Notice: the \ are gone!

By correcting the command manually (backslash to slash) to: db.select table=veg_buffer_1_dissolve database=D:/daten/GISDataBase/demolocation/PERMANENT/dbf/ driver=dbf everything works fine.

this is slightly different, but probably a close relative of error seen in https://trac.osgeo.org/grass/ticket/629#comment:5

All was tested with the GRASS6.4.0RC3 and GRASS6.4svn-2 (OSGeo4W installer)

please could you try with the latest stand-alone installer? a lot has been fixed there. (you can have both installed without conflicts)

Hamish

comment:2 Changed 10 years ago by hamish

probably G_convert_dirseps_to_host() is needed somewhere in the code.

comment:3 in reply to:  2 ; Changed 10 years ago by glynn

Replying to hamish:

probably G_convert_dirseps_to_host() is needed somewhere in the code.

Did you mean G_convert_dirseps_from_host()?

It's only necessary to convert / to \ if the path is being passed to Windows' command interpreter. The rest of the OS understands both / and \.

OTOH, some GRASS functions only recognise /, so any \ characters need to be converted.

comment:4 Changed 10 years ago by marmai

I just tested with the latest stand-alone version (wingrass-6.4.0SVN-r37703-1-setup.exe). Here everything works fine on my W2k system, so probably everything is already fixed. Thanks a lot.

comment:5 in reply to:  3 ; Changed 10 years ago by hamish

Replying to glynn:

Did you mean G_convert_dirseps_from_host()?

umm, yeah.

It's only necessary to convert / to \ if the path is being passed to Windows' command interpreter. The rest of the OS understands both / and \.

Ok, so the code we need to check then is the bit that launches e.g. dbf.exe child from lib/db/dbmi_client/start.c ???

OTOH, some GRASS functions only recognise /, so any \ characters need to be converted.

Hamish

comment:6 in reply to:  5 ; Changed 10 years ago by glynn

Replying to hamish:

It's only necessary to convert / to \ if the path is being passed to Windows' command interpreter. The rest of the OS understands both / and \.

Ok, so the code we need to check then is the bit that launches e.g. dbf.exe child from lib/db/dbmi_client/start.c ???

I don't think so. The backslashes are getting lost. Windows' command interpreter doesn't interpret backslashes, so this looks more like Tcl's doing. It looks like something is (implicitly or explicitly) using "eval" or "subst" on the string, causing backslash-<char> to be replaced with <char>.

comment:7 in reply to:  6 ; Changed 10 years ago by hamish

Replying to hamish:

Ok, so the code we need to check then is the bit that launches e.g. dbf.exe child from lib/db/dbmi_client/start.c ???

Replying to glynn:

I don't think so. The backslashes are getting lost.

(comment:4 of this ticket reports that fixed now)

Windows' command interpreter doesn't interpret backslashes, so this looks more like Tcl's doing. It looks like something is (implicitly or explicitly) using "eval" or "subst" on the string, causing backslash-<char> to be replaced with <char>.

maybe fixed by the new eval [list ] stuff?

anyway I was more thinking about the Bad file descriptor problems of unquoted spaces from https://trac.osgeo.org/grass/ticket/629#comment:5, not the missing '\'s.

Hamish

comment:8 in reply to:  7 Changed 10 years ago by hellik

Replying to hamish:

Replying to hamish:

Ok, so the code we need to check then is the bit that launches e.g. dbf.exe child from lib/db/dbmi_client/start.c ???

Replying to glynn:

I don't think so. The backslashes are getting lost.

(comment:4 of this ticket reports that fixed now)

Windows' command interpreter doesn't interpret backslashes, so this looks more like Tcl's doing. It looks like something is (implicitly or explicitly) using "eval" or "subst" on the string, causing backslash-<char> to be replaced with <char>.

maybe fixed by the new eval [list ] stuff?

anyway I was more thinking about the Bad file descriptor problems of unquoted spaces from https://trac.osgeo.org/grass/ticket/629#comment:5, not the missing '\'s.

Hamish

WinVista? 32 Grass65-svn - self-compiled today in the osgeo4w-tree nc-dataset

i can confirm the problem i'm not able to select a vector/raster map in the tcltk-gui, but it's possible to enter the name for displaying the maps

helli

comment:9 Changed 10 years ago by marisn

Owner: changed from grass-dev@… to marisn
Status: newassigned

If I didn't broke anything, this issue is fixed in 6.5 r38296

Please test. Candidate for 6.4.0.

comment:10 in reply to:  9 Changed 10 years ago by hellik

Replying to marisn:

If I didn't broke anything, this issue is fixed in 6.5 r38296

Please test. Candidate for 6.4.0.

WinVista32 grass65-svn self compiled at rev38296 in osgeo4w-tree nc_spm_08-dataset

in the bug-report, there seems to be two issues

(1) selecting raster/vector within the gui: => is working

(2) showing attribute data

db.select table=boundary_county database=C:/gisdata/grassdata/nc_spm_08/PERMANENT/dbf driver=dbf

at|AREA|PERIMETER|FIPS|NAME|NAME_LOCAS|DOT_DISTRI|DOT_DIVISI|DOT_COUNTY|COUNTY_100|DOT_GROUP_|ACRES|ABBR_5CHAR|ABBR_4CHAR|ABBR_2CHAR|Z_MEAN|Z_MIN|Z_MAX|Z_ZONE|CO_CENSUS|DIV_CONTAC|DIST_CONTA|CO_WIKIPED|Shape_Leng|Shape_Area 1|11920322825.1|521691.436874|9|ASHE|Ashe|3|11|4|5|ASHE,WILK|273888|ASHE|ASHE|AS|3203|2136|5169|5000|http://quickfacts.census.gov/qfd/states/37/37009.html|http://apps01.dot.state. [...]

=> attribute data is shown

so both issues seem to be fixed

Helli

comment:11 in reply to:  9 ; Changed 10 years ago by hamish

Replying to marisn:

If I didn't broke anything, this issue is fixed in 6.5 r38296

Please test. Candidate for 6.4.0.

Hi,

I have reviewed the patch, it looks good. Thanks!

One thing I notice though - gis.m/vector.tcl line ~ 273 (as seen in r38296 diff):

   if {![catch {open "|v.db.connect map=$mapname layer=$layernum -g" r} vdb]} {
...
   set db [file normalize [join [lrange $vdblist 3 end-1]]]

example output of v.db.connect -g:

v.db.connect map=archsites@PERMANENT layer=1 -g fs='|'
1|archsites|cat|/home/hamish/grassdata/spearfish60/PERMANENT/dbf/|dbf

instead of [lrange ... 3 end-1] to catch the spaces in pathnames it might be cleaner to use the new fs='|' option to v.db.connect. Or would you just have to convert any fs= to spaces to please Tcl's list logic, so may as well stick with the status quo?

your thoughts?

grep shows that chart.tcl and thematic.tcl also use v.db.connect -g with the same default space separator method & so will also fail with spaces in the path name.

and source:grass/trunk/gui/wxpython/gui_modules/sqlbuilder.py@#L46 may suffer a similar bug for DB paths containing spaces.

also, crossing over to the other bug ticket for a sec, is there any reason not to "quote" that $path in lib/gtcltk/select.tcl? even if it seems to work regardless, to be safe.

otherwise your fixes seem ready to backport to 6.4 & I'll do that soon.

regards, Hamish

comment:12 in reply to:  11 Changed 10 years ago by glynn

Replying to hamish:

instead of [lrange ... 3 end-1] to catch the spaces in pathnames it might be cleaner to use the new fs='|' option to v.db.connect. Or would you just have to convert any fs= to spaces to please Tcl's list logic, so may as well stick with the status quo?

fs=| should be passed to v.db.connect, to more robustly distinguish field separators from spaces in the filename field.

Tcl lists should be constructed using Tcl's list operators: list, lappend, concat, etc, not by using string operators and textual substitution and hoping that you got the syntax right.

also, crossing over to the other bug ticket for a sec, is there any reason not to "quote" that $path in lib/gtcltk/select.tcl? even if it seems to work regardless, to be safe.

Tcl isn't Bourne shell; variable substitutions aren't re-parsed, and don't need to be quoted. $foo is always one word, regardless of whether it contains spaces. You only need quotes for literal strings which contain spaces.

comment:13 in reply to:  11 ; Changed 10 years ago by marisn

Replying to hamish:

instead of [lrange ... 3 end-1] to catch the spaces in pathnames it might be cleaner to use the new fs='|' option to v.db.connect. Or would you just have to convert any fs= to spaces to please Tcl's list logic, so may as well stick with the status quo?

your thoughts?

It may look cleaner at first, but it still will require some cleaver string to list convertation, as on some (insane) system there may be some path with any fs in path and thus broke plain string parsing by fs parameter. lrange approach isn't beautiful, but it works.

grep shows that chart.tcl and thematic.tcl also use v.db.connect -g with the same default space separator method & so will also fail with spaces in the path name.

My fault - I didn't grep all gis.m source for similar bugs.

otherwise your fixes seem ready to backport to 6.4 & I'll do that soon.

regards, Hamish

Nice to hear that one of 6.4 stoppers seems to be solved :)

comment:14 in reply to:  13 Changed 10 years ago by glynn

Replying to marisn:

your thoughts?

It may look cleaner at first, but it still will require some cleaver string to list convertation, as on some (insane) system there may be some path with any fs in path and thus broke plain string parsing by fs parameter.

This isn't worth worrying about. There are always cases which don't work; you could have a newline in the pathname, which will break the code which splits the file into records before you even get to worrying about splitting the records into fields.

We only need to worry about the ones which might actually happen in reality. Otherwise, we would have to quote/escape the field separator (and also quote/escape the quote/escape characters), then change everything which parses the output accordingly. On Unix, any character except NUL can occur in a pathname.

I suppose that we could add support for fs=nul (analogous to "find -print0" for use with "xargs -0"); that should be parseable in Python, but I wouldn't be too sure about other languages (bash doesn't seem to like it). This would have to be implemented by the module; you can't have embedded NULs in argv[].

comment:15 Changed 10 years ago by hamish

Cc: grass-dev@… added

comment:16 Changed 10 years ago by hamish

sqlbuilder.py in the wxGUI now updated to safely handles spaces in the DB path and spaces in the SQL query.

Hamish

comment:17 Changed 10 years ago by marisn

Should be fixed in 6.4 as of r39149.

comment:18 in reply to:  17 Changed 10 years ago by hamish

Replying to marisn:

Should be fixed in 6.4 as of r39149.

thanks,

comment:19 Changed 10 years ago by hamish

tested & fixed with latest (WinGRASS-6.4.0SVN-r39271-1-Setup.exe) native installer.

still get _spawnl error if map is not in the current mapset (both wx and tcltk), but that's a different matter.

thanks, Hamish

comment:20 Changed 10 years ago by hamish

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.