r.watershed fails on wingrass

[hamish]

Hi,

GRASS 6.4.0svn (spearfish60) > r.watershed elevation=elevation.1
0m@PERMANENT threshold=10000 basin=elev10.basin s
tream=elev10.stream
D1/1: Mode: All in RAM
D1/1: Running: "c:/GRASS-6-SVN/etc/r.watershed.ram" el="elevation.10m@PERMANENT" t=10000 b
a="elev10.basin" se="elev10.stream"

'c:/GRASS-6-SVN/etc/r.watershed.ram" el="elevation.10m@PERMANENT" t=10000 ba="elev10.basin
" se="elev10.stream' is not recognized as an internal or external command,
operable program or batch file.

WARNING: Subprocess failed with exit code 1
WARNING: category information for [elev10.basin] in [user1] missing or
         invalid
WARNING: category information for [elev10.stream] in [user1] missing or
         invalid

r.watershed launches a .seg or .ram version of itself from $GISBASE/etc/. It builds up a char string with options then runs the string via system().

note in the above error messsage the " from the 1st arg and " from the end of the last arg have been stripped off. It seems that the quoting is greedy and the entire string is being treated as a the executable name?

Hamish

ps- will the .ram and .seg modules fail in GRASS 7 where uppercase option names are disallowed? (eg LS= and S=)

pps- should G_convert_dirseps_to_host() be done in the G_gisbase() and G_gisdbase() functions instead or making the modules do it?

[mmetz]

Replying to hamish:

D1/1: Running: "c:/GRASS-6-SVN/etc/r.watershed.ram" el="elevation.10m@PERMANENT" t=10000 b
a="elev10.basin" se="elev10.stream"

Can you try this patch:

--- main.c	(revision 39464)
+++ main.c	(working copy)
@@ -233,12 +233,12 @@
     }
 
     /* Build command line */
-    sprintf(command, "\"%s/etc/", G_gisbase());
+    sprintf(command, "\'%s/etc/", G_gisbase());
 
     if (flag_seg->answer)
- 	strcat(command, "r.watershed.seg\"");
+ 	strcat(command, "r.watershed.seg\'");
     else
-	strcat(command, "r.watershed.ram\"");
+	strcat(command, "r.watershed.ram\'");

The idea is to replace the double quotes around the path to the executable with single quotes in the hope that the windows version of system() is now no longer able to strip the leading and trailing quotes of the whole command because these are now different.

ps- will the .ram and .seg modules fail in GRASS 7 where uppercase option names are disallowed? (eg LS= and S=)

They still work in GRASS 7, probably because they don't call G_parser(argc, argv)? Not sure if this is a good idea not to call G_parser(argc, argv).

pps- should G_convert_dirseps_to_host() be done in the G_gisbase() and G_gisdbase() functions instead or making the modules do it?

In this case, the module should call G_convert_dirseps_to_host() because the module adds "/etc/" to G_gisbase().

Markus M

PS: Any reason why r39150 didn't make it into trunk?

[glynn]

Replying to mmetz:

ps- will the .ram and .seg modules fail in GRASS 7 where uppercase option names are disallowed? (eg LS= and S=)

No, because:

They still work in GRASS 7, probably because they don't call G_parser(argc, argv)?

Right.

Not sure if this is a good idea not to call G_parser(argc, argv).

I'm not sure if it really matters, given that these modules aren't supposed to be run directly by the user. In terms of style and structure, there are a couple of hundred more serious issues before worrying about G_parser().

pps- should G_convert_dirseps_to_host() be done in the G_gisbase() and G_gisdbase() functions instead or making the modules do it?

Eventually. But first you have to fix everything which expects pathnames to use "/".

The existing "quick fix" approach is to use "/" internally and convert to "\" at the last moment.

[hamish]

Replying to mmetz:

Can you try this patch:

not until someone else releases another wingrass build; I'm not set up to do that.

The idea is to replace the double quotes around the path to the executable with single quotes in the hope that the windows version of system() is now no longer able to strip the leading and trailing quotes of the whole command because these are now different.

AFAIK MS-Win does not like single quotes. We already had to go through everything that used system() and switch to double quotes.

I guess we have to live without quoting GISBASE for now, or fix it properly by constructing a args list and then using G_spawn() or similar instead of system("g.module opt1=\"\" opt2=\"\" ... ").

