[GRASS-dev] testing native winGRASS

[mlennert]

Hello,

I have finally started going through the GDF GRASS 6.0 manual and test all
commands mentioned there in the current CVS winGRASS. A first round of
results is attached. If the telegraphic style is not clear enough, let me
know.

I will post more as I go along.

And let me try once again: I think it might be good to try to increase the
number of pre-alpha testers, and I think there might be some candidates
among the many that post winGRASS questions on the wingrass mailing list.
Aren't we at the point where we can replace the link to Huidae's binaries
with a link to the current version, clearly indicating that this is
pre-alpha and only to be used by people who want to help us ?

Moritz

(attachments)

TestWinGRASS1.txt (1.25 KB)

[Paul_Kelly]

Hello Moritz

On Tue, 13 Mar 2007, Moritz Lennert wrote:

1) Create location with projection values: location created but mapset (other than PERMANENT) invalid

Could you try the patch below for lib/gis/getl.c and see if GRASS can then handle the newly created location? I think the problem is the created location contains Windows line endings in some of the files and other GRASS functions can't handle them. G_getl2() handles Windows line endings whereas G_getl() does not. I have a hunch this may apply in many places.

Index: getl.c

RCS file: /home/grass/grassrepository/grass6/lib/gis/getl.c,v
retrieving revision 2.6
diff -u -r2.6 getl.c
--- getl.c 12 Nov 2006 04:05:02 -0000 2.6
+++ getl.c 13 Mar 2007 22:31:02 -0000
@@ -16,6 +16,8 @@

  int G_getl ( char *buf, int n, FILE *fd)
  {
+ return G_getl2 ( buf, n, fd );
+ /*
      if (!fgets (buf, n, fd))
         return 0;

@@ -24,6 +26,7 @@
      *buf = 0;

      return 1;
+ */
  }

[Maciej_Sieczka2]

Moritz Lennert wrote:

Hello,

I have finally started going through the GDF GRASS 6.0 manual and test all
commands mentioned there in the current CVS winGRASS. A first round of
results is attached.

Moritz

I *guess* the failures you mention, when a module prints many lines to
tcl/tk GUI output, might be related to:

https://intevation.de/rt/webrt?serial_num=4487
https://intevation.de/rt/webrt?serial_num=2937

Glynn offers some explanations in his last post in the
https://intevation.de/rt/webrt?serial_num=2937.

I hope this is some help.

Best,
Maciek

[mlennert]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/03/07 23:36, Maciej Sieczka wrote:

Moritz Lennert wrote:

Hello,

I have finally started going through the GDF GRASS 6.0 manual and test all
commands mentioned there in the current CVS winGRASS. A first round of
results is attached.

Moritz

I *guess* the failures you mention, when a module prints many lines to
tcl/tk GUI output, might be related to:

https://intevation.de/rt/webrt?serial_num=4487
https://intevation.de/rt/webrt?serial_num=2937

Glynn offers some explanations in his last post in the
https://intevation.de/rt/webrt?serial_num=2937.

Yes, this seems related. So, a probably general tcl problem, not a win
problem...

Moritz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF+AWEMiPFf/8qzFYRApcnAJ9n5pQ7U1f0MDWpGplKuK8E5flTmwCeJyic
x+VuM/Q0H6FqnILHKtAcczk=
=/jdA
-----END PGP SIGNATURE-----

[mlennert]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/03/07 23:34, Paul Kelly wrote:

Hello Moritz

On Tue, 13 Mar 2007, Moritz Lennert wrote:

1) Create location with projection values: location created but mapset
(other than PERMANENT) invalid

Could you try the patch below for lib/gis/getl.c and see if GRASS can
then handle the newly created location? I think the problem is the
created location contains Windows line endings in some of the files and
other GRASS functions can't handle them. G_getl2() handles Windows line
endings whereas G_getl() does not. I have a hunch this may apply in many
places.

I will try this tonight, but just to make clear what is going wrong:
creating the location works and there is a valid PERMANENT mapset
created whatever I do, but if (in the console window where you enter the
name of the new location) I give a mapset name other than PERMANENT,
this mapset is then created (together with PERMANENT), but when I try to
enter it, I get an "invalid mapset" error, which is understandable since
the mapset directory is empty except for a dbf directory (IIRC).

Moritz

Index: getl.c

RCS file: /home/grass/grassrepository/grass6/lib/gis/getl.c,v
retrieving revision 2.6
diff -u -r2.6 getl.c
--- getl.c 12 Nov 2006 04:05:02 -0000 2.6
+++ getl.c 13 Mar 2007 22:31:02 -0000
@@ -16,6 +16,8 @@

int G_getl ( char *buf, int n, FILE *fd)
{
+ return G_getl2 ( buf, n, fd );
+ /*
     if (!fgets (buf, n, fd))
        return 0;

@@ -24,6 +26,7 @@
     *buf = 0;

     return 1;
+ */
}

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF+Aa8MiPFf/8qzFYRAl18AKCXOhyMDJMOAOQMdbI2D8T0xI98lQCfbhuD
3mPFzCfqifeyRuJob7BEUWs=
=uGoV
-----END PGP SIGNATURE-----

[Paul_Kelly]

On Wed, 14 Mar 2007, Moritz Lennert wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/03/07 23:34, Paul Kelly wrote:

Hello Moritz

On Tue, 13 Mar 2007, Moritz Lennert wrote:

1) Create location with projection values: location created but mapset
(other than PERMANENT) invalid

Could you try the patch below for lib/gis/getl.c and see if GRASS can
then handle the newly created location? I think the problem is the
created location contains Windows line endings in some of the files and
other GRASS functions can't handle them. G_getl2() handles Windows line
endings whereas G_getl() does not. I have a hunch this may apply in many
places.

I will try this tonight, but just to make clear what is going wrong:
creating the location works and there is a valid PERMANENT mapset
created whatever I do, but if (in the console window where you enter the
name of the new location) I give a mapset name other than PERMANENT,
this mapset is then created (together with PERMANENT), but when I try to
enter it, I get an "invalid mapset" error, which is understandable since
the mapset directory is empty except for a dbf directory (IIRC).

Ah OK. I believe the problem is this in lib/init/mke_mapset.c:

/* give the mapset a default window for the entire location */
         sprintf(buffer,"cat '%s'/PERMANENT/DEFAULT_WIND > '%s'/'%s'/WIND",
                 location, location, mapset) ;
         system(buffer) ;

but at the minute my mind has gone blank as to the portable way to replace that. I general audit of calls to system() would have caught that if we're able to get round to doing it I suppose.

Paul

[mlennert]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14/03/07 15:48, Paul Kelly wrote:

