[GRASS-dev] Re: testing native winGRASS

[Moritz Lennert]

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