Repair VC8 compile of test applications #593

hrabe · 2017-09-22T03:45:02Z

Due to unsupported default param usage in macros and no qualified given _WIN32_WINNT version the test apps will not compile with some of Microsoft compilers.
Exception Features used are only available if a minimum version is set due to conditional enabling in windows.h file (and sub required files)

Update master from soci/soci

- soci-platform.h got minimal required _WIN32_WINNT version to enable usage due to windows.h include of Add/RemoveVectoredExceptionHandler - default macro parameter (3 required / 2 given or 2 required / 1 given) do not expand and lead to syntax error (missing ')')

mloskot · 2017-09-22T06:33:00Z

with some of Microsoft compilers.

What are these?

The Catch does not seem to like the change, I'm afraid.
The AppVeyor build is failing.

hrabe · 2017-09-22T07:33:22Z

The AppVeyor show similar things like my local build for ODBC.

C:\projects\soci\tests\catch.hpp(6423): error C3861: 'AddVectoredExceptionHandler': identifier not found [C:\projects\soci\build.msvc2012\tests\sqlite3\soci_sqlite3_test_static.vcxproj]

This should be fixed with the define inside soci/soci-platform.h related to this Remarks: https://msdn.microsoft.com/de-de/library/windows/desktop/ms679274(v=vs.85).aspx
Seems that something will not be compiled using platform?

Tested compiler:
VS2005 -> VC 8
VS2008 -> VC 9
(know that they are aged but still required until rewrite of code is done)

hrabe · 2017-09-22T07:37:48Z

Ok, this one could be catched by conditional:

C:/projects/soci/include/soci/soci-platform.h:77:0: error: "_WIN32_WINNT" redefined [-Werror]
 # define _WIN32_WINNT 0x0500
 ^
In file included from C:/projects/mingw/4.8.3/mingw64/x86_64-w64-mingw32/include/vadefs.h:9:0,
                 from C:/projects/mingw/4.8.3/mingw64/x86_64-w64-mingw32/include/_mingw_stdarg.h:14,
                 from C:/projects/mingw/4.8.3/mingw64/x86_64-w64-mingw32/include/stdarg.h:140,
                 from C:/projects/mingw/4.8.3/mingw64/lib/gcc/x86_64-w64-mingw32/4.8.3/include/stdarg.h:1,
                 from C:/projects/soci/include/soci/soci-platform.h:16,
                 from C:\projects\soci\src\core\backend-loader.cpp:9:
C:/projects/mingw/4.8.3/mingw64/x86_64-w64-mingw32/include/_mingw.h:229:0: note: this is the location of the previous definition
 #define _WIN32_WINNT 0x502
 ^
cc1plus.exe: all warnings being treated as errors

But this is MinGW build I not tested yet.

mloskot

If,

AppVeyor build is fixed
Nobody reports any objections

it LGTM.

mloskot · 2017-09-22T07:40:29Z

include/soci/soci-platform.h

@@ -74,6 +74,7 @@ namespace std {

 //define DLL import/export on WIN32
 #ifdef _WIN32
+# define _WIN32_WINNT 0x0500


I'd do:

# ifndef _WIN32_WINNT # define _WIN32_WINNT 0x0500 # endif

mloskot · 2017-09-22T07:44:41Z

tests/common-tests.h

@@ -537,7 +537,7 @@ TEST_CASE_METHOD(common_tests, "Exception on not connected", "[core][exception]"
    CHECK_THROWS_AS(sql.get_last_insert_id(s, l), soci_error);
 }

-TEST_CASE_METHOD(common_tests, "Basic functionality")
+TEST_CASE_METHOD(common_tests, "Basic functionality", "basics")


Hmm, isn't the third argument of TEST_CASE_METHOD supposed to be a tag?

Aren't Catch tags supposed to be in form of ["tag1", "tag2"]?

mloskot · 2017-09-22T07:45:09Z

tests/common-tests.h

@@ -570,15 +570,15 @@ TEST_CASE_METHOD(common_tests, "Use and into", "[core][into]")

    auto_table_creator tableCreator(tc_.table_creator_1(sql));

-    SECTION("Round trip works for char")
+    SECTION("Round trip works for char", "section")


Same here, I'd make it a proper tag like ["char"]

mloskot · 2017-09-22T07:45:17Z

tests/common-tests.h

    {
        char c('a');
        sql << "insert into soci_test(c) values(:c)", use(c);
        sql << "select c from soci_test", into(c);
        CHECK(c == 'a');
    }

-    SECTION("Round trip works for string")
+    SECTION("Round trip works for string", "section")


Same here, I'd make it a proper tag like ["string"]

mloskot · 2017-09-22T07:45:26Z

tests/common-tests.h

@@ -587,7 +587,7 @@ TEST_CASE_METHOD(common_tests, "Use and into", "[core][into]")
        CHECK(str == "Hello, SOCI!");
    }

-    SECTION("Round trip works for short")
+    SECTION("Round trip works for short", "section")


Same here, I'd make it a proper tag like ["short"]

etc.

mloskot · 2017-09-22T07:47:06Z

tests/common-tests.h

@@ -596,7 +596,7 @@ TEST_CASE_METHOD(common_tests, "Use and into", "[core][into]")
        CHECK(sh == 3);
    }

-    SECTION("Round trip works for int")
+    SECTION("Round trip works for int", "section")


Same here, I'd make it a proper tag like ["int"]

Unless, we are going to get rid of it once SOCI switches to C++11 anyway.

If so, we may agree to replace all those with ["dummy"] (or even empty string if kosher for Catch) as we don't seem to use the tags currently anyway. That would be simpler to re-work the PR with search & replace.

Either would work for me.

hrabe · 2017-09-22T08:50:35Z

After updating the PR AppVeyor passes the MinGW build and tests.
Only the VS based builds have problems. It seems to me that files are not trashed correctly?
Something other than this PR may also influence the 3 Studio builds.

mloskot · 2017-09-22T09:01:02Z

Something other than this PR may also influence the 3 Studio builds.

Let's see if the master builds https://ci.appveyor.com/project/mloskot/soci/build/4.0.0.287

vadz · 2017-09-22T10:40:34Z

The additions of "[dummy]" are really ugly. I do still use VC9 for a project also using CATCH and didn't have any problems like this, is this for VC8 only? It would be nice to find another way to work around this...

And if not, I think there ought to be a macro like DUMMY_TAGS_FOR_MSVC8 or something like this to allow getting rid of it easily when support for this compiler is dropped.

hrabe · 2017-09-22T11:04:26Z

Ok, I will try to find another way but the issue at VC8 by one of many examples is:

2>test-odbc-access.cpp
2>C:\Github\hrabe\soci\tests\common-tests.h(573) : warning C4003: not enough actual parameters for macro 'SECTION'
2>C:\Github\hrabe\soci\tests\common-tests.h(573) : error C2059: syntax error : ')'

on C:\Github\hrabe\soci\tests\common-tests.h(573) as:

    SECTION("Round trip works for char")

and define as:

    #define SECTION( name, description ) INTERNAL_CATCH_SECTION( name, description )

mloskot · 2017-09-22T11:26:18Z

We're at Catch v1.9.6. Could you try to pull the latest (single header) and check if it works?

Meanwhile, I asked to confirm VC++ 8.0 support catchorg/Catch2#1029

hrabe · 2017-09-22T11:48:14Z

Will try that, but there seems to be a problem on MS side: https://connect.microsoft.com/VisualStudio/feedback/details/380090/variadic-macro-replacement

hrabe · 2017-09-22T11:58:33Z

Used now

 *  Catch v1.10.0
 *  Generated: 2017-08-26 15:16:46.676990

for compile but no luck at all, the problem of VC 8 variadic macros with too less argument persist!

mloskot · 2017-09-22T13:09:33Z

@hrabe Thanks for checking Catch 1.10.0. I think we can safely assume VC8 is not (fully) supported any longer.

hrabe · 2017-09-22T13:34:15Z

So how to solve? Drop support of VC 8.0 is no option to me (currently).
Any thoughts how to proceed if this won't be addressed by Catch?
BTW: Tested 2.0 branch of Catch too, I'm afraid this will bring much more dropped support!

And could your please update the issue headline you created at Catch from 2015 to 2005 please ?

mloskot · 2017-09-22T13:47:38Z

@hrabe

So how to solve?

I, personally, don't care about anything older than VS2015 (and GCC/clang with equivalent C++ support).

Nevertheless, I understand

@vadz needs support for older compilers (not sure which versions Vadim needs though)
there might be users who need suport for older compilers (you, @hrabe)

So, until SOCI team decides to switch to C++11 or later, I'm fine accepting PRs with any (reasonable) plumbing required by the older compilers. I, however, am not able to provide any assistance myself or promise I won't break anything with some commits in future - simply, I have no means to try such old compilers. Unless AppVeyor can help us.

Any thoughts how to proceed if this won't be addressed by Catch

Any chance to address @vadz suggestion #593 (comment) ?

Then, either Vadim or myself, we'll be happy to merge this PR in.

hrabe · 2017-09-22T15:16:07Z

Would try the macro for dummy but what would you say @vadz and @mloskot about this:

TEST_CASE_METHOD(common_tests, "Use and into", "[core][into]")
{
    soci::session sql(backEndFactory_, connectString_);

    auto_table_creator tableCreator(tc_.table_creator_1(sql));

    SECTION("Round trip works for char", "[core][into][char]")
    {
        char c('a');
        sql << "insert into soci_test(c) values(:c)", use(c);
        sql << "select c from soci_test", into(c);
        CHECK(c == 'a');
    }

    SECTION("Round trip works for string", "[core][into][string]")
    {
        std::string helloSOCI("Hello, SOCI!");
        sql << "insert into soci_test(str) values(:s)", use(helloSOCI);
        std::string str;
        sql << "select str from soci_test", into(str);
        CHECK(str == "Hello, SOCI!");
    }

    SECTION("Round trip works for short" , "[core][into][short]")
...

Why not using the tags really without dummy but with prefixed TestMethod Tag and Tag for current section like char, string, short etc?

vadz · 2017-09-22T15:21:52Z

I'm not sure, can tags be actually used with sections? I've never done this... If they can, then using real tags is indeed better, but if they can't, then this would be surprising/confusing, so I'd still prefer the dummy macro idea.

In fact... what about something like this:

#if defined(_MSC_VER) && (_MSC_VER < 1500)
#undef SECTION
#define SECTION(name) INTERNAL_CATCH_SECTION(name, "dummy-for-vc8")
#endif

Shouldn't this solve the problem for it?

mloskot · 2017-09-22T15:28:53Z

AFAIK, tags work for TEST_CASE[_METHOD] and SCENARIO macros only.

In fact... what about something like this:
#if defined(_MSC_VER) && (_MSC_VER < 1500)
#undef SECTION
#define SECTION(name) INTERNAL_CATCH_SECTION(name, "dummy-for-vc8")
#endif
Shouldn't this solve the problem for it?

👍

vadz · 2017-09-22T15:30:40Z

In fact the second parameter of the SECTION macro is "description", not "tag" at all.

So we should either add real descriptions to all of them, which seems to be a bit too verbose and redundant with the name, but bearable, or use the hack for VC8 above.

hrabe · 2017-09-22T15:47:20Z

In fact... what about something like this:
#if defined(_MSC_VER) && (_MSC_VER < 1500)
#undef SECTION
#define SECTION(name) INTERNAL_CATCH_SECTION(name, "dummy-for-vc8")
#endif
Shouldn't this solve the problem for it?

This would solve it but only as long as nobody uses descriptions, if so, this workaround will break.

hrabe · 2017-09-22T16:12:36Z

Now Travis complains about wrong CMake version.

CMake Error at CMakeLists.txt:13 (cmake_minimum_required):
  CMake 2.8.10 or higher is required.  You are running version 2.8.7

mloskot · 2017-09-22T17:18:13Z

Now Travis complains about wrong CMake version.

This is due to Travis timeout'ed during apt steps and did not pull CMake update from PPA repository.
The failure is also accompanied with Could not connect to ppa.launchpad.net:80 (91.189.95.83), connection timed out or similar (like here https://travis-ci.org/SOCI/soci/jobs/276502301).
Nudging Travis build usually helps.

I have manually restarted the three failing build jobs.

mloskot · 2017-09-22T18:28:42Z

Travis is green.

AppVeyor is also green https://ci.appveyor.com/project/mloskot/soci/build/4.0.0.288 despite the "Waiting for status to be reported". I'll hit "Update Branch" button to rebase from master and nudge the rebuild.

Once builds complete, it's ready to merge. @vadz you agree?

mloskot · 2017-09-22T18:31:32Z

Holy s***t! I had no idea the "Update branch" button messes the commit history a bit:
with "Merge branch 'master' into master" commit 4a4b9cb

It triggered builds, at least
https://ci.appveyor.com/project/mloskot/soci/build/4.0.0.289
https://travis-ci.org/SOCI/soci/builds/278730017

I'm not gonna use this button any more:)

@hrabe I'm sorry, feel free to git push -f to update your PR as you wish.

hrabe · 2017-09-23T09:16:03Z

PR updated. Hope will now be valid for merge after travis and appveyor.

Small question beside: If I work on a branch at my fork, could I also use travis and appveyor at forked branch?

mloskot · 2017-09-23T09:38:40Z

@hrabe

Small question beside: If I work on a branch at my fork, could I also use travis and appveyor at forked branch?

Yes, of course. I do it myself. Just enable the the services for your fork.

CI builds don't send any notifications to mailing list, so there won't be any unwanted noise.
CI builds send notifications to https://gitter.im/SOCI/soci room though, but that is fine.

Good practices is also to develop contributions in topic branch, not in master branch of your fork. Then, you submit PR from on that topic branch.

hrabe · 2017-09-23T09:46:01Z

Thanks for confirmation and suggestion. Will do next changes in feature branches beside master and will use a forked_master to sum up all in fork too.

mloskot · 2017-09-23T09:52:34Z

@hrabe I wouldn't worry about using forked_master, just keep master on your github.com/hrabe/soci plus topic branches. No need to shadow the real master, unless you want/need yourself.

mloskot · 2017-09-23T11:00:19Z

FYI, I've decide to try out the GitHub's Squash and merge button (second of the three buttons). I think it worked fine, no damage noticed.

mloskot · 2017-09-23T11:01:16Z

@hrabe Thanks for your efforts!

hrabe added 2 commits September 21, 2017 17:14

Merge pull request #1 from SOCI/master

043a638

Update master from soci/soci

mloskot approved these changes Sep 22, 2017

View reviewed changes

mloskot added Toolset/MinGW Toolset/MSVC Category/Test labels Sep 22, 2017

Update PR make VC compiler compatible

00afcfc

Set higher version for newer MSVS like VS2015 and above

a660c0e

mloskot mentioned this pull request Sep 22, 2017

Status of VS2005 (VC++ 8.0) support catchorg/Catch2#1029

Closed

Workarround for VC8 compiler related to few macro params

24860d9

mloskot changed the title ~~Repair MSVC compile of test applications~~ Repair VC8 compile of test applications Sep 22, 2017

hrabe force-pushed the master branch from 1fe803b to 24860d9 Compare September 23, 2017 09:11

mloskot merged commit 51d058f into SOCI:master Sep 23, 2017

mloskot added this to the 4.0.0 milestone Sep 23, 2017

Repair VC8 compile of test applications #593

Repair VC8 compile of test applications #593

Conversation

hrabe commented Sep 22, 2017

mloskot commented Sep 22, 2017

hrabe commented Sep 22, 2017 • edited

hrabe commented Sep 22, 2017 • edited

mloskot left a comment • edited

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

mloskot Sep 22, 2017

Choose a reason for hiding this comment

hrabe commented Sep 22, 2017 • edited

mloskot commented Sep 22, 2017

vadz commented Sep 22, 2017

hrabe commented Sep 22, 2017 • edited

mloskot commented Sep 22, 2017

hrabe commented Sep 22, 2017

hrabe commented Sep 22, 2017 • edited

mloskot commented Sep 22, 2017

hrabe commented Sep 22, 2017 • edited

mloskot commented Sep 22, 2017 • edited

hrabe commented Sep 22, 2017 • edited

vadz commented Sep 22, 2017

mloskot commented Sep 22, 2017

vadz commented Sep 22, 2017

hrabe commented Sep 22, 2017

hrabe commented Sep 22, 2017

mloskot commented Sep 22, 2017

mloskot commented Sep 22, 2017

mloskot commented Sep 22, 2017 • edited

hrabe commented Sep 23, 2017

mloskot commented Sep 23, 2017 • edited

hrabe commented Sep 23, 2017

mloskot commented Sep 23, 2017

mloskot commented Sep 23, 2017

mloskot commented Sep 23, 2017

hrabe commented Sep 22, 2017 •

edited

hrabe commented Sep 22, 2017 •

edited

mloskot left a comment •

edited

hrabe commented Sep 22, 2017 •

edited

hrabe commented Sep 22, 2017 •

edited

hrabe commented Sep 22, 2017 •

edited

hrabe commented Sep 22, 2017 •

edited

mloskot commented Sep 22, 2017 •

edited

hrabe commented Sep 22, 2017 •

edited

mloskot commented Sep 22, 2017 •

edited

mloskot commented Sep 23, 2017 •

edited