On Wed, 14 Mar 2007, Moritz Lennert wrote:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 13/03/07 23:34, Paul Kelly wrote:

Hello Moritz

On Tue, 13 Mar 2007, Moritz Lennert wrote:

1) Create location with projection values: location created but mapset
(other than PERMANENT) invalid

Could you try the patch below for lib/gis/getl.c and see if GRASS can
then handle the newly created location? I think the problem is the
created location contains Windows line endings in some of the files and
other GRASS functions can't handle them. G_getl2() handles Windows line
endings whereas G_getl() does not. I have a hunch this may apply in many
places.

I will try this tonight, but just to make clear what is going wrong:
creating the location works and there is a valid PERMANENT mapset
created whatever I do, but if (in the console window where you enter the
name of the new location) I give a mapset name other than PERMANENT,
this mapset is then created (together with PERMANENT), but when I try to
enter it, I get an "invalid mapset" error, which is understandable since
the mapset directory is empty except for a dbf directory (IIRC).

Ah OK. I believe the problem is this in lib/init/mke_mapset.c:

/* give the mapset a default window for the entire location */
        sprintf(buffer,"cat '%s'/PERMANENT/DEFAULT_WIND > '%s'/'%s'/WIND",
                location, location, mapset) ;
        system(buffer) ;

Glynn has taught me about the rename() and remove() function, but I
haven't come upon a copy() function, yet...

Probably some combination of fscanf & fwrite...
Or getc & putc : http://www.java2s.com/Code/C/File/Copyafile.htm

If it really doesn't exist, shouldn't we implement G_fcopy() or
something like that. Possibly in lib/gis/copy.c ?

but at the minute my mind has gone blank as to the portable way to
replace that. I general audit of calls to system() would have caught
that if we're able to get round to doing it I suppose.

This might be priority to my testing via the GDF GRASS Tutorial, so I'll
try to do that first.

Should all system calls be replaced by G_system ? Should any system /
G_sytem calls be avoided ? Or should we check which system() calls use
functions which might not exist in Windows ?

Moritz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFF+BVZMiPFf/8qzFYRAq90AJ9Rsy9ML9EfO2bLNNX7LSonQVIPFQCePXT4
AswvK5NfCrCv3kDLRCJp8Lg=
=4voq
-----END PGP SIGNATURE-----

[Glynn_Clements1]

Moritz Lennert wrote:

>>>> 1) Create location with projection values: location created but mapset
>>>> (other than PERMANENT) invalid
>>>
>>> Could you try the patch below for lib/gis/getl.c and see if GRASS can
>>> then handle the newly created location? I think the problem is the
>>> created location contains Windows line endings in some of the files and
>>> other GRASS functions can't handle them. G_getl2() handles Windows line
>>> endings whereas G_getl() does not. I have a hunch this may apply in many
>>> places.
>>
>> I will try this tonight, but just to make clear what is going wrong:
>> creating the location works and there is a valid PERMANENT mapset
>> created whatever I do, but if (in the console window where you enter the
>> name of the new location) I give a mapset name other than PERMANENT,
>> this mapset is then created (together with PERMANENT), but when I try to
>> enter it, I get an "invalid mapset" error, which is understandable since
>> the mapset directory is empty except for a dbf directory (IIRC).
>
> Ah OK. I believe the problem is this in lib/init/mke_mapset.c:
>
> /* give the mapset a default window for the entire location */
> sprintf(buffer,"cat '%s'/PERMANENT/DEFAULT_WIND > '%s'/'%s'/WIND",
> location, location, mapset) ;
> system(buffer) ;

Glynn has taught me about the rename() and remove() function, but I
haven't come upon a copy() function, yet...

There isn't one. There isn't even a universal definition of "copying"
a file (e.g., what happens if the target already exists, does copying
a symlink create a symlink or a copy of the file it points to, etc).

Probably some combination of fscanf & fwrite...
Or getc & putc : http://www.java2s.com/Code/C/File/Copyafile.htm

If it really doesn't exist, shouldn't we implement G_fcopy() or
something like that. Possibly in lib/gis/copy.c ?

That could be a useful addition. But in the case of copying
PERMANENT/DEFAULT_WIND to <mapset>/WIND file, I'd suggest trying
either G_get_default_window() and G_put_window() or G__get_window()
and G__put_window() first.

> but at the minute my mind has gone blank as to the portable way to
> replace that. I general audit of calls to system() would have caught
> that if we're able to get round to doing it I suppose.

This might be priority to my testing via the GDF GRASS Tutorial, so I'll
try to do that first.

Should all system calls be replaced by G_system ? Should any system /
G_sytem calls be avoided ? Or should we check which system() calls use
functions which might not exist in Windows ?

Avoid system() and G_system() equally. G_system() is almost identical
to system(), except that it appears to work around a signal handling
bug in some ancient system() implementations. Ditto for G_popen().