PS: Any reason why r39150 didn't make it into trunk?

The surviving && relevant bits have now made it into trunk.

Hamish

[hamish]

Hamish:

pps- should G_convert_dirseps_to_host() be done in the G_gisbase() and G_gisdbase() functions instead or making the modules do it?

Replying to glynn:

Eventually. But first you have to fix everything which expects pathnames to use "/".

... but shouldn't everything to the left of the G_gisd?base() string be outside the realm of the libs and modules' expectations? All the /etc/ action happens to things tacked onto the right of it.

Hamish

[hellik]

Replying to hamish: [...]

not until someone else releases another wingrass build; I'm not set up to do that.

[...]

at the moment I've managed to understand the instructions (​http://svn.osgeo.org/grass/grass/branches/releasebranch_6_4/mswindows/README.html) to build a nsis-GRASS-Windows-selfinstaller. maybe after some tests, a suitable installer will be possible. but i have no idea about svn-diffs etc at windows.

best regards helli

[hamish]

Replying to hellik:

at the moment I've managed to understand the instructions (​http://svn.osgeo.org/grass/grass/branches/releasebranch_6_4 /mswindows/README.html) to build a nsis-GRASS-Windows- selfinstaller. maybe after some tests, a suitable installer will be possible. but i have no idea about svn-diffs etc at windows.

Colin has been really good about making releases, I'm not worried about that. The issue for me is that I'm borrowing a coworker's desk/PC to test GRASS on in free moments and can't use that for quick recompile+tests or mess with it too much.

Hamish

[hamish]

quick & dirty workaround idea: add a space to the end of the system string so it doesn't end with a \".

[mmetz]

I have successfully compiled grass-6.4.svn on XP, thanks to the detailed instructions!

I tried double quotes, single quotes, space at the end, G_system() instead of system() and all combinations thereof. Nothing works. Only without any quotes around the $GISBASE/etc/r.watershed.[ram|seg] part does it run, otherwise I got the above described error. The new wxGUI, map display, r.info and v.info work, also r.watershed without the quotes.

I used the latest OSGeo4W installer and grass-6.4.svn_src_snapshot_2009_10_10.tar.gz on XP 32 bit.

Markus M

[hamish]

Thanks. A network outage just ate my long reply; short version is will remove $0 quotes for 6.4.0 and bumping this to 6.4.1 for the full argv + G_spawn() treatment.

Hamish

[glynn]

Replying to mmetz:

I tried double quotes, single quotes, space at the end, G_system() instead of system() and all combinations thereof. Nothing works. Only without any quotes around the $GISBASE/etc/r.watershed.[ram|seg] part does it run, otherwise I got the above described error. The new wxGUI, map display, r.info and v.info work, also r.watershed without the quotes.

Does it work (with the quotes) if you add ".exe" to the end of the command name and/or use G_convert_dirseps_to_host()?

I suspect that eliminating the quotes will mean that it fails if GRASS is installed in a directory which contains spaces, and without Administrator privileges you may be unable to install GRASS in a directory which doesn't contain spaces.

[mmetz]

Replying to glynn:

Does it work (with the quotes) if you add ".exe" to the end of the command name and/or use G_convert_dirseps_to_host()?

