Opened 12 years ago

Closed 6 years ago

#469 closed defect (fixed)

raster data needs binary mode on windows

Reported by: jef Owned by: grass-dev@…
Priority: major Milestone: 7.0.0
Component: Default Version: svn-trunk
Keywords: wingrass Cc:
CPU: Unspecified Platform: MSWindows XP

Description

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.

Attachments (2)

patch_for_469.diff (379 bytes) - added by jef 12 years ago.
patch to fix #469
patch_for_469.2.diff (3.3 KB) - added by jef 12 years ago.

Download all attachments as: .zip

Change History (25)

Changed 12 years ago by jef

Attachment: patch_for_469.diff added

patch to fix #469

comment:1 Changed 12 years ago by 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?

comment:2 in reply to:  1 ; Changed 12 years ago by 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

comment:3 Changed 12 years ago by 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.

comment:4 in reply to:  2 Changed 12 years ago by 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.

comment:5 in reply to:  3 ; Changed 12 years ago by 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.

comment:6 in reply to:  5 Changed 12 years ago by 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.

comment:7 Changed 12 years ago by neteler

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

Markus

comment:8 in reply to:  7 Changed 12 years ago by 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.

comment:9 Changed 12 years ago by neteler

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

Markus

Changed 12 years ago by jef

Attachment: patch_for_469.2.diff added

comment:10 in reply to:  9 ; Changed 12 years ago by 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.

comment:11 in reply to:  10 Changed 12 years ago by jef

Replying to jef:

I think the updated patch includes the change glynn mentioned.

Untested I should add.

comment:12 Changed 12 years ago by 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.

comment:13 in reply to:  12 ; Changed 12 years ago by 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).

comment:14 in reply to:  13 Changed 12 years ago by 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.

comment:15 Changed 12 years ago by 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).

comment:16 Changed 12 years ago by neteler

Milestone: 6.4.07.0.0
Version: 6.4.0 RCssvn-trunk

comment:17 Changed 9 years ago by hellik

Keywords: wingrass added

comment:18 Changed 7 years ago by neteler

See also ticket #715

comment:19 Changed 7 years ago by neteler

Still relevant for GRASS 7?

comment:20 in reply to:  19 ; Changed 7 years ago by mlennert

Replying to neteler:

Still relevant for GRASS 7?

Glynn ? MarkusM ? I would think that this is not an issue in grass7 ?

comment:21 in reply to:  20 Changed 7 years ago by mlennert

Replying to mlennert:

Replying to neteler:

Still relevant for GRASS 7?

Glynn ? MarkusM ? I would think that this is not an issue in grass7 ?

ping

comment:22 in reply to:  19 Changed 7 years ago by glynn

Replying to neteler:

Still relevant for GRASS 7?

GRASS 7 sets "_fmode = O_BINARY" in lib/gis/gisinit.c, so fmode.o probably isn't necessary.

comment:23 Changed 6 years ago by wenzeslaus

Resolution: fixed
Status: newclosed

From the ticket is seems that for GRASS 6.4 it is fixed (comment:15) and for GRASS 7 it is OK too (comment:22), so closing the ticket.

Moreover, it think I recently saw GRASS 7 (trunk) working on MS Windows 7 and 8 normally.

Note: See TracTickets for help on using tickets.