If you need to invoke an external program, G_spawn() or G_spawn_ex()
are preferable (although G_spawn_ex() hasn't been ported to Windows,
and hasn't really been tested). The advantage of the spawn.c functions
is that they don't involve a shell, so you don't have any issues
regarding quoting arguments.

But the only external programs which should be spawned are those which
are part of GRASS or were specified by the user. Anything else is
unlikely to exist on all platforms.

--
Glynn Clements <glynn@gclements.plus.com>

[mlennert]

On Wed, March 14, 2007 18:47, Glynn Clements wrote:

Moritz Lennert wrote:

Paul Kelly wrote:
> but at the minute my mind has gone blank as to the portable way to
> replace that. I general audit of calls to system() would have caught
> that if we're able to get round to doing it I suppose.

This might be priority to my testing via the GDF GRASS Tutorial, so I'll
try to do that first.

Should all system calls be replaced by G_system ? Should any system /
G_sytem calls be avoided ? Or should we check which system() calls use
functions which might not exist in Windows ?

Avoid system() and G_system() equally. G_system() is almost identical
to system(), except that it appears to work around a signal handling
bug in some ancient system() implementations. Ditto for G_popen().

I've done a first "audit" of system() and G_system() calls. The result is
attached.

One line for the call, the next for the line(s) defining the command that
is being called.

I left out most calls to GRASS modules, except for those where I felt that
there might be pathname issue. At this stage, I also left out all the
display/ and the image rectification modules which have been replaced by
georectify.

I don't have enough knowledge to decide what to do about each of them, but
I'm willing to work on this provided someone can guide me.

Moritz

P.S. Maybe we should put this list (or a revised version) in the wiki to
make it more permanent ?

(attachments)

systemcalls.txt (5.76 KB)

[mlennert]

On Fri, March 16, 2007 23:07, Moritz Lennert wrote:

On Wed, March 14, 2007 18:47, Glynn Clements wrote:

Moritz Lennert wrote:

Paul Kelly wrote:
> but at the minute my mind has gone blank as to the portable way to
> replace that. I general audit of calls to system() would have caught
> that if we're able to get round to doing it I suppose.

This might be priority to my testing via the GDF GRASS Tutorial, so
I'll
try to do that first.

Should all system calls be replaced by G_system ? Should any system /
G_sytem calls be avoided ? Or should we check which system() calls use
functions which might not exist in Windows ?

Avoid system() and G_system() equally. G_system() is almost identical
to system(), except that it appears to work around a signal handling
bug in some ancient system() implementations. Ditto for G_popen().

I've done a first "audit" of system() and G_system() calls. The result is
attached.

I forgot: Just in case it is useful here's the command line I used for
this "audit":

grep -nRI "system \?( \?" * | grep -v debian | grep -v dist | grep -v
"\.py" | grep -v "description\.html" | grep -v "gislib\.dox" | grep -v "*"
| sort | uniq > systemcalls.txt
(plus some manual editing)

Moritz

[Glynn_Clements1]

Moritz Lennert wrote:

>> > but at the minute my mind has gone blank as to the portable way to
>> > replace that. I general audit of calls to system() would have caught
>> > that if we're able to get round to doing it I suppose.
>>
>> This might be priority to my testing via the GDF GRASS Tutorial, so I'll
>> try to do that first.
>>
>> Should all system calls be replaced by G_system ? Should any system /
>> G_sytem calls be avoided ? Or should we check which system() calls use
>> functions which might not exist in Windows ?
>
> Avoid system() and G_system() equally. G_system() is almost identical
> to system(), except that it appears to work around a signal handling
> bug in some ancient system() implementations. Ditto for G_popen().

I've done a first "audit" of system() and G_system() calls. The result is
attached.

One line for the call, the next for the line(s) defining the command that
is being called.

I left out most calls to GRASS modules, except for those where I felt that
there might be pathname issue. At this stage, I also left out all the
display/ and the image rectification modules which have been replaced by
georectify.

I don't have enough knowledge to decide what to do about each of them, but
I'm willing to work on this provided someone can guide me.

general/g.mapset/main.c:135: ret = system ( buf ) ;
G_asprintf ( &buf, "%s %s/.gislock %s", lock_prog, mapset_new_path, gis_lock );

  sprintf(path, "%s/.gislock", mapset_new_path);
  G_spawn(lock_prog, lock_prog, path, gis_lock, NULL);

general/g.mapset/main.c:165: system( buf );
G_asprintf ( &buf, "/bin/sh -c \"%s/etc/clean_temp > /dev/null\"", G_gisbase() );

  sprintf(path, "%s/etc/clean_temp", G_gisbase());
  G_spawn(path, "clean_temp", NULL);
or:
  G_spawn_ex(path, "clean_temp", SF_REDIRECT_FILE, 1, O_WRONLY, "/dev/null", NULL);

general/g.mapsets/main_cmd.c:91&126: system (command);
sprintf (command, "ls -C %s/%s 1>&2", G_gisdbase(), G_location());

  G_ls(G_location_path(), stderr);

general/g.mapsets/set_path.c:65: if (system (command) == 0)
strcpy (command, "g.mapsets -p mapset=") & other strcat calls

  sprintf(mapset_arg, "mapset=%s", mapset)
  G_spawn("g.mapsets", "g.mapsets", "-p", mapset_arg, <other args>, NULL);

general/g.setproj/get_stp.c:198&315: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file1));

  G_spawn(pager, pager, G_convert_dirseps_to_host(Tmp_file1), NULL);
or:
  G_spawn_ex(pager, pager, G_convert_dirseps_to_host(Tmp_file1), SF_REDIRECT_DESCRIPTOR, 1, 2, NULL);

lib/g3d/g3dwindowio.c:226: if (access (path, 0) != 0) system (command);
strcpy (path = command, "mkdir ");

This is commented out, but in general use G_mkdir().

lib/gis/get_datum_name.c:83: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2",pager, G_convert_dirseps_to_host(Tmp_file));

lib/gis/get_ell_name.c:60: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file));

lib/gis/get_projname.c:74: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file));

All as for g.setproj above.

lib/gis/gisbase.c:34: system (command);
sprintf (command, "%s/etc/sroff", G_gisbase( ) );

  G_spawn(command, "sroff", NULL);

lib/gis/gishelp.c:55: system(buffer) ;
sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

lib/imagery/ls_groups.c:70&129: G_system(buf);
sprintf (buf, "$GRASS_PAGER %s", tempfile);

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), tempfile, NULL);

lib/init/mke_loc.c:179: system(buf);
sprintf (buf, "echo %s > \"%s/%s/%s/MYNAME\"", myname, gisdbase, location_name, mapset); & G_convert_dirseps_to_host(buf);

  sprintf(path, "%s/%s/%s/MYNAME", gisdbase, location_name, mapset);
  fp = fopen(path, "w");
  fputs(myname, fp);
  fclose(fp);

lib/proj/datum.c:300: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file));

As for other pager uses above.

lib/vask/V_clear.c:63: system("clear");

Commented out.

raster/r.average/main.c:94: if ((stat = system(command)))
sprintf (command, "%s -anC input=%s,%s fs=space > \"%s\"", STATS, basemap->answer, covermap->answer, tempfile1); & #define STATS "r.stats"

  sprintf(input_arg, "input=%s,%s", basemap->answer, covermap->answer);
  G_spawn_ex(STATS, STATS, "-anC", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile, NULL);