Nope. As long as there is any quote at the very beginning of the command string, system and G_system() remove that quote, search for the last quote, remove everything after the last quote (I tried putting flags at the end, they don't get quotes), and add their own quotes to the beginning and end (locale-specific, double quotes on my German XP version, single quotes on English XP version).

In order to avoid both quotes and spaces, the only alternative I could find was adding the path to r.watershed.[ram|seg] to PATH or r.watershed.[ram|seg] to something that's in PATH. But r.watershed.[ram|seg] is currently not in PATH in order to hide them from users IIRC.

Another possibility could be to rewrite r.watershed and put r.watershed.[ram|seg] into a local library like e.g. r.li.daemon?

In short, I could not find a way to make it work with quotes on windows (BTW, neither osgeo4w nor mingw-w64 with XP 64bit).

I suspect that eliminating the quotes will mean that it fails if GRASS is installed in a directory which contains spaces, and without Administrator privileges you may be unable to install GRASS in a directory which doesn't contain spaces.

[hamish]

Replying to glynn:

I suspect that eliminating the quotes will mean that it fails if GRASS is installed in a directory which contains spaces,

right.

and without Administrator privileges you may be unable to install GRASS in a directory which doesn't contain spaces.

Can you install to your Windows home dir or desktop anyway?

note that MSys devs do not seem interested in accepting patches which allow it to work in directories containing spaces, so it may be a long wait until this is more than an academic issue. ​http://sourceforge.net/search/?group_artifact_id=102435&type_of_search=artifact&group_id=2435&words=spaces

Replying to mmetz:

(I tried putting flags at the end, they don't get quotes), and add their own quotes to the beginning and end (locale-specific, double quotes on my German XP version, single quotes on English XP version).

IIguessC those added german " and english ' quotes are just for the formatting of the error message??

In order to avoid both quotes and spaces, the only alternative I could find was adding the path to r.watershed.[ram|seg] to PATH or r.watershed.[ram|seg] to something that's in PATH. But r.watershed.[ram|seg] is currently not in PATH in order to hide them from users IIRC.

Another possibility could be to rewrite r.watershed and put r.watershed.[ram|seg] into a local library like e.g. r.li.daemon?

In short, I could not find a way to make it work with quotes on windows (BTW, neither osgeo4w nor mingw-w64 with XP 64bit).

the easiest and least invasive solution seems to me to just replace system("") with G_spawn($0, list).

Long-term rationalizing of the code is a valid but different matter. Right now I am concerned with getting a working 6.4.0 out the door with least-risk. To me the answer to that is to ship it unquoted for 6.4.0 with a note in the errata page; use G_spawn() in 6.4.1; and refactor the module in trunk as you see fit.

Hamish

[glynn]

Replying to hamish:

and without Administrator privileges you may be unable to install GRASS in a directory which doesn't contain spaces.

Can you install to your Windows home dir or desktop anyway?

You ought to be able to create files in that directory, although the administrator could have added an ACL which prohibits execution (although I'm not all that sure what "execute" permission really means on Windows).

If you're a member of the "Power Users" group, you can also create files in the "Program Files" (etc) directory.

But both of those have spaces in the name; Microsoft may have even done this intentionally to force developers to handle spaces in pathnames. The only standard directory which doesn't have spaces in the name is the "Windows" directory, and we shouldn't be putting anything there even if we could.

note that MSys devs do not seem interested in accepting patches which allow it to work in directories containing spaces, so it may be a long wait until this is more than an academic issue.

Where does MSys' come into this?

We're talking about one binary executable invoking another via system() or G_system() (which uses cmd.exe on Windows). MSys doesn't have any effect upon this even if it's installed.

In short, I could not find a way to make it work with quotes on windows (BTW, neither osgeo4w nor mingw-w64 with XP 64bit).

the easiest and least invasive solution seems to me to just replace system("") with G_spawn($0, list).

I can't guarantee that G_spawn() works correctly on Windows; I suspect that it still needs more testing (with usable feedback) than it has received so far.

I note that Microsoft's documentation for the _spawn* functions says that the caller is supposed to quote arguments (which isn't happening at present), but I don't think that applies to the executable name.

In any case, G_spawn() only offers a _spawnl() interface, while r.watershed would need a _spawnv() interface, as the argument list can vary at run-time. Using G_vspawn_ex() might be a solution.

In any case, I don't doubt that it is possible to make it work using system(); it just needs someone who cares enough about getting 6.4 working on Windows to spend more time on it.

One of the first Google hits for "windows command syntax" suggests that two "levels" of quotes are required:

CMD /k ""c:\batch files\test.cmd" "Parameter 1 with space" "Parameter2 with space""

Brief testing from the command line confirms this. I suggest trying this with both system() and G_system(); if the two don't match, then G_system() needs to add the outermost quotes itself.

[mmetz]

Replying to glynn:

One of the first Google hits for "windows command syntax" suggests that two "levels" of quotes are required:

CMD /k ""c:\batch files\test.cmd" "Parameter 1 with space" "Parameter2 with space""

Brief testing from the command line confirms this. I suggest trying this with both system() and G_system(); if the two don't match, then G_system() needs to add the outermost quotes itself.

Adding outermost quotes works with both system() and G_system(), but only on Windows. Linux does not like the outermost quotes, but then there were no problems reported under Linux, therefore I have

#ifdef __MINGW32__

#endif

the new quotes in r39541-3.

Worksforme, closing ticket.