Visual Studio 2010 support #129

bel2125 · 2017-08-28T17:59:56Z

Support for C89/C90 compilers like Visual Studio 2010 and 2012

See also #125

While this header is standard in C99, it does not exist in C89/C90. Microsoft Visual Studio added this header only in version 2013. Still common common compilers Visual Studio 2010 and 2012 (and all earlier) do not support it. Here we do not really have a benefit as compared to just using 'int' instead of 'bool', so this dependency can easily be avoided to support Visual Studio 2010 (see also libcheck#125).

… functions

…headers

…and maybe drop some outdated ones later)

bel2125 · 2017-08-28T19:55:14Z

I added about every version of Visual Studio, that in theory should be supported. In practice some builds do not work, since the compiler is missing on the build image.
Visual Studio 2010 and 2012 work now (https://ci.appveyor.com/project/brarcher46583/check/build/1.0.513):

Environment: platform=msvc     ... OK
Environment: platform=vs2005_64 ... No CMAKE_C_COMPILER could be found
Environment: platform=vs2008_64 ... No CMAKE_C_COMPILER could be found
Environment: platform=vs2010_64 ... OK
Environment: platform=vs2012_64 ... OK
Environment: platform=vs2013_64 ... OK
Environment: platform=vs2015_64 ... OK
Environment: platform=vs2017_64 ... No CMAKE_C_COMPILER could be found.
Environment: platform=vs2012_ARM ... OK
Environment: platform=vs2013_ARM ... OK
Environment: platform=vs2015_ARM ... OK
Environment: platform=vs2017_ARM ... No CMAKE_C_COMPILER could be found.
Environment: platform=cygwin ... OK
Environment: platform=mingw32 ... OK
Environment: platform=mingw64msys ... OK

I do not really need 2005 and 2008 any more, and I do not yet need 2017.

For 2017, I think the appveyor.yml file needs image: Visual Studio 2017 (see documentation) to select a different build machine than the standard 2015 build image. I don't know if you need this, or if it makes sense for the "check" project now, or if it has any side effects (like using a slower, experimental, less stable build environment).

I could remove the 2005, 2008 and 2017 builds from the AppVeyor build list, or have a short look at either 2017 and/or the older ones. Just tell me what you prefer.

And please have a look at the changes, if you are willing to merge this, or I should do something different, in particular with the INFINITE and NAN issues.

bel2125 · 2017-08-28T21:21:39Z

Visual Studio 2017 and 2008 are fixed now: https://ci.appveyor.com/project/brarcher46583/check/build/1.0.514

only the Visual Studio 2017 build needs to move to the new 2017 image, while the rest may still use the standard (operationally well-tried) 2015 image
Visual Studio 2008 uses a different syntax

Only 2005 is still failing, but it seems AppVeyor dropped all versions prior to VS 2008 (https://www.appveyor.com/docs/build-environment/#visual-studio-2008) - anyway, I thing supporting VS2008 is already more than I'd expected, VS2005 is pretty similar to VS2008, ... I we can't change the AppVeyor images anyway. So I will remove the VS2005 build from appveyor.yml.

bel2125 · 2017-08-28T22:18:11Z

Continuous integration using AppVeyor is working now for Visual Studio 2008, 2010, 2012, 2013, 2015 and 2017: https://ci.appveyor.com/project/brarcher46583/check/build/1.0.515
I also added some ARM processor builds for VS 2012, 2013, 2015 and 2017.

It would be possible to do 32 and 64 bit builds and add debug and release builds, effectively 4 builds for a Visual Studio version, but I think this would be another pull request, no longer related to #125

brarcher · 2017-08-29T00:12:43Z

Jenkins: ok to test
^ runs the remaining tests

brarcher · 2017-08-29T00:14:33Z

Continuous integration using AppVeyor is working now for Visual Studio 2008, 2010, 2012, 2013, 2015 and 2017

Nice!

It would be possible to do 32 and 64 bit builds and add debug and release builds, effectively 4 builds for a Visual Studio version, but I think this would be another pull request, no longer related to #125

I will defer to you if you think that adding these builds to prevent Check from breaking on them has value to you. I do not normally use Windows (so there are many things that I do not understand about its build environment). However, I do want Check to be operational in different Windows' environments, if possible.

brarcher · 2017-08-29T01:16:58Z

appveyor.yml

@@ -52,7 +63,17 @@ before_build:
  - set PATH=%PATH:C:\Program Files\Git\usr\bin;=%
  - if %platform%==msvc call "C:\Program Files (x86)\Microsoft Visual Studio 12.0\VC\vcvarsall.bat"
  - if %platform%==msvc cmake -G "NMake Makefiles" -DCMAKE_INSTALL_PREFIX=%P%
-  - if %platform%==vs cmake -G "Visual Studio 14 2015 Win64" -DCMAKE_INSTALL_PREFIX=%P%
+  - if %platform%==vs2005_64 cmake -G "Visual Studio 8 2005" -DCMAKE_INSTALL_PREFIX=%P%


This adds more variations for compilation, but not running the unit tests. Should they also run the build unit tests?

Aren't the unit tests run just for the code built by the selected compiler?
At least I saw some tests running.

But there are some extra lines:

if %platform%==vs msbuild "ALL_BUILD.vcxproj"

if %platform%==vs ctest --extra-verbose -C Release

I will check what difference they really make. AppVeyor seemed pretty complete without them ...

I believe the ctest lines are what run the tests. When I looked at the AppVeyor output for the new platforms the unit tests were not being run.

brarcher · 2017-08-29T01:19:03Z

lib/gettimeofday.c

@@ -29,7 +29,7 @@

 int gettimeofday(struct timeval *tv, void *tz)
 {
-#if _MSC_VER


Out of curiosity, what the difference? Not all versions of _MSC_VER evaluated to 1 or true?

#if _MSC_VER
may lead to compile errors - maybe a standard violation for older compilers.
Nevertheless I saw this error (I could try to find the AppVeyor log, if you are really curious).
If you just want to detect if it's defined at all, use
#ifdef _MSC_VER

brarcher · 2017-08-29T01:21:22Z

tests/check_check_sub.c

@@ -267,7 +295,9 @@ START_TEST(test_ck_assert_int_lt)
 {
  int x = 2;
  int y = 3;
+


What adding the spaces necessary for C89 support? Or is this a whitespace only change?

It's not required by the standard. The reason was:

I made all the changes in tests/check_check_sub.c manually, and they were quite a lot.
So I finally came up with a "let's have all functions look like this":

START_TEST(test_something) { type1 var1; type2 var2; record_test_name(tcase_name()); test code more test code } END_TEST

Just to get this structure at first sight. This helped me to keep an overview what is already handled and what is not. Maybe it's helpful for maintenance as well.

But it's your project, so choose whatever structure you like.

Do you actually have something like a clang-format file for automatic code formatting?

I'm fine with the whitespace changes, just curious. I do not have a clang-format file for formatting.

brarcher · 2017-08-29T01:22:30Z

tests/check_check_sub.c

  record_test_name(tcase_name());
-  int result = atexit(exit_handler);
+
+  result = atexit(exit_handler);


If you did all this by hand, I give you credit for your dedication. (:

Yes I did it by hand. I think, there is no other way to do it.
In the end, it turned out as somewhat more work as initially expected ("just replace stdbool.h").
But it should be in a form that can be maintained in the project, so the test has to work as well, right?

brarcher · 2017-08-29T01:24:14Z

tests/check_check_sub.c

@@ -27,6 +27,17 @@
 #include <check.h>
 #include "check_check.h"

+#if !defined(NAN)


As mentioned earlier, is there a way to put this in libcompat.h?

The definition can be moved there.
But the definition alone is not enough - there needs to be some code to initialize the value. You need to generate the NAN and INFINITY at runtime, basically at start time of the project. Currently there is no initialization function in libcompat.c.
I can try if this can be done with some preprocessor magic, or by nesting some initializations.
But it's probably not really working if there is no initialization function.

brarcher · 2017-08-29T01:24:41Z

tests/check_check_sub.c

@@ -37,6 +37,40 @@ double NAN;
 #if !defined(INFINITY)
 double INFINITY;
 #endif
+#if !defined(isnan)


Can this be put into the lib dir as a reimplementation of these if they do not exist?

See timer_settime as an example.

I will try (37a1188)

bel2125 · 2017-08-29T22:07:39Z

Infinity/NaN code moved, AppVeyor is working (https://ci.appveyor.com/project/brarcher46583/check/build/1.0.517), Travis as well, but I just don't know what the test called "default" is supposed to be.

"Default" indicates a failed test (https://check.ci.cloudbees.com/job/github-merge-builder/284/)

I'm trying to get the 32 bit architecture for Windows running as well, but this may take some attempts.

bel2125 · 2017-08-29T22:35:44Z

To move INFINITY and NAN into libcompat.h, I would need an initialisation function.
I can't assign this as an initializer in VS 2010: https://ci.appveyor.com/project/brarcher46583/check/build/1.0.519/job/su1wjaku63aukeed#L226

Is there some function in the check library, that is always called first?

…iable

brarcher · 2017-08-31T02:04:26Z

but I just don't know what the test called "default" is supposed to be.

There are some tests which are run on a Jenkins instance. For whatever reason Github calls it "default". The test which is failing is the first one it attempts, here. The test runs the make prereleasecheck target (which builds check, runs the unit tests, then packages Check up in a tarball for release), then unpacks the tarball and build it using CMake. The CMake build is failing due to the following:

16:59:33 CMake Error at lib/CMakeLists.txt:68 (add_library):
16:59:33   Cannot find source file:
16:59:33 
16:59:33     fpclassify.c
16:59:33 
16:59:33   Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
16:59:33   .hxx .in .txx

I think that fpclassify.c needs to be mentioned in the autotools files so it is packaged when creating a release.

brarcher · 2017-08-31T02:15:54Z

Is there some function in the check library, that is always called first?

If getting these functions into libcompat is turning into a hassle, I would not worry about it then. The functions are used only in a unit test, and only in one file at that. I was hoping to have all the code for platform specific support in one spot, but having an initialization in Check for test code may be too much.

If you want to keep trying, you could put the initializations in the code which sets up the tests to be run, such as in check_check_sub.c's make_sub_suite(). Alternatively, you could have a getter function in that file which returns the appropriate value of INFINITY or NAN and the value is just computed each time.

bel2125 · 2017-08-31T08:08:55Z

16:59:33 CMake Error at lib/CMakeLists.txt:68 (add_library):
16:59:33   Cannot find source file:
16:59:33 
16:59:33     fpclassify.c
16:59:33 
16:59:33   Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
16:59:33   .hxx .in .txx

I don't know. I already added fpclassify to lib/CMakeLists.txt:24, in exactly the same way all other *.c files are added as well. It is also located in the same directory.
I could try if adding a prototype for fpclassify will help, but I don't think so. Still I don't have any other idea what might be missing here. I don't know enough detail how the check build works.

Do you know what has to be done?

If you want to keep trying, you could put the initializations in the code which sets up the tests to be run, such as in check_check_sub.c's make_sub_suite(). Alternatively, you could have a getter function in that file which returns the appropriate value of INFINITY or NAN and the value is just computed each time.

I think I found some combination of preprocessor magic and global variables (DOUBLE_ZERO) to make it work without initialization function. The DOUBLE_ZERO is actually defined in fpclassify.c, so I don't understand how fpclassify.c could be missing, since all the AppVeyor and Travis CI builds work now, and almost all tests - there would be a link error if the DOUBLE_ZERO symbol is missing.

All tests work now, except for the one in Visual Studio 2008. It seems the build is finally working, but some unit tests are failing: log starts here
I don't know the tests well enough to see at the first sight what's going on. Do you want to have a look?
Or should we just drop VS2008 for the moment - in the end I don't really need anything older than VS2010, I just handled VS2008 because "it was there" on AppVeyor.

bel2125 · 2017-08-31T17:23:15Z

I've removed the Visual Studio 2008 build and unit test - now all automatic CI test run, including "default".

While the Visual Studio 2008 build works, but the unit test fails - maybe here, or maybe this is intended ... I don't know enough how these test logs should look like
Maybe this is worth looking at, but it's not related to #125 (Visual Studio 2010 support).

Do you think this can me merged now?

brarcher · 2017-09-01T01:35:35Z

lib/fpclassify.c

+/* Replacement function for isnan/isinf/isfinite for processors using
+ * IEEE 754 floating point format.
+ * Copyright (C) 2017, bel2125
+ * License: LGPL v2 or MIT (as you like).


Is your intention to provide this file with a different license than that of Check? Check uses LGPLv2.1. I do not think that there is a functional difference between v2 and v2.1, except that the license's name changed in v2.1.

Do you mind if this file has the license which is mentioned in the other files, namely:

/* * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public * License as published by the Free Software Foundation; either * version 2.1 of the License, or (at your option) any later version. * * This library is distributed in the hope that it will be useful, * but WITHOUT ANY WARRANTY; without even the implied warranty of * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU * Lesser General Public License for more details. * * You should have received a copy of the GNU Lesser General Public * License along with this library; if not, write to the * Free Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, * MA 02110-1301, USA. */

brarcher · 2017-09-01T01:40:22Z

lib/fpclassify.c

@@ -0,0 +1,48 @@
+/* Replacement function for isnan/isinf/isfinite for processors using
+ * IEEE 754 floating point format.
+ * Copyright (C) 2017, bel2125


This caught me off guard, as I've not received a contribution which includes its own copyright. However, looking up what this implies it does not really change anything. Because you are not transferring the copyright to another legal entity or person you retain the copyright of your committed changes regardless of adding a header.

You are welcome to include the copyright header, but I'm not sure that it is needed, as the copyright still stays with you. Do you see copyright headers from individual contributors in other open source projects?

brarcher · 2017-09-01T01:48:02Z

The changes themselves look fine and are ready to merge. I've just a question about the license of the fpclasify.c file.

In addition, you are welcome to add an entry for yourself in the AUTHORS file.

bel2125 · 2017-09-01T07:06:45Z

If I write open source software, I usually do it with the MIT license. A main difference between MIT license and LGPL is, somewhat simplified: for LGPL, if you modify the source, you must publish this source or provide it in some way for your users. for MIT, you still may do this, but you don't have to.

My brief "LGPL v2 or MIT (as you like)" just means, you can choose your license. Since MIT has less restrictions, you could use MIT licensed files in GPL/LGPL projects - you can not use GPL files in MIT projects (for LGPL you can, if you make them a library). My intention was like "if you use it within check, use it as LGPL" and "if you use only this file, use it as MIT (or LGPL)".

Maybe a more clear way to do this would be:

I create a separate repository with this file with MIT license,
and the copy within check should stay with a LGPL license
This way, you have only LGPL code in check, and someone who needs a MIT license can take it from my separate repository.

bel2125 · 2017-09-01T08:52:52Z

I set the fpclassify.c here in check to LGPL, so it has the same license as the rest of the code.
I'll keep an MIT version here https://github.com/bel2125/floatmagic

So you have a "LGPL only" code in check, and someone who needs/prefers MIT can have it there.

brarcher · 2017-09-01T10:39:45Z

Thanks for the changeset! I've merged the pull request in. The addition of the Visual Studio tests on AppVeyor should prevent a change from coming in that breaks one of those compilers.

bel2125 added 12 commits August 25, 2017 20:29

Remove stdbool.h dependency from unit test

2df73ec

Use #if defined(_MSC_VER) instead of #if _MSC_VER

1dd497b

Add various Visual Studio versions to AppVeyor CI

5eed08f

Adapt unit tests to C89/C90 by moving variable declarations to top of…

5c7b771

… functions

Use regex_t only if ENABLE_REGEX is defined

2447c87

Fix copy paste error

77f78be

Unit test: INFINITY and NAN require math-h

20f4806

Generate NAN and INFINITY in case they are not defined by the system …

2b406f1

…headers

Add definitions for isnan, isinf and isfinite, in case they are missing

af96d7f

Merge branch 'master' into master

2f16e5b

AppVeyor unit test: Add all Visual Studio builds that are supported (…

6833a3e

…and maybe drop some outdated ones later)

bel2125 mentioned this pull request Aug 28, 2017

Check for Visual Studio 2010 - without stdbool.h #125

Closed

Specify AppVeyor image for unit tests using Visual Studio 2017

74c8f06

Unit test: Remove builds not supported by AppVeyor

fe95553

brarcher reviewed Aug 29, 2017

View reviewed changes

Move infinity/not-a-number handling to lib directory

37a1188

bel2125 added 2 commits August 30, 2017 00:07

Test additional AppVeyor builds

11e74f2

Test additional AppVeyor builds

f898fd6

bel2125 added 9 commits August 30, 2017 00:45

Test additional AppVeyor builds

66943a1

Define INFINITY and NAN as macro

babc7c3

Test additional AppVeyor builds

7e8f7b4

Some preprocessor magic for defining INFINITY and NAN

8beed15

AppVeyor build for ARM CPUs can not be tested on x86/x64 build machines

5f0591a

appveyor.yml: replace exit in test step by additional environment var…

6822866

…iable

Clean up and document the appveyor.yml file

bacd3db

Try another command line for VS2008

dc31f72

Try yet another command line for VS2008

8835f52

Add prototype for fpclassify

9e86b62

Add fpclassify to Makefile.am and comment VS2008 build

ed0a9c9

brarcher reviewed Sep 1, 2017

View reviewed changes

Set license statement

a1e98d6

brarcher merged commit 77e445d into libcheck:master Sep 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Visual Studio 2010 support #129

Visual Studio 2010 support #129

bel2125 commented Aug 28, 2017

bel2125 commented Aug 28, 2017 •

edited

Loading

bel2125 commented Aug 28, 2017

bel2125 commented Aug 28, 2017

brarcher commented Aug 29, 2017

brarcher commented Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

brarcher Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

brarcher Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

brarcher Aug 29, 2017

brarcher Aug 29, 2017

bel2125 Aug 29, 2017

bel2125 commented Aug 29, 2017 •

edited

Loading

bel2125 commented Aug 29, 2017

brarcher commented Aug 31, 2017

brarcher commented Aug 31, 2017

bel2125 commented Aug 31, 2017

bel2125 commented Aug 31, 2017

brarcher Sep 1, 2017

brarcher Sep 1, 2017

brarcher Sep 1, 2017

brarcher commented Sep 1, 2017

bel2125 commented Sep 1, 2017

bel2125 commented Sep 1, 2017

brarcher commented Sep 1, 2017

Visual Studio 2010 support #129

Visual Studio 2010 support #129

Conversation

bel2125 commented Aug 28, 2017

bel2125 commented Aug 28, 2017 • edited Loading

bel2125 commented Aug 28, 2017

bel2125 commented Aug 28, 2017

brarcher commented Aug 29, 2017

brarcher commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bel2125 commented Aug 29, 2017 • edited Loading

bel2125 commented Aug 29, 2017

brarcher commented Aug 31, 2017

brarcher commented Aug 31, 2017

bel2125 commented Aug 31, 2017

bel2125 commented Aug 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brarcher commented Sep 1, 2017

bel2125 commented Sep 1, 2017

bel2125 commented Sep 1, 2017

brarcher commented Sep 1, 2017

bel2125 commented Aug 28, 2017 •

edited

Loading

bel2125 commented Aug 29, 2017 •

edited

Loading