[GRASS-dev] Windows-style Pathnames

[Huidae Cho]

On Wed, Nov 01, 2006 at 09:25:22PM +0000, Glynn Clements wrote:
> 
> Paul Kelly wrote:
> 
> > While experimenting with native Windows GRASS I was having loads of 
> > problems getting anything to work at all because the GISDBASE path read 
> > from the GISRC file was being corrupted in some strange way. Eventually 
> > tracked it down to lots of MinGW-specific hacks in lib/gis/env.c. When I 
> > removed them all everything (that I've tried so far: text-based startup 
> > and quite a few different modules) worked fine. Really! I've included the 
> > patch that removes them below at the bottom of this e-mail.
> > 
> > It appears to contain lots of stuff about prepending and stripping the 
> > hardcoded path to an Msys installation - what is that all about? Surely:
> > 1) GRASS may be installed other places than under the Msys root
> > 2) The correct solution is to pass the correct Windows-style path to 
> > whichever functions need it, rather than hacking about with it in the GIS 
> > library functions.
> > OK maybe I've missed something but it really doesn't make sense to me and 
> > seems like (from what I can see) to be fixing the problem in the wrong 
> > place?
> 
> Quite possibly.
> 
> > There's lots of other MinGW hacks and ifdefs all over the place that look 
> > very dubious to me. An example is this one in lib/gis/parser.c:
> > 
> >          error = 0 ;
> >          need_first_opt = 1 ;
> >          i = strlen(pgm_name = argv[0]) ;
> >          while (--i >= 0)
> >          {
> > #ifdef __MINGW32__
> >                  if (pgm_name[i] == '/' || pgm_name[i] == '\\')
> > #else
> >                  if (pgm_name[i] == '/')
> > #endif
> >                  {
> >                          pgm_name += i+1;
> >                          break;
> >                  }
> >          }
> > 
> > It looks to me like pgm_name[i] is the first character in an array, so how 
> > does it make sense to compare it to a two-character string?
> 
> '\\' isn't a string, it's a character literal equal to 92 - the ASCII
> code for the backslash character (character literals support sequences
> such as '\n', so the backslash itself has to be preceded by a
> backslash).
> 
> > And if it is 
> > correct, why the #ifdef anyway? Why not do the logical or comparison on 
> > all platforms?
> 
> A backslash isn't a directory separator on Unix, it's a legal (albeit
> unlikely) character in filenames.
> 
> > I think the aim should be to try and find a way of doing 
> > things that works on all platforms, rather than putting in 
> > platform-specific bits like this.
> 
> For the above case, there should probably be a
> G_is_directory_separator() function which determines whether or not
> its argument is a directory separator on the host platform.
> 
> There also needs to be a macro (e.g. DIRSEP) which expands to the
> preferred separator. While Windows accepts both / and \ in a lot of
> situations, there are some (notably command lines) where only \ works.
> 
> > It is getting really annoying trying to 
> > debug things when you're never quite sure what seems to be happening is 
> > really happening, because there's a MinGW-specific hack in somewhere. 
> > Especially when (OK I'm generalising here but only because it's getting 
> > rather annoying!) that so many of the hacks seem to be rather bogus.
> > 
> > Any opinions/justifications/flames?
> 
> Windows isn't Unix. There will inevitably need to be some handling of
> platform-specific behaviour.
> 
> However, it's far better to create a set of functions to handle such
> issues than to deal with individual files separately.
> 
> > Index: env.c
> 
> > -#ifdef __MINGW32__
> > -           /* We need to prepend the MSYS base path (C:/msys/1.0) to any
> > -            * internal path variables since these variables cannot be
> > -            * auto-transformed to win32 path by the MSYS shell. */
> > -           if (strcmp(name, "GISDBASE") == 0 && value[1] != ':')
> > -           {
> > -               char *p;
> > -
> > -               sprintf(tmp, "%s", getenv("WD"));
> > -               /* the default $WD = C:\msys\1.0\\bin\ */
> > -               for(p=tmp+strlen(tmp); --p>=tmp && *p=='\\';);
> > -               /* C:\msys\1.0\\bin */
> > -               for(; --p>=tmp && *p!='\\';);
> > -               /* C:\msys\1.0\\ */
> > -               for(; --p>=tmp && *p=='\\';);
> > -               /* C:\msys\1.0 */
> > -               *(p+1) = 0;
> > -               for(p=tmp; *p; p++)
> > -                   if(*p == '\\')
> > -                       *p = '/';
> > -               /* now tmp is "C:/msys/1.0" and value is "/posix/path" */
> > -               strcat(tmp, value);
> > -               value = tmp;
> > -           }
> > -#endif
> 
> This is bogus, and should go.
> 
> >              if (*name && *value)
> >                  set_env (name, value, loc);
> >          }
> > @@ -271,29 +243,7 @@
> >          for (n = 0; n < count; n++)
> >              if (env[n].name && env[n].value && env[n].loc == loc
> >              && (sscanf (env[n].value,"%1s", dummy) == 1))
> > -#ifdef __MINGW32__
> > -           /* Strip out the MSYS base path (C:/msys/1.0). */
> > -           {
> > -               char *value = env[n].value;
> > -               char tmp[200];
> > -
> > -               if (strcmp(env[n].name, "GISDBASE") == 0 && value[1] == 
> > ':')
> > -               {
> > -                   char *p;
> > -
> > -                   sprintf(tmp, "%s", getenv("WD"));
> > -                   for(p=tmp+strlen(tmp); --p>=tmp && *p=='\\';);
> > -                   for(; --p>=tmp && *p!='\\';);
> > -                   for(; --p>=tmp && *p=='\\';);
> > -                   *(p+1) = 0;
> > -                   /* skip C:/msys/1.0 */
> > -                   value += p+1-tmp;
> > -               }
> > -               fprintf(fd,"%s: %s\n", env[n].name, value);
> > -           }
> > -#else
> 
> Ditto.
> 
> > -#ifdef __MINGW32__
> > -       sprintf ( buf, "%s\\%s\\VAR", G_location_path(), G_mapset() );
> > -#else
> >          sprintf ( buf, "%s/%s/VAR", G_location_path(), G_mapset() );
> > -#endif
> 
> This is arguably correct, although possbily better formulated as e.g.:
> 
> 	sprintf ( buf, "%s" DIRSEP "%s" DIRSEP "VAR", G_location_path(), G_mapset() );
> or:
> 	sprintf ( buf, "%s%s%s%sVAR", G_location_path(), DIRSEP, G_mapset(), DIRSEP );
> 
> rather than using a platform test.
> 
> However, using / should work as an argument to fopen(). The issue is
> more significant if paths are constructed and stored for later use
> (e.g. in $GISRC). In that situation, we need to decide whether to:
> 
> 1. create and store the pathname in the host's syntax, or
> 2. store it in Unix syntax and convert it as required.

The above code actually does #2 under the wrong assumption that GISDBASE
is in an MSys installation directory.

> 
> Either option will involve a lot of changes. #1 is easier to
> implement, but will probably need to be done in more places (it needs
> to be done for every pathname which might need to be in host syntax at
> some point). #2 is harder, but only needs to be performed in places
> where host syntax is known to be necessary.

I found #2 a better choice becase MSys shell scripts will be happier
with posix path.

Huidae

> 
> -- 
> Glynn Clements <glynn at gclements.plus.com>
> 
> _______________________________________________
> grass-dev mailing list
> grass-dev at grass.itc.it
> http://grass.itc.it/mailman/listinfo/grass-dev