raster data needs binary mode on windows

[jef]

I compiled the GRASS plugin for GDAL for OSGeo4W and tried to use it from QGIS (MinGW build GRASS used from MSVC built QGIS, GDAL plugin and GDAL).

GRASS fails to recognize for instance slope from spearfish60 as valid raster. This seems to be because the fcell file is not opened in binary mode. The attached patch changes that.

[jef]

patch to fix #469

[pkelly]

This seems like some sort of compile problem. There is an object file fmode.o that is supposed to be linked into everything compiled on Windows to force files to be opened in binary mode. Perhaps that is not getting included with the OSGeo4W compile?

[jef]

Replying to pkelly:

This seems like some sort of compile problem. There is an object file fmode.o that is supposed to be linked into everything compiled on Windows to force files to be opened in binary mode. Perhaps that is not getting included with the OSGeo4W compile?

Ah, ok. I just linked against the necessary DLLs, but not fmode.o. I'll give that a try. Thanks

[warmerdam]

Dear grass team. I would humbly submit that making anyone who wants to use the grass libraries also link with fmode.o is risky and that it would be safer to actually enforce binary behavior at the open() call(s). In other packages my practice has been to always pass O_BINARY to open(), and to predefine it to zero if it isn't already defined.

[jef]

Replying to jef:

Ah, ok. I just linked against the necessary DLLs, but not fmode.o. I'll give that a try. Thanks

Doesn't do the trick for me. I'm probably not using the same runtime library the GRASS DLLs use and therefore cannot reach the right _fmode.

But I think the better approch would be to have the DLLs open in the right mode anyway instead of requiring to mess with the defaults, which might break other things.

[glynn]

Replying to warmerdam:

Dear grass team. I would humbly submit that making anyone who wants to use the grass libraries also link with fmode.o is risky and that it would be safer to actually enforce binary behavior at the open() call(s). In other packages my practice has been to always pass O_BINARY to open(), and to predefine it to zero if it isn't already defined.

Note: setting _fmode is the only way to get stdin/stdout into binary mode (we don't care about stderr). At least, this used to be the case. It appears that later versions of MSVCRT require that you explicitly change the mode of these descriptors. However, MinGW always links against msvcrt.dll, not a later version.

BTW, I count 52 occurrences of open() or open64() in GRASS, including 6 in libgis and 6 in other libraries.

I don't know how this affects other functions which create descriptors, e.g. pipe(), dup2(), etc. Putting "_fmode = O_BINARY" into gisinit() may suffice for everything except stdin/stdout; that would be preferable to requiring each case to be handled explicitly.

[jef]

Replying to glynn:

Note: setting _fmode is the only way to get stdin/stdout into binary mode (we don't care about stderr). At least, this used to be the case. It appears that later versions of MSVCRT require that you explicitly change the mode of these descriptors. However, MinGW always links against msvcrt.dll, not a later version.

Ok, I adapted fmode.c to dllmain.c and include it in every dll. _fmode is set when the DLL is loaded. That also helps - and might fix more than the raster problem addressed by the original patch - which I replaced.

[neteler]

@grass-dev: is the new patch "patch_for_469.2.diff" ok?

Markus

[glynn]

Replying to neteler:

@grass-dev: is the new patch "patch_for_469.2.diff" ok?

Rather than using "dllmain.dat" to prevent compilation on MinGW, you can call it dllmain.c and exclude it from compilation with:

MOD_OBJS := $(filter-out dllmain.o,$(AUTO_OBJS))

Hmm; we should do this for fmode.o as well.

[neteler]

I would like to close this but fail to write a functional Makefile with the suggested changes. Hints welcome.

Markus

[jef]

Replying to neteler:

I would like to close this but fail to write a functional Makefile with the suggested changes. Hints welcome.

I think the updated patch includes the change glynn mentioned.

[jef]

Replying to jef:

I think the updated patch includes the change glynn mentioned.

Untested I should add.

[neteler]

The r.li part I have submitted with correction (it needs to come after the

include $(MODULE_TOPDIR)/include/Make/Platform.make

line).

The lib/gis/ patch still fails to compile on my Linux box:

gcc -I/home/neteler/grass64/dist.x86_64-unknown-linux-gnu/include  -g -Wall  -fno-common -mtune=nocona -m64 -minline-all-stringops    -fPIC   -DPACKAGE=\""grasslibs"\" -D_FILE_OFFSET_BITS=64 -DGDAL_LINK=1 -DGDAL_DYNAMIC=1   -DPACKAGE=\""grasslibs"\"  
-I/usr/local/include -I/usr/local/include -I/home/neteler/grass64/dist.x86_64-unknown-linux-gnu/include -o OBJ.x86_64-unknown-linux-gnu/dllmain.o -c dllmain.c
dllmain.c:1:21: error: windows.h: No such file or directory
dllmain.c:5: error: expected '=', ',', ';', 'asm' or '__attribute__' before 'WINAPI'
make: *** [OBJ.x86_64-unknown-linux-gnu/dllmain.o] Error 1

as it doesn't exclude dllmain.c from compilation.

I also wonder (unrelated) if the last lines in  http://trac.osgeo.org/grass/browser/grass/branches/develbranch_6/lib/gis/Makefile are GRASS 5 junk.

[glynn]

Replying to neteler:

The lib/gis/ patch still fails to compile on my Linux box:

dllmain.c:1:21: error: windows.h: No such file or directory

as it doesn't exclude dllmain.c from compilation.

I think that you need to modify ARCH_LIB_OBJS (which has the $(OBJDIR)/ prefix added) on 6.x. That uses eager assignment (:=), so its value is set based upon the value of $(LIB_OBJS) at the point that Lib.make is included; subsequent changes to LIB_OBJS won't have any effect.

7.0 added an extra lazy assignment step to make it easier to override the list of object files.

I also wonder (unrelated) if the last lines in  http://trac.osgeo.org/grass/browser/grass/branches/develbranch_6/lib/gis/Makefile are GRASS 5 junk.

They aren't necessary. Rules.make treats all header files in the current directory as pre-requisites for each object file (see LOCAL_HEADERS).

[glynn]

Replying to glynn:

I think that you need to modify ARCH_LIB_OBJS (which has the $(OBJDIR)/ prefix added) on 6.x. That uses eager assignment (:=), so its value is set based upon the value of $(LIB_OBJS) at the point that Lib.make is included; subsequent changes to LIB_OBJS won't have any effect.

Scratch that; it won't work in 6.x. The only way to override the list of object files is to fully define LIB_OBJS before Lib.make is included, e.g.:

LIB_OBJS := $(subst .c,.o,$(wildcard *.c))
LIB_OBJS := $(filter-out fmode.o dllmain.o,$(LIB_OBJS))

7.0 added an extra lazy assignment step to make it easier to override the list of object files.

It also separated out the variable definitions from the rules, so that you can override variables after including Vars.make but before including e.g. Lib.make.

6.x doesn't have this; you can't get at the variables before the rules which use them are defined. So, you can't modify any variables which are used in dependency lines, as those are expanded as they are read. You can override variables which are used in commands, as those aren't expanded until the commands are executed.

[neteler]

Implemented in GRASS 6.4.svn (r35767, 35769) and 6.4.0svn (r35768, 35770).

I leave 7 untouched since the Makefile system is different (hence don't close the bug yet).