Building SDE support on 64-bit Linux needs work. #4521

Merged
merged 3 commits into from Aug 8, 2013

Projects

None yet

3 participants

@sdlime
Member
sdlime commented Nov 9, 2012

Been trying to get 6.0 w/SDE support working on 64-bit Linux (SuSE). I was able to get it to work after some trial and error... Changes were:

Need to compile with -DSDE64, not sure how best to handle that in configure. Can just add CFLAGS='-DSDE64' but since 64-bit is the norm perhaps this should be detected.

With that flag set, building mapsde.c generates a dozen or so warnings about passing arguments of incompatible pointer types. In all cases it's issues with long ints. Switching to int fixes all of them except for one related to shapeObj index attribute (long). Without those changes I ran into memory allocation errors. What's the proper way to support both 32 and 64-bit in a case like this?

from #4700, this would also need to be applied:

void msSDELayerFreeItemInfo(layerObj *layer)
{
#ifdef USE_SDE
    msSDELayerInfo *sde = NULL;
    int i;
    if (!msSDELayerIsOpen(layer)) {
        msSetError( MS_SDEERR,
                    "SDE layer has not been opened.",
                    "msSDELayerFreeItemInfo()");
    }
    sde = layer->layerinfo;
    if (sde != NULL && sde->basedefs) {
        SE_table_free_descriptions(sde->basedefs);
        sde->basedefs = NULL;
    }
    if (sde != NULL && sde->joindefs) {
        SE_table_free_descriptions(sde->joindefs);
        sde->joindefs = NULL;
    }
    if (layer->iteminfo) {
@sdlime
Member
sdlime commented Nov 10, 2012

Well, we could use SDE's types (sg.h), e.g. SE_INT32 instead of long's... Or should we use the ms_int32 typedef?

@tbonfort , @dmorissette

Steve

@tbonfort
Member

@sdlime does this need to be taken care of for 6.2.1 or can we just concentrate on the cmake part of the fix and target 6.4 ?

@sdlime
Member
sdlime commented Apr 10, 2013

Moving to 6.4... I can live with my local hack for 6.2.x.

Steve

@tbonfort
Member

without access to the SDE sdk, there's not much I can do to help out on this one.

@sdlime
Member
sdlime commented Apr 10, 2013

Maybe I can help on the cmake side. I’m more curious on how to approach the integer types.

From: Thomas Bonfort [mailto:notifications@github.com]
Sent: Wednesday, April 10, 2013 11:49 AM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] Building SDE support on 64-bit Linux needs work. (#4521)

without access to the SDE sdk, there's not much I can do to help out on this one.


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/issues/4521#issuecomment-16186694.

@sdlime sdlime was assigned Jul 22, 2013
tbonfort added some commits Aug 6, 2013
@tbonfort tbonfort refactor SDE detection in cmake files (#4512,#4700)
- add a specific plugin configuration
- checks for sde,sg and pe libs, adds pthread,socketi and dl if found
- adds #define SDE64 on 64 bit platforms
b677cf3
@tbonfort tbonfort add missing file 167ee0d
@tbonfort
Member
tbonfort commented Aug 6, 2013

@sdlime This pull request refactors SDE detection in the cmake files.

for windows plugin builds:
-DSDE_VERSION=91|92
-DWITH_SDE_PLUGIN=1 (requires SDE_INCLUDE_DIR and SDE_LIBRARY_DIR)

for linux, and/or for non plugin windows builds:
-DWITH_SDE=1
-DSDE_DIR=/path/to/sde
-DSDE_VERSION=91|92
the configuration step will add #define SDE64 if a 64bit platform is detected.

@szekerest this touches the previous windows-only SDE detection. For your builds you should probably now be using -DWITH_SDE_PLUGIN=1 -DSDE_VERSION=91 -DSDE_INCLUDE_DIR=foo -DSDE_LIBRARY_DIR=bar

@sdlime
Member
sdlime commented Aug 6, 2013

Stupid question but how can I merge this with my local copy of master? Is version limited to just 91|92? I've compiled successfully with 10.0 (version=100) as well. --Steve

@tbonfort
Member
tbonfort commented Aug 6, 2013

steve, you can either manually merge https://github.com/mapserver/mapserver/pull/4521.diff , or do it the "git" way

git add remote tbonfort https://github.com/tbonfort/mapserver.git
git fetch tbonfort
git merge tbonfort/cmake-sde

as for the 91|92|100 version, I just tried to copy what was in the autoconf files, i.e. if 91, use -lsde91 -lsg91 etc, otherwise use -lsde -lsg. I have no idea how that naming evolved for 10.0.

@sdlime
Member
sdlime commented Aug 8, 2013

Kinda works just using -DSDE_VERSION=100. I end up with a bunch of errors of the form:

libmapserver.so.6.3-dev: undefined reference to `SE_layerinfo_create'

so somthing isn't quite right. Still, I think this is ok to merge and we can tweak from there. Interestingly SDE ships with PostgreSQL (libpq.so) so I see an error like:

Cannot generate a safe linker search path for target mapserv because file
in some directories may conflict with libraries in implicit directories:

link library [libpq.so] in /usr/lib64 may be hidden by files in:
  /opt/sde/sdeexe100/lib

Some of these libraries may not be found correctly.

I assume this has always been a potential issue and this just exlicitly notes the fact.

Couple of other ideas:

  • maybe the 'cmake ..' output could note that it's detected a 64-bit system
  • 'cmake ..' output should acknowledge/echo standard settings like -DCMAKE_INSTALL_PREFIX=/usr/local/mapserver-6-4. It's a little scary to do a 'make install' without that confirmation.
@sdlime sdlime merged commit 90ff3a6 into mapserver:master Aug 8, 2013

1 check passed

default The Travis CI build passed
Details
@sdlime
Member
sdlime commented Aug 8, 2013

Thomas/Daniel: Working through the long issue now. I'm using SE_INT32 in mapsde.c instead of long. One issue is that shapeObj->index is also a long and SDE barfs on it in a 64-bit settting. Do you see any problem with using ms_int32 for that definition (mapprimitive.h)? Things compile fine and I've used a patched version of MapServer 6.0 with this modification for 6 month but have not tested on a 32-bit box. --Steve

Steve

@dmorissette
Contributor

I'm not aware of cases where 64 bit are required for the shapeObj->index at the moment, but there could be cases I'm not aware of.

So it may be safe to use ms_int32 for the shapeObj->index for the time being, but we should keep in mind that use cases for 64 bit shape ids will come up eventually and this patch will have to be reverted at that time, or a cast or other mechanism put in place in the SDE code to deal wit the 32 bit ids.

Actually, why not simply cast the shape->index to ms_int32 in the SDE code? Since you know that the value originates from the SDE code, this would be a safe thing to do, no?

@sdlime
Member
sdlime commented Aug 8, 2013

Wasn’t sure if that cast was legal…

From: Daniel Morissette [mailto:notifications@github.com]
Sent: Thursday, August 08, 2013 3:38 PM
To: mapserver/mapserver
Cc: Lime, Steve D (MNIT)
Subject: Re: [mapserver] Building SDE support on 64-bit Linux needs work. (#4521)

I'm not aware of cases where 64 bit are required for the shapeObj->index at the moment, but there could be cases I'm not aware of.

So it may be safe to use ms_int32 for the shapeObj->index for the time being, but we should keep in mind that use cases for 64 bit shape ids will come up eventually and this patch will have to be reverted at that time, or a cast or other mechanism put in place in the SDE code to deal wit the 32 bit ids.

Actually, why not simply cast the shape->index to ms_int32 in the SDE code? Since you know that the value originates from the SDE code, this would be a safe thing to do, no?


Reply to this email directly or view it on GitHubhttps://github.com/mapserver/mapserver/pull/4521#issuecomment-22353633.

@dmorissette
Contributor

It wouldn't be safe to do this if you were not sure that the value won't overflow the 32 bits of a ms_int32... but since you know that the shape id value originates from the SDE code which is limited to 32 bits... that would be safe I think.

@tbonfort
Member
tbonfort commented Aug 9, 2013

install dir is printed in 476569c

@sdlime sdlime added a commit that referenced this pull request Aug 13, 2013
@sdlime sdlime Updated msSDELayerFreeItemInfo() to make sure the layerinfo struct is…
… allocated before freeing. (#4521/#4700)
fcc56a7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment