#6191 closed enhancement (fixed)
Add IBM DB2 spatial support to OGR
Reported by: | dadler | Owned by: | dadler |
---|---|---|---|
Priority: | normal | Milestone: | 2.1.0 |
Component: | OGR_SF | Version: | svn-trunk |
Severity: | normal | Keywords: | IBM DB2 OGR |
Cc: |
Description (last modified by )
Many relational databases with spatial support have GDAL/OGR drivers but one doesn't currently exist for the IBM DB2 database.
A customer has requested this support and in response we have created a first version of this which support the DB2 LUW and DB2 for z/OS products.
So far this has been tested with, test_ogrsf, ogrinfo, ogr2ogr to import a shapefile to DB2, ogr2ogr to export a shapefile from DB2, and a GDAL autotest Python script
The attached file contains an 'svn diff' of the code changes and additions. All of the DB2 driver code should go in ogr/ogrsf_frmts/db2.
The following files outside the DB2 driver were changed:
apps/test_ogrsf.cpp
- add code to check for DB2 driver
- change HUGE_COORDS from 1e300 to 1e70 as the largest floating pt number on z/OS is around 1e75. If this change isn't acceptable, it will just result in the huge coords test failing
ogr/ogrsf_frmts/generic/makefile.vc
- add DB2_ENABLED format variable
ogr/ogrsf_frmts/makefile.vc
- add DB2 support
ogr/ogrsf_frmts/generic/ogrregisterall.cpp
- add call to register DB2 driver
ogr/ogrsf_frmts/ogrsf_frmts.h
- add DB2 register function
The current driver is in db2.patch. Ignore db2.zip (I don't know how to delete this)
A DB2 test for autotest is in db2_test.patch.
Attachments (3)
Change History (40)
by , 9 years ago
comment:1 by , 9 years ago
Description: | modified (diff) |
---|
comment:2 by , 9 years ago
Description: | modified (diff) |
---|
comment:3 by , 8 years ago
Description: | modified (diff) |
---|
comment:4 by , 8 years ago
Description: | modified (diff) |
---|
comment:5 by , 8 years ago
Description: | modified (diff) |
---|
follow-up: 7 comment:6 by , 8 years ago
comment:7 by , 8 years ago
Replying to mloskot:
Note regarding the HUGE_COORDS change, I guess it might be not accepted. Instead, it might be possible to replace the compile-time define with value based on driver (e.g. 1e300 as default, while for DB2 driver it would be 1e70).
Still, not ideal but better than changing current test which worked.
Or clamp values in the implementation of SetSpatialFilter() in the DB2 driver. The purpose of 1e300 was to detect buffer overflows of the kind 'char szbuffer[80]; sprintf(szBuffer, "%f", x)'
follow-ups: 11 13 comment:8 by , 8 years ago
I have done brief review of DB2 driver, a walk through ReviewerChecklist and rfc8_devguide but without any attempt to build or run it.
- Whitespaces
Use spaces instead of hard tab characters in source code. There is mixture of both in the submitted files. Also, hard tabs affect indentation.
I'm not sure how hard is the "Use 4 character indentation levels." rule of RFC8 but there are places where indentation could be correct, e.g.:
+ virtual void ResetReading(); + virtual GIntBig GetFeatureCount( int );
Also, there are trailing spaces which shouldn't be there, like after declaration of AppendFieldValue in OGRDB2TableLayer class.
- drv_db2.html
- Not linked from ogr/ogrsf_frtms/ogr_formats.html
- AFAIU, currently the driver is Windows-only, it should be mentioned.
- It may be good to add note on installation/building and third-party software required, if any.
- I would avoid unnecessary #define's in ogr_db2.h and move to .cpp file, especially that DB2LAYERSTATUS_* seems to be only used in ogrdb2datasource.cpp
- The convention for variable names is to capitalize each word in a variable name.
There are places that that need to be corrected, e.g.
+ void AppendFieldValue(CPLODBCStatement *poStatement, + OGRFeature* poFeature, int i, int *bind_num, void **bind_buffer);
- Perhaps, int used for boolean variables could be changed to C++ bool where possible (i.e. not part of public interface), then it would save Kurt Schwehr's on refactoring work.
- See my previous comment to HUGE_COORDS change and Even's answer.
- Try to keep lines to 79 characters or less.
There are lines exceeding even 160 characters.
- Avoid leading underscore in preprocessor #define's.
For example here, leading underscore is C/C++ reserved it does not conform ot C/C++ standard http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html Not sure how GDAL is strong about this, but _OGR_DB2_H_INCLUDED better reads e.g. GDAL_OGR_DB2_H_INCLUDED
- ogr/ogrsf_frmts/generic/makefile.vc
I understand that the driver does not depend on any thirdpardy software, hence the compilation is enabled by default.
- Is OGRDB2Driver class needed at all?
- End-of-line characters should be the Unix way (\n)
AFAICT, there are CRLF, so it should be replaced with LF.
- db2_test.py
The driver is Windows-only, so perhaps it should be skipped with simple check if os.name != 'nt', though I don't know what is the recommended way to do that in autotest.
I have no more comments. If I have a chance to build/run it, I will let you know. Meanwhile, I see no reason not to accept it.
comment:9 by , 8 years ago
David,
Also, since GDAL AppVeyor setup builds with Visual C++ flag /WX, it is important to ensure the code compiles warnings-free.
I will do my best to build it myself too, once updated patch is attached.
follow-up: 12 comment:10 by , 8 years ago
Thank you for the review and comments.
I will update the patch.
Did you use some tools to check the tabs, line ending and line lengths? Are there some tools to fix this? I can certainly write a Perl script to fix it but don't need to recreate the wheel.
Is there a preferred or most common Linux distribution that is used for GDAL?
comment:11 by , 8 years ago
Replying to mloskot:
Regarding the leading underscore, all the OGR .h files appear to use this approach, although I can certainly make the change.
I have done brief review of DB2 driver, a walk through ReviewerChecklist and rfc8_devguide but without any attempt to build or run it.
- Whitespaces
Use spaces instead of hard tab characters in source code. There is mixture of both in the submitted files. Also, hard tabs affect indentation.
I'm not sure how hard is the "Use 4 character indentation levels." rule of RFC8 but there are places where indentation could be correct, e.g.:
+ virtual void ResetReading(); + virtual GIntBig GetFeatureCount( int );Also, there are trailing spaces which shouldn't be there, like after declaration of AppendFieldValue in OGRDB2TableLayer class.
- drv_db2.html
- Not linked from ogr/ogrsf_frtms/ogr_formats.html
- AFAIU, currently the driver is Windows-only, it should be mentioned.
- It may be good to add note on installation/building and third-party software required, if any.
- I would avoid unnecessary #define's in ogr_db2.h and move to .cpp file, especially that DB2LAYERSTATUS_* seems to be only used in ogrdb2datasource.cpp
- The convention for variable names is to capitalize each word in a variable name.
There are places that that need to be corrected, e.g.
+ void AppendFieldValue(CPLODBCStatement *poStatement, + OGRFeature* poFeature, int i, int *bind_num, void **bind_buffer);
- Perhaps, int used for boolean variables could be changed to C++ bool where possible (i.e. not part of public interface), then it would save Kurt Schwehr's on refactoring work.
- See my previous comment to HUGE_COORDS change and Even's answer.
- Try to keep lines to 79 characters or less.
There are lines exceeding even 160 characters.
- Avoid leading underscore in preprocessor #define's.
For example here, leading underscore is C/C++ reserved it does not conform ot C/C++ standard http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html Not sure how GDAL is strong about this, but _OGR_DB2_H_INCLUDED better reads e.g. GDAL_OGR_DB2_H_INCLUDED
- ogr/ogrsf_frmts/generic/makefile.vc
I understand that the driver does not depend on any thirdpardy software, hence the compilation is enabled by default.
- Is OGRDB2Driver class needed at all?
- End-of-line characters should be the Unix way (\n)
AFAICT, there are CRLF, so it should be replaced with LF.
- db2_test.py
The driver is Windows-only, so perhaps it should be skipped with simple check if os.name != 'nt', though I don't know what is the recommended way to do that in autotest.
I have no more comments. If I have a chance to build/run it, I will let you know. Meanwhile, I see no reason not to accept it.
comment:12 by , 8 years ago
Replying to dadler:
Did you use some tools to check the tabs, line ending and line lengths?
Since you are targeting the driver for Windows first, I assume you are developing with Visual Studio. And, since the number of source files is not large, you an simply use Untabify selected lines command.
The trailing whitespaces can be auto-removed with Trailing Whitespace Visualizer extension.
Finally, change VS options to Insert Spaces for C/C++ language.
I hope it helps.
Is there a preferred or most common Linux distribution that is used for GDAL?
Possibly, simply, Ubuntu.
Replying to dadler:
Regarding the leading underscore, all the OGR .h files appear to use this approach, although I can certainly make the change.
Yes, I'm aware of the legacy code, that's why I said I'm "not sure how GDAL is strong about this" regarding new code. IMHO, it should be fixed.
comment:13 by , 8 years ago
Replying to mloskot:
I have done brief review of DB2 driver, a walk through ReviewerChecklist and rfc8_devguide but without any attempt to build or run it.
- Whitespaces
Use spaces instead of hard tab characters in source code. There is mixture of both in the submitted files. Also, hard tabs affect indentation.
I'm not sure how hard is the "Use 4 character indentation levels." rule of RFC8 but there are places where indentation could be correct, e.g.:
+ virtual void ResetReading(); + virtual GIntBig GetFeatureCount( int );Also, there are trailing spaces which shouldn't be there, like after declaration of AppendFieldValue in OGRDB2TableLayer class.
I think I fixed all the tabs to spaces, trailing spaces and indentation.
- drv_db2.html
- Not linked from ogr/ogrsf_frtms/ogr_formats.html
- AFAIU, currently the driver is Windows-only, it should be mentioned.
- It may be good to add note on installation/building and third-party software required, if any.
I modified ogr/ogrsf_frtms/ogr_formats.html to include DB2 but none of the links actually work.
- I would avoid unnecessary #define's in ogr_db2.h and move to .cpp file, especially that DB2LAYERSTATUS_* seems to be only used in ogrdb2datasource.cpp
Done.
- The convention for variable names is to capitalize each word in a variable name.
There are places that that need to be corrected, e.g.
+ void AppendFieldValue(CPLODBCStatement *poStatement, + OGRFeature* poFeature, int i, int *bind_num, void **bind_buffer);
A number of the issues you mentioned relate to modifying the MS SQL Spatial driver to support DB2. I've fixed most issues except for the capitalization convention. I'm currently integrating raster image support and would like to incorporate this change with this effort.
- Perhaps, int used for boolean variables could be changed to C++ bool where possible (i.e. not part of public interface), then it would save Kurt Schwehr's on refactoring work.
Will consider.
- See my previous comment to HUGE_COORDS change and Even's answer.
Done - removed HUGE_COORDS change.
- Try to keep lines to 79 characters or less.
There are lines exceeding even 160 characters.
Fixed all of these. It does make it more readable. It is somewhat ironic that there are conventions associated with the size of a computer punched card and 24x80 "green screen" displays.
- Avoid leading underscore in preprocessor #define's.
For example here, leading underscore is C/C++ reserved it does not conform ot C/C++ standard http://www.gnu.org/software/libc/manual/html_node/Reserved-Names.html Not sure how GDAL is strong about this, but _OGR_DB2_H_INCLUDED better reads e.g. GDAL_OGR_DB2_H_INCLUDED
I'd rather be consistent with all the other OGR drivers that use the leading underscore.
- ogr/ogrsf_frmts/generic/makefile.vc
I understand that the driver does not depend on any thirdpardy software, hence the compilation is enabled by default.
Yes.
- Is OGRDB2Driver class needed at all?
Isn't the driver class what is registered? All the other drivers use a corresponding class.
- End-of-line characters should be the Unix way (\n)
AFAICT, there are CRLF, so it should be replaced with LF.
These should all be fixed.
- db2_test.py
The driver is Windows-only, so perhaps it should be skipped with simple check if os.name != 'nt', though I don't know what is the recommended way to do that in autotest.
Added the test for 'nt'.
I have no more comments. If I have a chance to build/run it, I will let you know. Meanwhile, I see no reason not to accept it.
comment:14 by , 8 years ago
I've tried to build the driver with Visual Studio 2015 and there are two issues:
- The driver seems to include DB2 header, sqlcli1.h, whereas it was mentioned it is based on the ODBC. Has it changed?
cl /nologo /MD /EHsc /Ox /FC /D_CRT_SECURE_NO_DEPRECATE /D_CRT_NONSTDC_NO_DEPRECATE /DNDEBUG /W4 /wd4127 /wd4251 /wd4275 /wd4786 /wd4100 /wd4245 /wd4206 /wd4611 /DHAVE_SSE_AT_COMPILE_TIME -I..\..\..\port -I..\..\..\ogr -I..\..\..\gcore -I..\..\..\alg -I..\..\..\ogr\ogrsf_frmts -I..\..\..\gnm -I..\..\..\gnm\gnm_frmts -I..\..\..\apps /DHAVE_AVX_AT_COMPILE_TIME -I.. -I..\.. -DOGR_ENABLED -DGDAL_COMPILATION /c ogrdb2driver.cpp ogrdb2datasource.cpp ogrdb2layer.cpp ogrdb2tablelayer.cpp ogrdb2selectlayer.cpp ogrdb2driver.cpp d:\dev\gdal\trunk\gdal\ogr\ogrsf_frmts\db2\ogrdb2driver.cpp(29): fatal error C1083: Cannot open include file: 'sqlcli1.h': No such file or directory
- Compilation is not warnings free. Althought GDAL at large does not compile with VS2015 without warnings yet, this may be worth to address.
ogrdb2layer.cpp(93): warning C4458: declaration of 'poStmt' hides class member ogr_db2.h(58): note: see declaration of 'OGRDB2Layer::poStmt' ogrdb2layer.cpp(310): warning C4456: declaration of 'iField' hides previous local declaration ogrdb2layer.cpp(271): note: see declaration of 'iField' ogrdb2tablelayer.cpp(273): warning C4458: declaration of 'pszLayerName' hides class member ogr_db2.h(143): note: see declaration of 'OGRDB2TableLayer::pszLayerName' ogrdb2tablelayer.cpp(657): warning C4458: declaration of 'pszQuery' hides class member ogr_db2.h(133): note: see declaration of 'OGRDB2TableLayer::pszQuery'
Other than that, it looks good to me and ready for submission.
comment:15 by , 8 years ago
In regards to point #1. The include of sqlcli1.h is commented out in ogr_db2.h in the patch file that I uploaded. But I see that this is included redundantly and erroneously in ogrdb2driver.cpp. Should I upload another version with this removed?
In regards to #2, there were no warnings when I use nmake with VS 10.0. Is there some additional compiler flag to report these?
It would be helpful to have committer status for the db2-specific code. I wouldn't change anything else without the appropriate project controls. (At some point it would be nice to do some refactoring as I'm copying and duplicating a lot of code that has nothing to do with DB2.)
comment:16 by , 8 years ago
Uploaded a new db2.patch removing the extraneous include.
I will work on the other warning messages as I apply the m_ convention to class member variables as part of the raster support.
comment:17 by , 8 years ago
David,
Thanks for removing the sqlcli1.h. Clean patch is always better.
Regarding the warnings, I did not try any different compilation flags.; Simply, later versions of compilers usually become more strict than previous ones. Microsoft has made a mile step in VS2015 in this regard.
I have requested commit access for you (http://lists.osgeo.org/pipermail/gdal-dev/2015-December/043210.html). Please, reply regarding the RFC3 matter. Hopefully, the PSC will vote to pass my motion.
follow-up: 20 comment:19 by , 8 years ago
Replying to rouault:
For the record: GDAL now builds warning free with VS2015
I'm a bit confused.
Does this have to do with the DB2 driver or in general since the DB2 code is not yet integrated.
Should the next step for the DB2 driver be for someone to give me committer access and then I check in my changes associated with this ticket?
comment:20 by , 8 years ago
Does this have to do with the DB2 driver or in general since the DB2 code is not yet integrated.
In general (except in the DB2 driver that I've no tested) : https://ci.appveyor.com/project/rouault/gdal-coverage/history has a trunk_vc12 (VS 2013) and trunk_vc13 (VS 2015) branches with a minimal build (no external dependency)
Should the next step for the DB2 driver be for someone to give me committer access and then I check in my changes associated with this ticket?
Yes, I believe Mateusz should declare the motion passed soon and I'll grant you commit rights.
comment:21 by , 8 years ago
Owner: | changed from | to
---|
The motion passed (https://lists.osgeo.org/pipermail/gdal-dev/2015-December/043229.html).
David, once Even grants you with the commit access, you are welcome to submit your driver into the trunk and close the ticket :)
Thanks!
comment:22 by , 8 years ago
OGR DB2 driver committed with revision 32093. DB2 autotest case committed with revision 32095.
comment:23 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
Version: | unspecified → svn-trunk |
follow-up: 30 comment:24 by , 8 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
- In https://trac.osgeo.org/gdal/changeset/32093#file11 -DDB2_ENABLED shouldn't be put in the BASEFORMATS but in the ODBCDEF line , so as to allow ODBC-less builds
- Build failure on error-on-warning VC12 amd64 build : https://ci.appveyor.com/project/rouault/gdal-coverage/build/1.0.1481
C:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts>cd db2 && nmake /nologo /f makefile.vc && cd .. || exit 1 cl /Zi /Fd..\..\..\gdal201.pdb /nologo /MD /EHsc /FC /D_CRT_SECURE_NO_DEPRECATE /D_CRT_NONSTDC_NO_DEPRECATE /DDEBUG /W4 /wd4127 /wd4251 /wd4275 /wd4786 /wd4100 /wd4245 /wd4206 /wd4611 /WX /DHAVE_SSE_AT_COMPILE_TIME -I..\..\..\port -I..\..\..\ogr -I..\..\..\gcore -I..\..\..\alg -I..\..\..\ogr\ogrsf_frmts -I..\..\..\gnm -I..\..\..\gnm\gnm_frmts -I..\..\..\apps /DHAVE_AVX_AT_COMPILE_TIME -I.. -I..\.. -DOGR_ENABLED -DGDAL_COMPILATION /c ogrdb2driver.cpp ogrdb2datasource.cpp ogrdb2layer.cpp ogrdb2tablelayer.cpp ogrdb2selectlayer.cpp ogrdb2driver.cpp ogrdb2datasource.cpp c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(133) : error C2220: warning treated as error - no 'object' file generated c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(133) : warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(265) : warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(495) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(578) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(655) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data ogrdb2layer.cpp ogrdb2tablelayer.cpp ogrdb2selectlayer.cpp
follow-up: 31 comment:25 by , 8 years ago
You might want to remove the CPLDebug() traces in OGRDB2DriverIdentify() and OGRDB2DriverOpen() since they will be called for all datasources identification. We want to limit the verbosity at such generic level.
follow-up: 32 comment:26 by , 8 years ago
The i386 VC12 build at https://ci.appveyor.com/project/rouault/gdal-coverage/build/1.0.1481/job/ewwgdbxc3f3x0m45 succeeds but the autotest/cpp/gdal_unit_test.exe tests crashes with
cd autotest cd cpp nmake /f makefile.vc check MSVC_VER=1800 %WIN64_ARG% Microsoft (R) Program Maintenance Utility Version 12.00.21005.1 Copyright (C) Microsoft Corporation. All rights reserved. gdal_unit_test.exe ERROR 7: Assertion `FALSE' failed in file `c:\projects\gdal-coverage\gdal\gcore\gdaldrivermanager.cpp', line 405
This is the first time I see that... Hum, actually I think I know the reason. There's an inconsistency in ogrdb2driver.cpp :
if (GDALGetDriverByName("DB2") != NULL) return; poDriver = new GDALDriver(); poDriver->SetDescription( "DB2ODBC" );
The string tested in GDALGetDriverByName() and in SetDescription() should be the same so as to detect driver re-registration correctly, which must be what is tested here.
follow-up: 34 comment:27 by , 8 years ago
Linux builds also fail on tests https://s3.amazonaws.com/archive.travis-ci.org/jobs/95619543/log.txt
Running tests from ogr/ogr_db2.py TEST: ogr_db2_check_driver ... fail
--> this should return 'skip' to avoid job failure
follow-up: 33 comment:28 by , 8 years ago
Not critical, but "poDriver->SetMetadataItem( GDAL_DCAP_RASTER, "YES" );" is not appropriate given the current capabilities of the driver.
comment:29 by , 8 years ago
Please set the test to executable so that people can run just that test. e.g.
follow-up: 35 comment:30 by , 8 years ago
Replying to rouault:
- In https://trac.osgeo.org/gdal/changeset/32093#file11 -DDB2_ENABLED shouldn't be put in the BASEFORMATS but in the ODBCDEF line , so as to allow ODBC-less builds
Fixed.
- Build failure on error-on-warning VC12 amd64 build : https://ci.appveyor.com/project/rouault/gdal-coverage/build/1.0.1481
C:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts>cd db2 && nmake /nologo /f makefile.vc && cd .. || exit 1 cl /Zi /Fd..\..\..\gdal201.pdb /nologo /MD /EHsc /FC /D_CRT_SECURE_NO_DEPRECATE /D_CRT_NONSTDC_NO_DEPRECATE /DDEBUG /W4 /wd4127 /wd4251 /wd4275 /wd4786 /wd4100 /wd4245 /wd4206 /wd4611 /WX /DHAVE_SSE_AT_COMPILE_TIME -I..\..\..\port -I..\..\..\ogr -I..\..\..\gcore -I..\..\..\alg -I..\..\..\ogr\ogrsf_frmts -I..\..\..\gnm -I..\..\..\gnm\gnm_frmts -I..\..\..\apps /DHAVE_AVX_AT_COMPILE_TIME -I.. -I..\.. -DOGR_ENABLED -DGDAL_COMPILATION /c ogrdb2driver.cpp ogrdb2datasource.cpp ogrdb2layer.cpp ogrdb2tablelayer.cpp ogrdb2selectlayer.cpp ogrdb2driver.cpp ogrdb2datasource.cpp c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(133) : error C2220: warning treated as error - no 'object' file generated c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(133) : warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(265) : warning C4244: 'initializing' : conversion from '__int64' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(495) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(578) : warning C4267: '=' : conversion from 'size_t' to 'int', possible loss of data c:\projects\gdal-coverage\gdal\ogr\ogrsf_frmts\db2\ogrdb2datasource.cpp(655) : warning C4267: 'initializing' : conversion from 'size_t' to 'int', possible loss of data
Fixed. This was code that I had copied not long ago from the MSSQLSpatial driver... I can see that it has since been fixed to avoid this problem.
Is there any simple way to test for problems like this if I am using a 32-bit compiler?
ogrdb2layer.cpp ogrdb2tablelayer.cpp ogrdb2selectlayer.cpp }}}
comment:31 by , 8 years ago
Replying to rouault:
You might want to remove the CPLDebug() traces in OGRDB2DriverIdentify() and OGRDB2DriverOpen() since they will be called for all datasources identification. We want to limit the verbosity at such generic level.
Fixed.
comment:32 by , 8 years ago
Replying to rouault:
The i386 VC12 build at https://ci.appveyor.com/project/rouault/gdal-coverage/build/1.0.1481/job/ewwgdbxc3f3x0m45 succeeds but the autotest/cpp/gdal_unit_test.exe tests crashes with
cd autotest cd cpp nmake /f makefile.vc check MSVC_VER=1800 %WIN64_ARG% Microsoft (R) Program Maintenance Utility Version 12.00.21005.1 Copyright (C) Microsoft Corporation. All rights reserved. gdal_unit_test.exe ERROR 7: Assertion `FALSE' failed in file `c:\projects\gdal-coverage\gdal\gcore\gdaldrivermanager.cpp', line 405This is the first time I see that... Hum, actually I think I know the reason. There's an inconsistency in ogrdb2driver.cpp :
if (GDALGetDriverByName("DB2") != NULL) return; poDriver = new GDALDriver(); poDriver->SetDescription( "DB2ODBC" );The string tested in GDALGetDriverByName() and in SetDescription() should be the same so as to detect driver re-registration correctly, which must be what is tested here.
Fixed.
comment:33 by , 8 years ago
Replying to rouault:
Not critical, but "poDriver->SetMetadataItem( GDAL_DCAP_RASTER, "YES" );" is not appropriate given the current capabilities of the driver.
Fixed.
comment:34 by , 8 years ago
Replying to rouault:
Linux builds also fail on tests https://s3.amazonaws.com/archive.travis-ci.org/jobs/95619543/log.txt
Running tests from ogr/ogr_db2.py TEST: ogr_db2_check_driver ... fail--> this should return 'skip' to avoid job failure
There is a test at the end of the testcase driver whether it is on Windows - but I just realized this is only executed when the test is executed standalone.. Fixed.
comment:35 by , 8 years ago
Is there any simple way to test for problems like this if I am using a 32-bit compiler?
Thanks for adressing the issues. The build bots are green now. I encourage you checking https://travis-ci.org/rouault/gdal_coverage/builds/ and https://ci.appveyor.com/project/rouault/gdal-coverage/history after your commits ( the failure on trunk_gcc5.2_sanitize is expected )
Regarding your question, I'm not aware of options to easily check warnings thrown in 64bit when you compile in 32bit (although conceptually the compiler could anticipate size_t/int incompatibilities, but I guess they have a hard enough job trying to figure out the right warnings for the current architecture).
comment:36 by , 8 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
Note regarding the HUGE_COORDS change, I guess it might be not accepted. Instead, it might be possible to replace the compile-time define with value based on driver (e.g. 1e300 as default, while for DB2 driver it would be 1e70).
Still, not ideal but better than changing current test which worked.