Just gtest. #28

codecurve · 2015-02-18T02:00:20Z

Originally was pull request cellml/libcellml#26, but due to not being able to change the target branch of a pull request, had to close and restart (since original erroneously targeted master).
The focus here is on getting Google Test into the repo, so that it does not clutter future pull requests which focus on actual use cases.

Avoid confusion about primary repo when forking

Also, use MarkDown for hyperlink.

@codecurve

Merging pull request from Randall ( @codecurve ) to clarify the ReadMe to avoid possible confusion on the prime repository.

codecurve · 2015-02-18T02:03:18Z

Just to be clear: review comments on cellml/libcellml#26 still apply to this pull request, but please continue any further discussion on this pull request.

(Created as follows: "doxygen -g")

These are based on settings use on December's work, as well as ones that were on Hugh Sorby's pull request.

This is to show that the test actually does test something.

This reverts commit bd4a32c.

All lowercase, "_" separates words. See cellml/libcellml#30

Was in Model.h in previous code, lost since rollback.

codecurve · 2015-02-24T03:02:43Z

Review progress

Done (or otherwise addressed):

This pull request should be made against the develop branch of cellml/libcellml
src file names should be all lowercase
header guards missing from LibcellmlConfig.h.in
version number variables should be all uppercase (Libcellml_VERSION_MAJOR)
version test doesn’t test anything
#include should be the first include in Version.cpp
patch version missing from version number

The following are not done yet. I'd propose we add all but the first one of these as a separate issue ("cmake improvements"), and defer it till later, so that we can focus on domain specific issues for now.

