Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build and run tests for the MSSQL Driver on Appveyor #5990

Merged
merged 4 commits into from
Feb 13, 2020

Conversation

geographika
Copy link
Member

This pull request adds msautotests for the MSSQL MapServer driver, and runs them as part of the Windows Appveyor build.

Appveyor has a MSSQL 2017 database service added. A test database is created with a couple of shapefiles imported using ogr2ogr. Appveyor shallow_clone was disabled as the msautotest suite is not available by the GitHub HTTP download (it is in the .gitignore list). The -DWITH_MSSQL2008=1 setting was added to the CMake command.

The line include_directories(${ODBC_INCLUDE_DIR}) was removed from CMakeLists.txt as it caused build errors. See https://ci.appveyor.com/project/SethG/mapserver/build/job/o3ch02bfevwwcrkc for an example where errors such as those below are listed. I believe this is due to WINAPI being redefined/undefined when including all the files in ODBC_INCLUDE_DIR (which is C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\um\ on Appveyor):

C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\um\fileapifromapp.h(32): error C2054: expected '(' to follow 'WINSTORAGEAPI' [C:\projects\mapserver\build\mapserver.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\um\fileapifromapp.h(37): error C2085: 'CopyFileFromAppW': not in formal parameter list [C:\projects\mapserver\build\mapserver.vcxproj]
C:\Program Files (x86)\Windows Kits\10\Include\10.0.18362.0\um\fileapifromapp.h(40): error C2061: syntax error: identifier 'WINSTORAGEAPI' [C:\projects\mapserver\build\mapserver.vcxproj]

A first test MAP file was created based on the existing filters_postgis.map file to test MSSQL driver functionality. Several FILTER options don't work with the MSQL driver so these are currently ignored (a separate issue can be created for these).

Code warnings have been suppressed for mapmssql2008.c as my attempts to fix them caused memory issues, and not suppressing them cause the build test to fail. A few code issues are listed below.

  d:\github\mapserver\mapmssql2008.c(909): warning C4477: 'sprintf' : format string '%d' requires an argument of type 'int', but variadic argument 1 has type 'SQLULEN' [D:\MapServer\release-1911-x64-dev\build\msplugin_mssql2008.vcxproj]
  d:\github\mapserver\mapmssql2008.c(2291): warning C4244: '=': conversion from 'SQLLEN' to 'int', possible loss of data [D:\MapServer\release-1911-x64-dev\build\msplugin_mssql2008.vcxproj]
  d:\github\mapserver\mapmssql2008.c(2561): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\MapServer\release-1911-x64-dev\build\msplugin_mssql2008.vcxproj]
  d:\github\mapserver\mapmssql2008.c(2721): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data [D:\MapServer\release-1911-x64-dev\build\msplugin_mssql2008.vcxproj]

Finally the msautotest/mspython tests recently updated by @rouault in #5984 are also now run on Appveyor/Windows. There were a couple of minor path changes to get all of these to pass. There was one failing test I couldn't resolve so this is skipped on Windows with the following line:

@pytest.mark.skipif(sys.platform == 'win32', reason="Fails on Windows")

geographika and others added 2 commits February 11, 2020 10:34
* Add MSSQL to CI

* Update db script and Python tests

* Turn off shallow_clone to clone msautotest

* Restrict clone size

* Add mssql test case and results

* Build MSSQL driver

* Remove include directory that breaks build

* Set projection and update results paths

* Use file protocol for pip

* Change test order

* Add MSSQL to CI

* Update db script and Python tests

* Turn off shallow_clone to clone msautotest

* Fix YAML issues with %

* Add full path to work on Windows

* Ignore test on Windows as it fails

* Update test and skip failing tests

* Fix some compiler warnings and disable the rest
@rouault
Copy link
Contributor

rouault commented Feb 11, 2020

as my attempts to fix them caused memory issues

Really ? Can you show them ? Simple casts should hopefully fix the warnings. For mapmssql2008.c(909), the right fix is probably using %u instead of %d

@geographika
Copy link
Member Author

Really ? Can you show them ? Simple casts should hopefully fix the warnings. For mapmssql2008.c(909), the right fix is probably using %u instead of %d

I added a few changes using size_t instead but these caused issues in the x32 build e.g. msSmallMalloc(): Out of memory allocating -18 bytes. at https://ci.appveyor.com/project/SethG/mapserver/builds/30706851/job/e3dbxr5mvm5fgqu1#L686

The attempt to fix them was in this commit - but I soon realised I was doing more harm than good.

I looked at #5873 for some hints but didn't see examples of int/size_t fixes. I'll attempt the suggested approach of casts for some of the other errors.

@geographika
Copy link
Member Author

@rouault - I've re-enabled the complier warnings and used casts and changed types to fix these. See this commit for changes. The found = -1 is slightly clunky but I couldn't find a way to store size_t calculations and have a -1 flag in the same variable.

Initial warnings prior to commit were as follows:

mapmssql2008.c(652): warning C4133: 'function': incompatible types - from 'SQLLEN *' to 'SQLINTEGER *' 
mapmssql2008.c(668): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(698): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(699): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(717): warning C4244: '=': conversion from '__int64' to 'int', possible loss of data 
mapmssql2008.c(787): warning C4090: 'function': different 'const' qualifiers 
mapmssql2008.c(909): warning C4477: 'sprintf' : format string '%d' requires an argument of type 'int', but variadic argument 1 has type 'SQLULEN' 
mapmssql2008.c(1355): warning C4101: 'extent': unreferenced local variable 
mapmssql2008.c(2292): warning C4244: '=': conversion from 'SQLLEN' to 'int', possible loss of data 
mapmssql2008.c(2562): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(2689): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(2860): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data 
mapmssql2008.c(2968): warning C4267: 'initializing': conversion from 'size_t' to 'int', possible loss of data 

mapmssql2008.c Outdated
@@ -690,10 +690,10 @@ static char *strstrIgnoreCase(const char *haystack, const char *needle)
{
char *hay_lower;
char *needle_lower;
int len_hay,len_need;
size_t len_hay,len_need, match;
int found = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  bool found = false;

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bool type isn't available (at least not for me using MSVC). I've updated to use MS_FALSE and MS_TRUE for clarity.

mapmssql2008.c Outdated
@@ -715,12 +715,13 @@ static char *strstrIgnoreCase(const char *haystack, const char *needle)
loc = strstr(hay_lower, needle_lower);
if(loc) {
match = loc - hay_lower;
found = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

found = true;

mapmssql2008.c Outdated
}

msFree(hay_lower);
msFree(needle_lower);

return (char *) (match < 0 ? NULL : haystack + match);
return (char *) (found < 0 ? NULL : haystack + match);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return (char *) (!found ? NULL : haystack + match);

@rouault rouault merged commit 28b9f83 into MapServer:master Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants