Opened 14 years ago

Closed 9 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 14 years ago.
patch to fix #469
patch_for_469.2.diff (3.3 KB ) - added by jef 14 years ago.

Download all attachments as: .zip

Change History (25)

by jef, 14 years ago

Attachment: patch_for_469.diff added

patch to fix #469

comment:1 by pkelly, 14 years ago

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?

in reply to:  1 ; comment:2 by jef, 14 years ago

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 by warmerdam, 14 years ago

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.

in reply to:  2 comment:4 by jef, 14 years ago

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.

in reply to:  3 ; comment:5 by glynn, 14 years ago

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.

in reply to:  5 comment:6 by jef, 14 years ago

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 by neteler, 14 years ago

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

Markus

in reply to:  7 comment:8 by glynn, 14 years ago

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 by neteler, 14 years ago

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

Markus

by jef, 14 years ago

Attachment: patch_for_469.2.diff added

in reply to:  9 ; comment:10 by jef, 14 years ago

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.

in reply to:  10 comment:11 by jef, 14 years ago

Replying to jef:

I think the updated patch includes the change glynn mentioned.

Untested I should add.

comment:12 by neteler, 14 years ago

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.

in reply to:  12 ; comment:13 by glynn, 14 years ago

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).

in reply to:  13 comment:14 by glynn, 14 years ago

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 by neteler, 14 years ago

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 by neteler, 14 years ago

Milestone: 6.4.07.0.0
Version: 6.4.0 RCssvn-trunk

comment:17 by hellik, 11 years ago

Keywords: wingrass added

comment:18 by neteler, 9 years ago

See also ticket #715

comment:19 by neteler, 9 years ago

Still relevant for GRASS 7?

in reply to:  19 ; comment:20 by mlennert, 9 years ago

Replying to neteler:

Still relevant for GRASS 7?

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

in reply to:  20 comment:21 by mlennert, 9 years ago

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

in reply to:  19 comment:22 by glynn, 9 years ago

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 by wenzeslaus, 9 years ago

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.