api header files put into libcellml directory so that all api includes are the same for internal calls (please clarify what is meant here)
refactor CMakeLists.txt layout put CMakeLists.txt in src directory for the library target put CMakeLists.txt in tests directory for the test targets, leaving the main CMakeLists.txt for handling general options, tests, build type etc.
not able to set type of library to build [shared, static]
library target should be named cellml, not add_library(Libcellml
test option should be LIBCELLML_TESTS with a test for TESTS variable, all configurable options should be namespaced with LIBCELLML_ for gui users
can’t set -g -Wall to CMAKE_CXX_FLAGS this is not generic to all compilers
check for cmake c++ compiler support is not well thought through, if there is a problem with the compiler it should be an error

nickerso · 2015-02-24T04:14:49Z

I don't think it is acceptable to delay addressing outstanding issues in the review. They all seem like either critical issues to address in the proposed code or trivial changes that should be quick to fix.

nickerso · 2015-02-24T04:17:02Z

Further review comment: I'd expect the documentation of the getVersion() method to include a description of the returned string.

hsorby · 2015-02-24T07:56:48Z

Clarification: in the src directory create a directory "libcellml" into which you put API header files

nickerso · 2015-02-24T10:09:11Z

Further to @hsorby's comment regarding "-g" in the CXX flags, it is bad to force debug build (and even worse to do so in a manner that will not work across all target compiler toolchains) - should make use of standard CMake build types (Debug, Release, etc) which will take care of such flags in a way that is safe across compiler toolchains.

Test target only generated if `enable_testing()` is in top-level file.

Also for Build dir, and only where appropriate.

codecurve · 2015-03-03T00:34:36Z

"check for cmake c++ compiler support is not well thought through, if there is a problem with the compiler it should be an error" is resolved if we use cmake 3.2 (currently at rc2 stage). See https://github.com/cellml/libcellml/blob/ab70fbb3798597fde0c23cad39c5df16df0cfefc/CMakeLists.txt

hsorby · 2015-03-03T00:52:39Z

I would probably use this form for setting a targets properties:

set_target_properties(libcellmlTest PROPERTIES CXX_STANDARD 11 CXX_STANDARD_REQUIRED ON)

I would prefer to stick with CMake 3.1 if possible. As demonstrated the above works on my GNU/Linux with CMake 3.1

codecurve · 2015-03-03T01:20:36Z

BuildBot tests currently failing on Windows and Mac slaves, passing on Linux. The fix is to use CMake 3.2. Once CMake 3.2 is released (should be soon, since it is at release candidate 2 at the time of writing this), we will install it on the build slaves, and update the minimum version requirement to 3.2. Leaving minimum version requirement at 3.1 for now so that it still builds on Linux. See http://www.cmake.org/Bug/view.php?id=15355

This was discussed with @hsorby a few minutes ago.

hsorby · 2015-03-03T19:57:32Z

the test for the version should test the expected value vs. the actual value, not actual vs. actual
still have the tests option, this should be removed
LIBCELLML_TESTS variable should default to ON
should be able to set build type similarly to how tests is set

CodeSynthesis XSD is an XML bindings generator. This is the library to which generated bindings code will make calls. See http://www.codesynthesis.com/products/xsd/

@hsorby

@hsorby finished off the remaining cmake task (still to be checked?) and added Apache 2.0 license.

…n when CMake version 3.2 is available.

Commenting out setting of cxx standard to cxx-11, intending to add it back later.

…ult set by CMake.

Merge CodeSynthesis XSD library (take 2)

Changes required to get the tests passing on Buildbot

nickerso · 2015-03-04T00:34:39Z

That

Merge branch 'upstream_master' into gtest01

should not be here.

nickerso · 2015-03-04T10:23:33Z

With the mix up in merging #33 and reverting (#35) and its subsequent merge into this pull request, I wonder if it might not be significantly less hassle to simply start this one over? Is cherry picking 7ea7aab and starting from there an option? That seems to be the one with everything in a state which is working and presumably answering most issues that have arisen in this review...

agarny · 2015-03-04T10:37:42Z

I haven't followed things very closely, but it certainly looks like a mess... :)

Randall Britten and others added 9 commits September 10, 2014 11:30

Avoid misleading ReadMe in forks.

51d7cf2

URL to main repo.

8624e63

Merge pull request #1 from codecurve/rewordReadme01

8b1880d

Avoid confusion about primary repo when forking

Was using RST for URL instead of MarkDown.

a1f09f5

Highlight that current repo maybe fork

5737f0e

Also, use MarkDown for hyperlink.

Roll back to just plain gtest.

1850ccb

Merge pull request #18 from codecurve/rewordReadme01

f61347e

Merging pull request from Randall ( @codecurve ) to clarify the ReadMe to avoid possible confusion on the prime repository.

Update heading to match the name of the project

8181fbb

Add link to the documentation.

606ced1

codecurve mentioned this pull request Feb 18, 2015

Roll back to just plain gtest. #26

Closed

hsorby and others added 14 commits February 21, 2015 18:40

Adding doxygen config file.

ffd021c

Doxygen generated config file.

03d7145

(Created as follows: "doxygen -g")

Adjust Doxygen settings.

1ab1782

These are based on settings use on December's work, as well as ones that were on Hugh Sorby's pull request.

Doxygen project "brief".

2abe32e

Include guards

878c0d3

Break version implementation intentionally.

bd4a32c

This is to show that the test actually does test something.

Revert "Break version implementation intentionally."

4b69cec

This reverts commit bd4a32c.

Use correct case convention for cmake variables.

a20eace

Changed filename case convention.

b3f7ada

All lowercase, "_" separates words. See cellml/libcellml#30

Reinstate namespace dox.

dbd46ca

Was in Model.h in previous code, lost since rollback.

Dox: Enable auto brief.

349d06c

Fixed: BuildBot filenames had machine specific paths.

9959b10

Moved corresponding #include to top of #includes

fcc2f26

Version patch level number added.

1b452d9

Randall Britten added 5 commits February 27, 2015 16:14

Revive "test" makefile target.

af0b435

Test target only generated if `enable_testing()` is in top-level file.

Changed PROJECT to CMAKE_CURRENT, for source dir.

01261d4

Also for Build dir, and only where appropriate.

Removed "if": inclusion is already conditional.

d52a929

Added dependency on config header.

55c5601

Renamed so that binary not called libLibcellml*

a86829f

Changed CMake test for C++11 support.

33cea29

hsorby added 3 commits March 4, 2015 09:43

Merge remote-tracking branch 'coworker/gtest01' into gtest01

2e2b257

Finish off some CMake bits and pieces.

ccd7a4f

Add Apache license 2.0

33a4d4f

codecurve mentioned this pull request Mar 3, 2015

CodeSynthesis XSD library 01 #32

Closed

Randall Britten added 2 commits March 4, 2015 11:16

Library for CodeSynthesis XSD v4.0.0.

a1124a0

CodeSynthesis XSD is an XML bindings generator. This is the library to which generated bindings code will make calls. See http://www.codesynthesis.com/products/xsd/

Merge pull request #5 from hsorby/gtest01

9ce9a24

@hsorby finished off the remaining cmake task (still to be checked?) and added Apache 2.0 license.

codecurve mentioned this pull request Mar 3, 2015

CodeSynthesis XSD library (take 2) #33

Merged

hsorby and others added 7 commits March 4, 2015 11:48

Commenting out setting of cxx standard to cxx-11. Can be added back i…

f11f029

…n when CMake version 3.2 is available.

Merge pull request #6 from hsorby/gtest01

06f2701

Commenting out setting of cxx standard to cxx-11, intending to add it back later.

Set the default of the gtest_force_shared_crt to ON to match the defa…

d28fbf1

…ult set by CMake.

Having to work around bug 15355 in CMake

7ea7aab

Merge pull request #33 from codecurve/code-synthesis-xsd-lib02

2803ced

Merge CodeSynthesis XSD library (take 2)

Merge pull request #7 from hsorby/gtest01

a945fb3

Changes required to get the tests passing on Buildbot

Merge branch 'upstream_master' into gtest01

ead848a

nickerso closed this Mar 4, 2015

codecurve mentioned this pull request Mar 4, 2015

Just gtest (take 3) #39

Closed

nickerso mentioned this pull request Mar 5, 2015

Initial code stub #38

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Just gtest. #28

Just gtest. #28

codecurve commented Feb 18, 2015

codecurve commented Feb 18, 2015

codecurve commented Feb 24, 2015

nickerso commented Feb 24, 2015

nickerso commented Feb 24, 2015

hsorby commented Feb 24, 2015

nickerso commented Feb 24, 2015

codecurve commented Mar 3, 2015

hsorby commented Mar 3, 2015

codecurve commented Mar 3, 2015

hsorby commented Mar 3, 2015

nickerso commented Mar 4, 2015

nickerso commented Mar 4, 2015

agarny commented Mar 4, 2015

Just gtest. #28

Just gtest. #28

Conversation

codecurve commented Feb 18, 2015

codecurve commented Feb 18, 2015

codecurve commented Feb 24, 2015

nickerso commented Feb 24, 2015

nickerso commented Feb 24, 2015

hsorby commented Feb 24, 2015

nickerso commented Feb 24, 2015

codecurve commented Mar 3, 2015

hsorby commented Mar 3, 2015

codecurve commented Mar 3, 2015

hsorby commented Mar 3, 2015

nickerso commented Mar 4, 2015

nickerso commented Mar 4, 2015

agarny commented Mar 4, 2015