[GRASS-dev] [GRASS GIS] #783: r.watershed fails on wingrass

GRASS GRASS developer
1

#783: r.watershed fails on wingrass
-----------------------------------+----------------------------------------
Reporter: hamish | Owner: grass-dev@lists.osgeo.org
     Type: defect | Status: new
Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Keywords: r.watershed, wingrass | Platform: MSWindows XP
      Cpu: x86-32 |
-----------------------------------+----------------------------------------
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?

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783&gt;
GRASS GIS <http://grass.osgeo.org>

2

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by mmetz):

Replying to [ticket:783 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?

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:1&gt;
GRASS GIS <http://grass.osgeo.org>

3

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by glynn):

Replying to [comment:1 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:2&gt;
GRASS GIS <http://grass.osgeo.org>

4

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by hamish):

Replying to [comment:1 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:3&gt;
GRASS GIS <http://grass.osgeo.org>

5

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by 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 [comment:2 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:4&gt;
GRASS GIS <http://grass.osgeo.org>

6

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by hellik):

Replying to [comment:3 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:5&gt;
GRASS GIS <http://grass.osgeo.org>

7

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by hamish):

Replying to [comment:5 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:6&gt;
GRASS GIS <http://grass.osgeo.org>

8

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by hamish):

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

--
Ticket URL: <http://trac.osgeo.org/grass/ticket/783#comment:7&gt;
GRASS GIS <http://grass.osgeo.org>

9

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:8&gt;
GRASS GIS <http://grass.osgeo.org>

10

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.1
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Changes (by hamish):

  * milestone: 6.4.0 => 6.4.1

Comment:

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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:9&gt;
GRASS GIS <http://grass.osgeo.org>

11

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Changes (by glynn):

  * milestone: 6.4.1 => 6.4.0

Comment:

Replying to [comment:8 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:10&gt;
GRASS GIS <http://grass.osgeo.org>

12

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by mmetz):

Replying to [comment:10 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:11&gt;
GRASS GIS <http://grass.osgeo.org>

13

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by hamish):

Replying to [comment:10 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 [comment:11 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

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:12&gt;
GRASS GIS <http://grass.osgeo.org>

14

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: new
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Comment (by glynn):

Replying to [comment:12 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:13&gt;
GRASS GIS <http://grass.osgeo.org>

15

#783: r.watershed fails on wingrass
---------------------------+------------------------------------------------
  Reporter: hamish | Owner: grass-dev@lists.osgeo.org
      Type: defect | Status: closed
  Priority: major | Milestone: 6.4.0
Component: Raster | Version: 6.4.0 RCs
Resolution: fixed | Keywords: r.watershed, wingrass
  Platform: MSWindows XP | Cpu: x86-32
---------------------------+------------------------------------------------
Changes (by mmetz):

  * status: new => closed
  * resolution: => fixed

Comment:

Replying to [comment:13 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.

--
Ticket URL: <https://trac.osgeo.org/grass/ticket/783#comment:14&gt;
GRASS GIS <http://grass.osgeo.org>