raster/r.average/main.c:154: stat = system(command);
sprintf (command, "%s input=%s output=%s < \"%s\"", RECODE, basemap->answer, outputmap->answer, tempfile2); & #define RECODE "r.recode"

  sprintf(input_arg, "input=%s", basemap->answer);
  sprintf(output_arg, "output=%s", outputmap->answer);
  G_spawn_ex(RECODE, RECODE, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

  G_spawn("clear", "clear", NULL);

raster/r.coin/inter.c:87: G_system(command);
sprintf(command,"$GRASS_PAGER %s",dumpname);

As for other pager uses above.

raster/r.coin/inter.c:108: G_system(command);
sprintf(command,"cp %s %s/%s",dumpname,G_home(),outname);

We could really do with a G_copy(), but for now:

  sprintf(path, "%s/%s", G_home(), outname);
  G_spawn("cp", "cp", dumpname, path, NULL);

Except: the Unix version of G_home() is nonsense; the correct way to
determine the user's home directory is getenv("HOME").

raster/r.coin/inter.c:130: G_system(command);
sprintf(command,"lpr %s",dumpname);

  G_spawn("lpr", "lpr", dumpname, NULL);

raster/r.kappa/stats.c:47: if (system(buf)) {
strcpy (buf, "r.stats -cin"); & many strcat() calles

  G_spawn("r.stats", "r.stats", "-cin", <other arguments>, NULL);

lots in r.le/*, but all seem to be calls to GRASS modules, plus a series of G_system("clear") calls

Hopefully r.le won't be around much longer.

raster/r.out.mpeg/main.c:82: if (256 == G_system("ppmtompeg 2> /dev/null"))

  G_spawn("ppmtompeg", "ppmtompeg", NULL);
or:
  G_spawn_ex("ppmtompeg", "ppmtompeg", SF_REDIRECT_FILE, 2, O_WRONLY, "/dev/null", NULL);

raster/r.out.mpeg/main.c:84: else if (256 == G_system("mpeg_encode 2> /dev/null"))

  G_spawn("mpeg_encode", "ppmtompeg", NULL);
or:
  G_spawn_ex("mpeg_encode", "ppmtompeg", SF_REDIRECT_FILE, 2, O_WRONLY, "/dev/null", NULL);

raster/r.out.mpeg/main.c:319: if (0 != G_system(cmd))
sprintf(cmd, "%s %s", encoder, mpfilename);

  G_spawn(encoder, encoder, mpfilename, NULL);

raster/r.out.mpeg/main.c:351: G_system(cmd);
sprintf(cmd, "cd %s; \\ls %s >> %s 2> /dev/null", path, wildarg, tfile);

Ick; leave this alone for now.

raster/r.out.mpeg/main.c:377: G_system(cmd);
sprintf(cmd, "\\rm %s", tfile);

  remove(tfile);

raster/r.out.mpeg/write.c:351&356: G_system(cmd);
sprintf(cmd, "\\rm %s", file);

  remove(file);

raster/r.report/stats.c:48: if(system(buf))
strcpy (buf, "r.stats -acr"); & more calls to strcat

See above.

raster/r.statistics/o_average.c:30: if (stat = system(command))
sprintf (command, "%s -an input='%s,%s' fs=space > %s", STATS, basemap, covermap, tempfile1); & #define STATS "r.stats"

  sprintf(input_arg, "input=%s,%s", basemap, covermap);
  G_spawn_ex(STATS, STATS, "-an", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile1, NULL);

raster/r.statistics/o_average.c:70: stat = system(command);
sprintf (command, "%s input='%s' output='%s' < %s", RECLASS, basemap, outputmap, tempfile2); & #define RECLASS "r.reclass"

  sprintf(input_arg, "input=%s", basemap);
  sprintf(output_arg, "output=%s", outputmap);
  G_spawn_ex(RECLASS, RECLASS, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

raster/r.statistics/o_sum.c:31: if (stat = system(command))
sprintf (command, "%s -cn input='%s,%s' fs=space > %s", STATS, basemap, covermap, tempfile1); & #define STATS "r.stats"

  sprintf(input_arg, "input=%s,%s", basemap, covermap);
  G_spawn_ex(STATS, STATS, "-cn", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile1, NULL);

raster/r.statistics/o_sum.c:72: stat = system(command);
sprintf (command, "%s input='%s' output='%s' < %s", RECLASS, basemap, outputmap, tempfile2); & #define RECLASS "r.reclass"

  sprintf(input_arg, "input=%s", basemap);
  sprintf(output_arg, "output=%s", outputmap);
  G_spawn_ex(RECLASS, RECLASS, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

raster/r.support/front/run.c:19: if ((stat = G_system(buf)))
G_snprintf(buf, sizeof(buf), "%s/etc/support/%s '%s'", G_gisbase(), pgm, rast);

  sprintf(path, "%s/etc/support/%s", G_gisbase(), pgm);
  G_spawn(path, pgm, rast, NULL);

raster/r.support/front/run.c:37: if ((stat = G_system(buf)))
G_snprintf(buf, sizeof(buf), "%s", pgm);

  G_spawn(pgm, pgm, NULL);

[Exactly why this doesn't just use G_system(pgm), I have no idea.]

raster/r.support/modhead/check_un.c:63: G_system(command);
G_snprintf(command, sizeof(command), "$GRASS_PAGER %s", tempfile);

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), tempfile, NULL);

raster/r.topmodel/misc.c:10: if(G_system(cmd)){
where command can be one of a series of GRASS modules

  G_spawn(...)

raster/r.transect/main.c:135: exit (system(command));
sprintf (command, "r.profile %s input='%s' output='-' null='%s' profile=", coord_str, parms.map->answer, parms.null_str->answer); & more strcat() calls

  sprintf(input_arg, "input=%s", parms.map->answer);
  sprintf(null_arg, "null=%s", parms.null_str->answer);
  G_spawn_ex(r.profile, r.profile, coord_str, input_arg, null_arg, <other args>, NULL);

raster/r.watershed/front/main.c:318: ret = system(command);
sprintf (command, "%s/etc/water/", G_gisbase()) ; & (strcat(command,"r.watershed.seg") || strcat(command,"r.watershed.ram") & more strcat() calls

  sprintf(command, "%s/etc/water/%s", G_gisbase(), prog);
  G_spawn(command, prog, NULL);

raster/r.watershed/shed/main.c:36: if (G_system (input.com_line_ram)) {

raster/r.watershed/shed/main.c:40: if (G_system (input.com_line_seg)) {

raster/r.watershed/shed/main.c:47: } else if (G_system (input.com_line_seg)) {

These might actually need to use G_system(); it depends upon where
input.com_line_* come from.

vector/v.transform/creat_trans.c:66: G_system("clear") ;

  G_spawn("clear", "clear", NULL);

visualization/nviz/src/do_zoom.c:169: if (system(cmd) != 0) {
strcpy(cmd, "pnmcat -lr "); & more strcat() calls

visualization/nviz/src/do_zoom.c:184: if (system(cmd2) != 0) {
strcpy(cmd2, "pnmcat -tb "); & more strcat() calls

Ick. I need to either extend G_spawn[_ex] or add a G_vspawn
(G_spawnv?) function to allow for an array of arguments.

visualization/xganim/main.c:512: system(cmd);
sprintf(cmd, "cd %s; \\ls %s >> %s 2> /dev/null", path, wildarg, tfile);_______________________________________________

Same as r.out.mpeg; leave for now.

--
Glynn Clements <glynn@gclements.plus.com>

[Paul_Kelly]

Moritz's audit and Glynn's replacements both look really good; just a few queries/suggestions below.

On Fri, 16 Mar 2007, Glynn Clements wrote:

general/g.setproj/get_stp.c:198&315: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file1));

  G_spawn(pager, pager, G_convert_dirseps_to_host(Tmp_file1), NULL);
or:
  G_spawn_ex(pager, pager, G_convert_dirseps_to_host(Tmp_file1), SF_REDIRECT_DESCRIPTOR, 1, 2, NULL);

I don't think this will work on Windows - there GRASS_PAGER is "more" which is a cmd.exe built-in rather than a separate command - therefore we still need to call G_system() which will run the command using cmd.exe /c on Windows (or /bin/sh -c on Unix).

lib/vask/V_clear.c:63: system("clear");

Commented out.

Note that there is G_clear_screen() if needed.

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

  G_spawn("clear", "clear", NULL);

G_system_clear() should be better as will also work on Windows.

Same comments go for other usages of GRASS_PAGER and clear!

Paul

[Glynn_Clements1]

Paul Kelly wrote:

>> general/g.setproj/get_stp.c:198&315: G_system(buff);
>> sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file1));
>
> G_spawn(pager, pager, G_convert_dirseps_to_host(Tmp_file1), NULL);
> or:
> G_spawn_ex(pager, pager, G_convert_dirseps_to_host(Tmp_file1), SF_REDIRECT_DESCRIPTOR, 1, 2, NULL);

I don't think this will work on Windows - there GRASS_PAGER is "more"
which is a cmd.exe built-in rather than a separate command - therefore we
still need to call G_system() which will run the command using cmd.exe /c
on Windows (or /bin/sh -c on Unix).

In that case, we need to point GRASS_PAGER at a batch file which
invokes "more". We shouldn't be using a shell because $GRASS_PAGER
*might* be a shell built-in.

>> lib/vask/V_clear.c:63: system("clear");
>
> Commented out.

Note that there is G_clear_screen() if needed.

In that case, it should replace all occurrences of system("clear").
And it should be changed to use G_spawn(); no need for a shell (unless
"cls" is a shell built-in on Windows).

--
Glynn Clements <glynn@gclements.plus.com>

[mlennert]

On 17/03/07 04:25, Glynn Clements wrote:

Paul Kelly wrote:

lib/vask/V_clear.c:63: system("clear");

Commented out.

Note that there is G_clear_screen() if needed.

In that case, it should replace all occurrences of system("clear"). And it should be changed to use G_spawn(); no need for a shell (unless
"cls" is a shell built-in on Windows).

I cannot find a cls.exe on my machine. Does that mean it is a shell built-in ?

Moritz

[Glynn_Clements1]

Moritz Lennert wrote:

>>>> lib/vask/V_clear.c:63: system("clear");
>>> Commented out.
>> Note that there is G_clear_screen() if needed.
>
> In that case, it should replace all occurrences of system("clear").
> And it should be changed to use G_spawn(); no need for a shell (unless
> "cls" is a shell built-in on Windows).

I cannot find a cls.exe on my machine. Does that mean it is a shell
built-in ?

It appears so; typing "help cls" in the shell provides documentation,
which implies that it's built in.

--
Glynn Clements <glynn@gclements.plus.com>

[mlennert]

On 17/03/07 00:48, Glynn Clements wrote:

Moritz Lennert wrote:

but at the minute my mind has gone blank as to the portable way to
replace that. I general audit of calls to system() would have caught
that if we're able to get round to doing it I suppose.

This might be priority to my testing via the GDF GRASS Tutorial, so I'll
try to do that first.

Should all system calls be replaced by G_system ? Should any system /
G_sytem calls be avoided ? Or should we check which system() calls use
functions which might not exist in Windows ?

Avoid system() and G_system() equally. G_system() is almost identical
to system(), except that it appears to work around a signal handling
bug in some ancient system() implementations. Ditto for G_popen().

I've done a first "audit" of system() and G_system() calls. The result is
attached.

One line for the call, the next for the line(s) defining the command that
is being called.

I left out most calls to GRASS modules, except for those where I felt that
there might be pathname issue. At this stage, I also left out all the
display/ and the image rectification modules which have been replaced by
georectify.

I don't have enough knowledge to decide what to do about each of them, but
I'm willing to work on this provided someone can guide me.

general/g.mapset/main.c:135: ret = system ( buf ) ;
G_asprintf ( &buf, "%s %s/.gislock %s", lock_prog, mapset_new_path, gis_lock );

  sprintf(path, "%s/.gislock", mapset_new_path);
  G_spawn(lock_prog, lock_prog, path, gis_lock, NULL);

Does the return value of G_spawn have the same meaning than the one of system and can thus be used in the same way (i.e. in this case:

if ( ret != 0 )
         G_fatal_error ( "%s is currently running GRASS in selected
                mapset or lock file cannot be checked.", G_whoami());

general/g.mapset/main.c:165: system( buf );
G_asprintf ( &buf, "/bin/sh -c \"%s/etc/clean_temp > /dev/null\"", G_gisbase() );

  sprintf(path, "%s/etc/clean_temp", G_gisbase());
  G_spawn(path, "clean_temp", NULL);
or:
  G_spawn_ex(path, "clean_temp", SF_REDIRECT_FILE, 1, O_WRONLY, "/dev/null", NULL);

G_spawn_ex is not ready for windows, yet, correct ?
Should I wait until this is done, or just go ahead with implementing the first solution ?
Just for my education: why is there a choice here and not in the preceding case ?

general/g.mapsets/main_cmd.c:91&126: system (command);
sprintf (command, "ls -C %s/%s 1>&2", G_gisdbase(), G_location());

  G_ls(G_location_path(), stderr);

done

general/g.mapsets/set_path.c:65: if (system (command) == 0)
strcpy (command, "g.mapsets -p mapset=") & other strcat calls

  sprintf(mapset_arg, "mapset=%s", mapset)
  G_spawn("g.mapsets", "g.mapsets", "-p", mapset_arg, <other args>, NULL);

As the different strcat calls are embedded in if-then checks, I imagine that we have to keep these checks and construct an array of <other args> ?

lib/gis/gisbase.c:34: system (command);
sprintf (command, "%s/etc/sroff", G_gisbase( ) );

  G_spawn(command, "sroff", NULL);

I just realised that this is actually a comment to explain how to use G_gisbase(). Should this example be replaced by another one, not to incite people to use system() ?

lib/gis/gishelp.c:55: system(buffer) ;
sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

done, but I get:

warning: implicit declaration of function 'G_spawn'

Is this a problem ? If yes, do I need to add an include statement or change something in the make file ?

lib/imagery/ls_groups.c:70&129: G_system(buf);
sprintf (buf, "$GRASS_PAGER %s", tempfile);

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), tempfile, NULL);

done

A few lines after the G_system(buf) call, there is a G_gets(buf) call. What does this do ? Can I erase that ?

I also see the following code in ls_groups.c (line 37ff):

strcpy (buf, "cd ");
G__file_name (buf+strlen(buf), element, "", G_mapset());
strcat (buf, ";ls");
if (!full) strcat (buf, " -C");
if ((ls = popen (buf, "r")))

Isn't this somewhat equivalent to a system() call ?

lib/init/mke_loc.c:179: system(buf);
sprintf (buf, "echo %s > \"%s/%s/%s/MYNAME\"", myname, gisdbase, location_name, mapset); & G_convert_dirseps_to_host(buf);

  sprintf(path, "%s/%s/%s/MYNAME", gisdbase, location_name, mapset);

No need to run G_convert_dirseps_to_host(path) here ?

  fp = fopen(path, "w");
  fputs(myname, fp);
  fclose(fp);

lib/proj/datum.c:300: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file));

As for other pager uses above.

replaced by

G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"),
     G_convert_dirseps_to_host(Tmp_file), NULL);

Again: is G_convert_dirseps_to_host(Tmp_file) necessary ?

raster/r.average/main.c:94: if ((stat = system(command)))
sprintf (command, "%s -anC input=%s,%s fs=space > \"%s\"", STATS, basemap->answer, covermap->answer, tempfile1); & #define STATS "r.stats"

  sprintf(input_arg, "input=%s,%s", basemap->answer, covermap->answer);
  G_spawn_ex(STATS, STATS, "-anC", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile, NULL);

raster/r.average/main.c:154: stat = system(command);
sprintf (command, "%s input=%s output=%s < \"%s\"", RECODE, basemap->answer, outputmap->answer, tempfile2); & #define RECODE "r.recode"

  sprintf(input_arg, "input=%s", basemap->answer);
  sprintf(output_arg, "output=%s", outputmap->answer);
  G_spawn_ex(RECODE, RECODE, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

Should we use G_spawn_ex even though it is not ported to Windows ?

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

  G_spawn("clear", "clear", NULL);

Should be G_clear_screen(). I guess I can just go ahead with this and we can deal with the internals of G_clear_screen() later, or ?

raster/r.coin/inter.c:87: G_system(command);
sprintf(command,"$GRASS_PAGER %s",dumpname);

As for other pager uses above.

done

G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), dumpname, NULL);

raster/r.coin/inter.c:108: G_system(command);
sprintf(command,"cp %s %s/%s",dumpname,G_home(),outname);

We could really do with a G_copy(), but for now:

  sprintf(path, "%s/%s", G_home(), outname);
  G_spawn("cp", "cp", dumpname, path, NULL);

Except: the Unix version of G_home() is nonsense; the correct way to
determine the user's home directory is getenv("HOME").

Actually the code reads:

if(outname[0] != '/'){
                sprintf(command,"cp %s %s/%s",dumpname,G_home(),outname);
                fprintf(stderr, _("'%s' being saved in your home directory"), outname);
             }
             else{
                 sprintf(command,"cp %s %s", dumpname, outname);
                 fprintf(stderr, _("'%s' being saved"), outname);
             }
G_system(command);

Replaced this by:

if(outname[0] != '/'){
                sprintf(path, "%s/%s", G_home(), outname);
                 fprintf(stderr, _("'%s' being saved in your home directory"), outname);
             }
             else{
                 sprintf(path, "%s", outname);
                 fprintf(stderr, _("'%s' being saved"), outname);
             }
G_spawn("cp", "cp", dumpname, path, NULL);

Any rule as to how I should declare path (char path[254] ?), i.e. which length ?

raster/r.coin/inter.c:130: G_system(command);
sprintf(command,"lpr %s",dumpname);

  G_spawn("lpr", "lpr", dumpname, NULL);

done;

Getting tired, the rest is for later...

Moritz

[Paul_Kelly]

Hello Moritz,
A few comments below.

On Mon, 19 Mar 2007, Moritz Lennert wrote:

lib/gis/gishelp.c:55: system(buffer) ;
sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/

  G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

done, but I get:

Note that that now won't work on Windows (while it presumably did before!) because GRASS_PAGER is a shell built-in and we haven't done changed that yet...
Same goes for the other usages of GRASS_PAGER.

I also see the following code in ls_groups.c (line 37ff):

strcpy (buf, "cd ");
G__file_name (buf+strlen(buf), element, "", G_mapset());
strcat (buf, ";ls");
if (!full) strcat (buf, " -C");
if ((ls = popen (buf, "r")))

Isn't this somewhat equivalent to a system() call ?

We can maybe do something with G_ls() or G__ls().

lib/init/mke_loc.c:179: system(buf);
sprintf (buf, "echo %s > \"%s/%s/%s/MYNAME\"", myname, gisdbase, location_name, mapset); & G_convert_dirseps_to_host(buf);

  sprintf(path, "%s/%s/%s/MYNAME", gisdbase, location_name, mapset);

No need to run G_convert_dirseps_to_host(path) here ?

No because the fopen() function on Windows can handle Unix file separator characters OK.

  fp = fopen(path, "w");
  fputs(myname, fp);
  fclose(fp);

lib/proj/datum.c:300: G_system(buff);
sprintf(buff,"%s \"%s\" 1>&2", pager, G_convert_dirseps_to_host(Tmp_file));

As for other pager uses above.

replaced by

G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"),
   G_convert_dirseps_to_host(Tmp_file), NULL);

As well as not working on Windows as mentioned above, this will also be broken on Unix now for some circumstances as it will print the list of datum choices to stdout but print the prompts to stderr. So if you run something like
g.proj -wi epsg=29900 > wkt.prj
to write a WKT description of EPSG co-ordinate system 29900 to a text file, but want g.proj to interactively prompt you on the datum names, the list of datum names will be written into the file rather than printed to the screen with the other prompts.

Again: is G_convert_dirseps_to_host(Tmp_file) necessary ?

Actually yes here - because you are passing a pathname to an external program and it needs to have native system separator characters.

raster/r.average/main.c:94: if ((stat = system(command)))
sprintf (command, "%s -anC input=%s,%s fs=space > \"%s\"", STATS, basemap->answer, covermap->answer, tempfile1); & #define STATS "r.stats"

  sprintf(input_arg, "input=%s,%s", basemap->answer, covermap->answer);
  G_spawn_ex(STATS, STATS, "-anC", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile, NULL);

raster/r.average/main.c:154: stat = system(command);
sprintf (command, "%s input=%s output=%s < \"%s\"", RECODE, basemap->answer, outputmap->answer, tempfile2); & #define RECODE "r.recode"

  sprintf(input_arg, "input=%s", basemap->answer);
  sprintf(output_arg, "output=%s", outputmap->answer);
  G_spawn_ex(RECODE, RECODE, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

Should we use G_spawn_ex even though it is not ported to Windows ?

Well seeing the point of this audit was to fix occurences of system() that were broken on Windows, and that this one (like a lot of them) as far as I can see isn't actually broken, but the change would break it, I should think not! It could do with being tidied up obviously but needs a bit more work.

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

  G_spawn("clear", "clear", NULL);

Should be G_clear_screen(). I guess I can just go ahead with this and we can deal with the internals of G_clear_screen() later, or ?

I think so. This actually is a case of something that currently is broken on Windows and will be fixed by the change.

raster/r.coin/inter.c:87: G_system(command);
sprintf(command,"$GRASS_PAGER %s",dumpname);

As for other pager uses above.

Still won't work on Windows but wouldn't have before anyway so nothing lost. If we get GRASS_PAGER working as a batch file we can get it working without having to revisit this though.

Paul

[Glynn_Clements1]

Moritz Lennert wrote:

>> general/g.mapset/main.c:135: ret = system ( buf ) ;
>> G_asprintf ( &buf, "%s %s/.gislock %s", lock_prog, mapset_new_path, gis_lock );
>
> sprintf(path, "%s/.gislock", mapset_new_path);
> G_spawn(lock_prog, lock_prog, path, gis_lock, NULL);

Does the return value of G_spawn have the same meaning than the one of
system and can thus be used in the same way (i.e. in this case:

if ( ret != 0 )
         G_fatal_error ( "%s is currently running GRASS in selected
                mapset or lock file cannot be checked.", G_whoami());

Yes. system(), G_system() and G_spawn() all return the exit status of
the program.

>> general/g.mapset/main.c:165: system( buf );
>> G_asprintf ( &buf, "/bin/sh -c \"%s/etc/clean_temp > /dev/null\"", G_gisbase() );
>
> sprintf(path, "%s/etc/clean_temp", G_gisbase());
> G_spawn(path, "clean_temp", NULL);
> or:
> G_spawn_ex(path, "clean_temp", SF_REDIRECT_FILE, 1, O_WRONLY, "/dev/null", NULL);

G_spawn_ex is not ready for windows, yet, correct ?

Correct.

Should I wait until this is done, or just go ahead with implementing the
first solution ?

I'm not sure. It would be nice if someone would volunteer to write the
Windows implementation (using CreateProcess()).

Hmm. We might want a function which returns the name of the platform's
null device (i.e. /dev/null on Unix, NUL: on Windows).

Just for my education: why is there a choice here and not in the
preceding case ?

In the preceding case, there's no benefit to using G_spawn_ex(). In
this case, using G_spawn() means that you lose the redirection to
/dev/null.

>> general/g.mapsets/set_path.c:65: if (system (command) == 0)
>> strcpy (command, "g.mapsets -p mapset=") & other strcat calls
>
> sprintf(mapset_arg, "mapset=%s", mapset)
> G_spawn("g.mapsets", "g.mapsets", "-p", mapset_arg, <other args>, NULL);

As the different strcat calls are embedded in if-then checks, I imagine
that we have to keep these checks and construct an array of <other args> ?

Ah. That will need to wait until G_spawn_ex() has been extended (or
has a G_vspawn_ex() version).

>> lib/gis/gisbase.c:34: system (command);
>> sprintf (command, "%s/etc/sroff", G_gisbase( ) );
>
> G_spawn(command, "sroff", NULL);

I just realised that this is actually a comment to explain how to use
G_gisbase(). Should this example be replaced by another one, not to
incite people to use system() ?

Yes please.

>> lib/gis/gishelp.c:55: system(buffer) ;
>> sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
>> sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/
>
> G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

done, but I get:

warning: implicit declaration of function 'G_spawn'

Is this a problem ? If yes, do I need to add an include statement or
change something in the make file ?

Currently:

  #include <grass/spawn.h>

Maybe it could be merged with gis.h/gisdefs.h.

>> lib/imagery/ls_groups.c:70&129: G_system(buf);
>> sprintf (buf, "$GRASS_PAGER %s", tempfile);
>
> G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), tempfile, NULL);

done

A few lines after the G_system(buf) call, there is a G_gets(buf) call.
What does this do ?

Re-using the same buffer for different things.

Can I erase that ?

No.

I also see the following code in ls_groups.c (line 37ff):

strcpy (buf, "cd ");
G__file_name (buf+strlen(buf), element, "", G_mapset());
strcat (buf, ";ls");
if (!full) strcat (buf, " -C");
if ((ls = popen (buf, "r")))

This should probably use G__ls().

Isn't this somewhat equivalent to a system() call ?

popen() is similar to system, in that it spawns a command using a
shell. But popen() returns a FILE* corresponding to either the
program's stdin ("w") or stdout ("r"), and doesn't wait for the
command to complete.

You can achieve the same thing using G_spawn_ex(), pipe() and
fdopen(), e.g.:

  int fds[2];
  FILE *fp;
  pid_t pid;

  pipe(fds);
  pid = G_spawn_ex(..., SF_BACKGROUND, SF_REDIRECT_DESCRIPTOR, 1, fds[1], SF_CLOSE_DESCRIPTOR, fds[0], ...);
  close(fds[1]);
  fp = fdopen(fds[0], "r");

>> lib/init/mke_loc.c:179: system(buf);
>> sprintf (buf, "echo %s > \"%s/%s/%s/MYNAME\"", myname, gisdbase, location_name, mapset); & G_convert_dirseps_to_host(buf);
>
> sprintf(path, "%s/%s/%s/MYNAME", gisdbase, location_name, mapset);

No need to run G_convert_dirseps_to_host(path) here ?

No; you only need that function if you are passing the filename to a
command; Windows system calls can handle / as the separator, but
commands treat it as signifying an option (analogous to - on Unix).

>> raster/r.coin/inter.c:32: G_system("clear");
>>
>> raster/r.coin/inter.c:51: G_system("clear");
>
> G_spawn("clear", "clear", NULL);

Should be G_clear_screen(). I guess I can just go ahead with this and we
can deal with the internals of G_clear_screen() later, or ?

Yes.

Any rule as to how I should declare path (char path[254] ?), i.e. which
length ?

GPATH_MAX. It's currently hard-coded to 4096, which is likely to be
sufficient (that's over 50 80-column lines; no-one is going to have a
$HOME directory anywhere near that long).

--
Glynn Clements <glynn@gclements.plus.com>

[mlennert]

On 19/03/07 01:02, Paul Kelly wrote:

Hello Moritz,
A few comments below.

On Mon, 19 Mar 2007, Moritz Lennert wrote:

lib/gis/gishelp.c:55: system(buffer) ;
sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/

    G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

done, but I get:

Note that that now won't work on Windows (while it presumably did before!) because GRASS_PAGER is a shell built-in and we haven't done changed that yet...
Same goes for the other usages of GRASS_PAGER.

Ok, so, should I revert this, or should we leave it as a motivation to develop a GRASS_PAGER batch script ?

replaced by

G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"),
   G_convert_dirseps_to_host(Tmp_file), NULL);

As well as not working on Windows as mentioned above, this will also be broken on Unix now for some circumstances as it will print the list of datum choices to stdout but print the prompts to stderr. So if you run something like
g.proj -wi epsg=29900 > wkt.prj
to write a WKT description of EPSG co-ordinate system 29900 to a text file, but want g.proj to interactively prompt you on the datum names, the list of datum names will be written into the file rather than printed to the screen with the other prompts.

So, should I revert or is there a better fix ? Unless we decide that any form of prompting is "illegal" in modules ?

raster/r.average/main.c:94: if ((stat = system(command)))
sprintf (command, "%s -anC input=%s,%s fs=space > \"%s\"", STATS, basemap->answer, covermap->answer, tempfile1); & #define STATS "r.stats"

    sprintf(input_arg, "input=%s,%s", basemap->answer, covermap->answer);
    G_spawn_ex(STATS, STATS, "-anC", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile, NULL);

raster/r.average/main.c:154: stat = system(command);
sprintf (command, "%s input=%s output=%s < \"%s\"", RECODE, basemap->answer, outputmap->answer, tempfile2); & #define RECODE "r.recode"

    sprintf(input_arg, "input=%s", basemap->answer);
    sprintf(output_arg, "output=%s", outputmap->answer);
    G_spawn_ex(RECODE, RECODE, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

Should we use G_spawn_ex even though it is not ported to Windows ?

Well seeing the point of this audit was to fix occurences of system() that were broken on Windows, and that this one (like a lot of them) as far as I can see isn't actually broken, but the change would break it, I should think not! It could do with being tidied up obviously but needs a bit more work.

Glynn seems to argue that we should try to avoid any calls to a shell...

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

    G_spawn("clear", "clear", NULL);

Should be G_clear_screen(). I guess I can just go ahead with this and we can deal with the internals of G_clear_screen() later, or ?

I think so. This actually is a case of something that currently is broken on Windows and will be fixed by the change.

At least some good

Moritz

[Paul_Kelly]

On Mon, 19 Mar 2007, Moritz Lennert wrote:

On 19/03/07 01:02, Paul Kelly wrote:

Hello Moritz,
A few comments below.

On Mon, 19 Mar 2007, Moritz Lennert wrote:

lib/gis/gishelp.c:55: system(buffer) ;
sprintf(buffer, "%%GRASS_PAGER%% %s", file) ; /*ifdef __MINGW32__*/
sprintf(buffer, "$GRASS_PAGER %s", file) ;/*else*/

    G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"), file, NULL);

done, but I get:

Note that that now won't work on Windows (while it presumably did before!) because GRASS_PAGER is a shell built-in and we haven't done changed that yet...
Same goes for the other usages of GRASS_PAGER.

Ok, so, should I revert this, or should we leave it as a motivation to develop a GRASS_PAGER batch script ?

Leave it as motivation I think - although that instance too also has prompts printing to stderr and pager output going to stdout so we're just keeping the same bug.

replaced by

G_spawn(getenv("GRASS_PAGER"), getenv("GRASS_PAGER"),
   G_convert_dirseps_to_host(Tmp_file), NULL);

As well as not working on Windows as mentioned above, this will also be broken on Unix now for some circumstances as it will print the list of datum choices to stdout but print the prompts to stderr. So if you run something like
g.proj -wi epsg=29900 > wkt.prj
to write a WKT description of EPSG co-ordinate system 29900 to a text file, but want g.proj to interactively prompt you on the datum names, the list of datum names will be written into the file rather than printed to the screen with the other prompts.

So, should I revert or is there a better fix ? Unless we decide that any form of prompting is "illegal" in modules ?

I thought we decided that terminal interaction was OK if the user specifically requested it, in this case by specifying the -i "interactive" flag. Looks like G_spawn_ex() with appropriate flags would solve the problem though, so it can be a motivation to develop that on Windows.

raster/r.average/main.c:94: if ((stat = system(command)))
sprintf (command, "%s -anC input=%s,%s fs=space > \"%s\"", STATS, basemap->answer, covermap->answer, tempfile1); & #define STATS "r.stats"

    sprintf(input_arg, "input=%s,%s", basemap->answer, covermap->answer);
    G_spawn_ex(STATS, STATS, "-anC", input_arg, "fs=space", SF_REDIRECT_FILE, 1, O_WRONLY|O_CREAT, tempfile, NULL);

raster/r.average/main.c:154: stat = system(command);
sprintf (command, "%s input=%s output=%s < \"%s\"", RECODE, basemap->answer, outputmap->answer, tempfile2); & #define RECODE "r.recode"

    sprintf(input_arg, "input=%s", basemap->answer);
    sprintf(output_arg, "output=%s", outputmap->answer);
    G_spawn_ex(RECODE, RECODE, input_arg, output_arg, SF_REDIRECT_FILE, 0, O_RDONLY, tempfile2, NULL);

Should we use G_spawn_ex even though it is not ported to Windows ?

Well seeing the point of this audit was to fix occurences of system() that were broken on Windows, and that this one (like a lot of them) as far as I can see isn't actually broken, but the change would break it, I should think not! It could do with being tidied up obviously but needs a bit more work.

Glynn seems to argue that we should try to avoid any calls to a shell...

Well I see this point yes but I don't think we should do it to the extent that it breaks things that worked before. But looking at G_spawn_ex() for Windows is probably the best way out here.

raster/r.coin/inter.c:32: G_system("clear");

raster/r.coin/inter.c:51: G_system("clear");

    G_spawn("clear", "clear", NULL);

Should be G_clear_screen(). I guess I can just go ahead with this and we can deal with the internals of G_clear_screen() later, or ?

I think so. This actually is a case of something that currently is broken on Windows and will be fixed by the change.

At least some good

Moritz