[GRASS-dev] Re: [Qgis-developer] [patch] MSVC patch (less warnings and GRASS)

[Glynn_Clements1]

Jürgen E. Fischer wrote:

> AFAICT, GRASS uses __STDC__ in a small number of headers to decide
> whether to use ANSI-style prototypes. This is pointless; if you have a
> compiler which doesn't understand them, you aren't going to be able to
> compile GRASS.

> I'll fix both of these issues today.

Thanks.

Unfortunatley the MinGW DLLs didn't seem to work as well as I hoped. I
decided to build them with MSVC - with CMake out of the QGIS sources.
To do that I had to modify the libs a bit.

The patch isn't probably of any use to GRASS itself, but AFAICS it
doesn't do any harm either (not sure about the __BEGIN_DECLS/__END_DECLS
stuff).

What do you think?

I've committed all the "harmless" changes, i.e. removing the
__{BEGIN,END}_DECLS and C99 variable declarations, and changing
strcasecmp() to G_strcasecmp().

I'm not keen on some of the other changes. I'd rather keep portability
issues in specific library functions than sprinkled around.

E.g. the S_ISDIR() stuff suggessts that we need a G_is_directory()
function. Similarly, a G_rmdir() function.

I'm not sure about the rewinddir() issue:

Index: lib/db/dbmi_base/dirent.c
Index: lib/gis/list.c

+#ifndef _MSC_VER
     rewinddir(dp);
+#else
+ closedir(dp);
+ dp = opendir(dirname);
+#endif

AFAICT, Windows itself doesn't have any of the dirent functions;
they're normally provided by some form of compatibility layer. In that
case, it would be preferable to use a library which supports
rewinddir().

Or failing that, just unconditionally re-open the directory rather
than only doing so on Windows; I doubt that efficiency is that great a
priority.

Both re-opening the directory and rewinddir() create a race condition,
i.e. you could get more entries on the second pass. G_list() will
typically segfault if this happens. It would be better to change the
code to dynamically re-allocate the array, eliminating the need to
re-scan the directory.

The chmod() call in lib/db/dbmi_base/login.c should probably just be
skipped on Windows. The purpose is to prevent other users from reading
the password stored in that file, so the Windows version is rather
pointless:

+#ifdef _MSC_VER
+ _chmod( file, S_IREAD | S_IWRITE );
+#else
     chmod ( file, S_IRUSR | S_IWUSR );
+#endif

Creating a proper ACL for that file would be better, but I don't know
how to do that.

Regarding the DBMI RPC mechanism:

Index: lib/db/dbmi_base/xdr.c

-#ifdef __MINGW32__
+#if defined(__MINGW32__) && !defined(_MSC_VER)
#define USE_STDIO 0
#define USE_READN 1
#else

This change was made because the MSVCRT fread() and fwrite() functions
didn't seem to work on pipes. We observed data corruption, which went
away when switching to read() and write(). I have no idea why this
would be any different when compiling with MSVC; have you checked that
using stdio actually works?

This:

Index: lib/gis/G.h

+#ifdef _MSC_VER
+#include <basetsd.h>
+typedef SSIZE_T ssize_t;
+#endif //_MSC_VER
+

doesn't belong in G.h, as that file doesn't reference ssize_t.

AFAICT, it's only used in get_row.c. The MSDN documentation says that
their read() returns "int", so we should probably use that:

  http://msdn2.microsoft.com/en-us/library/wyssk1bs(VS.80).aspx

Index: lib/gis/error.c
Index: lib/gis/tempfile.c

+#ifdef _MSC_VER
+#include <windows.h>
+#define getpid() GetCurrentProcessId()
+#endif

This suggests a need for a G_getpid() function.

Index: lib/gis/parser.c

What about G_parser() is causing problems with MSVC? Obviously, we
can't disable that function in GRASS itself.

Index: lib/gis/spawn.c
Index: lib/gis/system.c

#ifndef __MINGW32__
#include <sys/wait.h>
+#else
+#include <process.h>
#endif

For legibility, the sense should probably be switched, i.e.:

  #ifdef __MINGW32__
  #include <process.h>
  #else
  #include <sys/wait.h>
  #endif

Or the tests should be separated, i.e.:

  #ifndef __MINGW32__
  #include <sys/wait.h>
  #endif
  #ifdef __MINGW32__
  #include <process.h>
  #endif

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

[Glynn_Clements1]

Jürgen E. Fischer wrote:

> E.g. the S_ISDIR() stuff suggessts that we need a G_is_directory()
> function. Similarly, a G_rmdir() function.

lib/db/dbmi_base/isdir.c doesn't include unistd.h, so I still need to
define S_ISDIR there as long as there is no G_is_directory()

The Linux stat(2) manpage implies that <unistd.h> should be included
for stat(), so I'll add that.

> I'm not sure about the rewinddir() issue:
> > Index: lib/db/dbmi_base/dirent.c
> > Index: lib/gis/list.c

dirent.h is a wrapper I found that maps opendir/readdir/closedir to
FindFirstFile/FindNextFile/FindClose. It used to have no rewinddir(), which
I've added now.

The dirent stuff is about the only actual code which is part of MinGW.
It provides a fair amount of Unix compatibility in its headers, but it
relies upon MSVCRT for the actual functions.

> Regarding the DBMI RPC mechanism:
> > Index: lib/db/dbmi_base/xdr.c

I compile lib/db/dbmi_base/xdr.c without __MINGW32__ now. I didn't check if
fread() works, I'm not even sure the QGIS uses that at all. I'll keep you
posted if I run into trouble.

It's fundamental to the DBMI subsystem. The individual DB drivers
(PostgreSQL, MySQL, DBF, SQLite, ODBC) are separate processes. When a
db.* or v.* module initialises DBMI, the driver is spawned as a child
process with its stdin/stdout connected via pipes.

The various DBMI functions call the driver via a home-grown RPC
mechanism. This used to use XDR, but we ran into problems with the
xdrstdio_* not working, apparently due to fread/fwrite not working
with pipes. When the sunrpc library was hacked to use read/write, the
problem went away.

Rather than requiring a hacked sunrpc library, we just eliminated the
use of XDR. As both the client and the driver run on the same system,
there's no need for the RPC protocol to be portable.

> > Index: lib/gis/parser.c
> What about G_parser() is causing problems with MSVC? Obviously, we
> can't disable that function in GRASS itself.

No, that was to avoid additional dependencies - at least I thought so.
But replacing popen/pclose with G_popen and G_pclose is enough to get
the DLL build with G_parser().

Right. Although that should probably be changed, I'm not especially
confident in GRASS' G_popen() implementation, and that code is
relatively important.

[FWIW, GRASS uses G_popen() in two files, lib/gis/error.c and
lib/gis/list.c, versus 27 files for popen(). Although more than half
of those are accounted for by i.ortho.photo (6 files) and (10 files).]

It's unfortunate that all of this has arisen right now, as:

a) We've recently started preparing for a 6.3.0 release, and
b) my Windows XP system is (still) out of commission due to a dead motherboard.

So following is what's left of the "not so keen" part of the patch . The first
part would be replaced by G_is_directory() later and the second looks ok as it
is, I suppose.

Index: lib/db/dbmi_base/isdir.c

Hopefully this should be fixed by including <unistd.h>.

Index: lib/gis/parser.c

I think that I'll change this to G_popen() and see how it goes.

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