rdbisme commented 5 years ago

This MR basically improves the CMakeLists.txt in order to:

Export a mutation++Config.cmake file that allows other projects to find mutation just doing find_package(mutation++) without the need of providing a custom FindMutation++.cmake module. Check the examples/c++/<dir> to watch it in action
Modernizing the linking of dependencies using the target-based deps transitivity (vs the legacy way of adding STUFF_INCLUDE_DIRS and STUFF_LIBRARIES), ref: https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/
Re-arranging includes to be coherent with the standard ways:
- #include <catch2/catch.hpp>
- #include <Eigen/Dense>
Employing Semantic versioning (http://semver.org)

I wanted also to provide auto-download of the most modern revision of Eigen3 and catch2, but with the new version of catch2 the Approx method has been revamped and in particular the check against 0 (e.g. a == Approx(0)) needs to be modified in the source code.

This was an old PR on the Gitlab repository, I rebased on the current master.

Let me know what do you think.

rdbisme commented 5 years ago

Also:

examples now are compiled with cmake and not Makefile and they can be compiled individually or in the main building process. In addition, the only thing needed to add a new example is to create a new folder in examples, symlink the CMakeLists.shared file, done!

Note: The example smb is obsolete. So I gzipped it to avoid that cmake automatically adds it and tests it but I think it should be removed from the repository (or updated)

codecov-io commented 5 years ago

Codecov Report

Merging #67 into master will decrease coverage by 0.16%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #67      +/-   ##
==========================================
- Coverage   72.98%   72.82%   -0.17%     
==========================================
  Files         244      134     -110     
  Lines       12009     8559    -3450     
==========================================
- Hits         8765     6233    -2532     
+ Misses       3244     2326     -918

Impacted Files	Coverage Δ
src/gsi/GSIRateLaw.h	`83.33% <ø> (+83.33%)`	:arrow_up:
src/gsi/SurfaceChemistry.h	`100% <ø> (ø)`	:arrow_up:
src/transport/CollisionGroup.h	`100% <ø> (ø)`	:arrow_up:
src/utilities/Units.h	`82.35% <ø> (ø)`	:arrow_up:
src/transport/Wilke.h	`100% <ø> (+7.14%)`	:arrow_up:
src/transport/DiffusionMatrix.h	`83.33% <ø> (+83.33%)`	:arrow_up:
src/transport/CoulombIntegrals.cpp	`97.27% <ø> (+0.02%)`	:arrow_up:
src/thermo/MultiPhaseEquilSolver.h	`59.32% <ø> (+59.32%)`	:arrow_up:
src/transport/ExactDiffMat.cpp	`85.71% <ø> (ø)`	:arrow_up:
src/general/Mixture.cpp	`36.36% <ø> (-7.39%)`	:arrow_down:
... and 214 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 2e1e71a...33d8b4b. Read the comment docs.

jbscoggi commented 5 years ago

Hey @rubendibattista, this is great but we need to get the tests passing before we can merge. I tried to figure out what's going on but I think you should look at it.

rdbisme commented 5 years ago

Sure, yesterday I was tired. I will look at it today and get back to you!

On Thu, 25 Apr 2019, 04:41 J.B. Scoggins, notifications@github.com wrote:

@jbscoggi requested changes on this pull request.

This is a very nice PR and really helps to cleanup the current build/install setup. There are a few minor changes/comments to take a look at. In addition, I would like to see the relevant documentation updated in docs to reflect these changes before we merge. In particular, I'm thinking of dependencies.md and docs/installation.md. Of course, the tests must also pass before merging.

In tests/test_capitelli_integrals.cpp https://github.com/mutationpp/Mutationpp/pull/67#discussion_r278377415:

@@ -0,0 +1,187 @@ +/*

Why did you add this file? Everything here is in test_collision_integrals.cpp I think.

In CMakeLists.txt https://github.com/mutationpp/Mutationpp/pull/67#discussion_r278377796:

Install prefix settings

-################################################################################ -SET (CMAKE_INSTALL_PREFIX "../install" CACHE STRING

"Install path prefix, prepended onto install directories." FORCE) -MARK_AS_ADVANCED (CMAKE_INSTALL_PREFIX) +############################################################################### +set(CMAKE_INSTALL_PREFIX "/usr/local" CACHE PATH

"Install path prefix, prepended onto install directories.")

+if(NOT IS_ABSOLUTE ${CMAKE_INSTALL_PREFIX})

I'm not sure I understand why this is important, can you provide your reasoning?

In examples/fortran/wrapper_test/wrapper_test.f90 https://github.com/mutationpp/Mutationpp/pull/67#discussion_r278379539:

@@ -36,7 +36,7 @@ program main real(8), dimension(:), allocatable :: species_x, wdot, species_y real(8), dimension(:), allocatable :: mwi

mixture = "air_11"

This should not be changed. air_11 is the correct name.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mutationpp/Mutationpp/pull/67#pullrequestreview-230428550, or mute the thread https://github.com/notifications/unsubscribe-auth/AB5UANGMLUQWNCGZIN2LQMTPSEK4ZANCNFSM4HIGTRQA .

rdbisme commented 5 years ago

@jbscoggi Tests should be passing and I updated installation.md. For the dependency stuff, in another project I wrote a cmake script that automatically pulls stuff from Github and builds it using cmake, acquiring the project using ExternalProject. Here the file.

I need to polish it and I will open another PR for it once it is ok. That means it will download the most recent github release of each dependency and build it automatically without needing Python and the install_dependencies.py script. It works ok for catch2, need to test it with Eigen. But take care, the new version of catch2 messes up with mutation++ tests.

rdbisme commented 5 years ago

PS: Once everything passes, do you want me to squash all the commits in one?

rdbisme commented 5 years ago

Am I missing a test? Why the coverage dropped so much?

jbscoggi commented 5 years ago

Hey @rubendibattista, it seems there are far fewer tests being run now for some reason. There should be 50 tests but only 10 are being run with your modifications. That is why the code coverage is so much less. Any idea what's going on?

rdbisme commented 5 years ago

Hey @rubendibattista, it seems there are far fewer tests being run now for some reason. There should be 50 tests but only 10 are being run with your modifications. That is why the code coverage is so much less. Any idea what's going on?

The ParseAndAddCatchTests wanted the TEST_CASE macro to be followed on the same line by a parethesis. Dunno why...

But I'm getting SIGSEV for some tests on my machine. Let's see what Travis says...

rdbisme commented 5 years ago

Another thing to note is that the tests on Travis, even if configured with a different compiler, are compiled with GCC 4.8. :/ (and I checked also the tests previous to my modification...)

rdbisme commented 5 years ago

Ok, now coverage is good. There's few more stuff to address:

[x] Refactor the targets export following this: (https://youtu.be/bsXLMQ6WgIk?t=50m30s) [In particular the things about the CmakeFindDependencyMacro stuff because I'm hitting it with Eigen3::Eigen target
[x] Fix travis.yml compiler options for gcc

rdbisme commented 5 years ago

We should be good now. Compilers in travis are correctly used and we correctly export the mutation++Config.cmake file.

As you finish merging this, I'll ask you to tag a 0.3.0 release (I used the 0.2.0 to build a package for EasyBuild... So if it is not much trouble to jump it :)). This way, in my spare time, I'll try to write down package definitions for EasyBuild and spack.

rdbisme commented 5 years ago

Oh, also tell me if you want me to squash all the commits in one.

jbscoggi commented 5 years ago

Yea I think it's best to squash the commits. Normally we don't ask that but probably we should...

rdbisme commented 5 years ago

Ok. As soon as all the tests finish I'll squash everything in one single commit.

jbscoggi commented 5 years ago

Seems there is still a problem, now there are only 30 tests being run. Better than 10, but still not 50. The danger in changing the test name parsing is that you miss some tests. I think it's best to go back to how they were parsed before because that was definitely working...

Alternatively, we could specify the tests which are run, but that just means people adding tests have to remember to add the test name to a file somewhere.

rdbisme commented 5 years ago

I think the "test counting" changed how it's counting them. But I think all of them are being tested. If you run locally the catch main file run_tests, you can check the number of assertions and compare w.r.t. before.

Moreover the coverage is the same as before, so I think that all the tests are being runned. I think only the way they're "considered" changed...

Could you check that?

PS: Even with the previous parsing script, I was getting only few tests tested (and moreover I could not turn on the verbose mode to exactly see what it is parsing). If you check the verbose parsing output, all the source files should be correctly parsed and analyzed.

rdbisme commented 5 years ago

Maybe I know the problem. Maybe there're multiple test cases in a single file. I just fixed the first one. My bad. I'll fix this.

rdbisme commented 5 years ago

Ok, it's fixed. 47 tests. The 3 left are the compile tests you added for the examples that now are automatically tested during the build without express need of adding them. The run of the examples is correctly handled using ctest -j2.

jbscoggi commented 5 years ago

Hey @rubendibattista, still a few problems... :). It seems that only the release build is being tested, not sure why it doesn't run the debug build. Also AFAICT, only clang 5.0.0 and gcc 4.8.4 are being used, regardless of how it is supposed to be setup.

Another thing I have noticed (this was a problem before your PR) is that the tests themselves, and the third-party libraries are included in the coverage analysis. Obviously, it would make more sense to only do coverage analysis for examples and src. If you don't mind looking into that as part of this PR, that would be much appreciated. I tried to figure it out a while back but gave up. Maybe your CMake/gcov muscles are bigger... :) If you don't want to deal with that, no worries.

rdbisme commented 5 years ago

Wait, If I check the output of $ cmake -H. -Bbuild-release -DCMAKE_BUILD_TYPE=Release -DENABLE_TESTING=1 -DCMAKE_INSTALL_PREFIX=$MPP_DIRECTORY/install

I get this for gcc-6:

-- The CXX compiler identification is GNU 6.5.0
-- Check for working CXX compiler: /usr/bin/g++-6
-- Check for working CXX compiler: /usr/bin/g++-6 -- works
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Searching for Eigen3...
-- Found Eigen3: /home/travis/build/mutationpp/Mutationpp/thirdparty/eigen  
-- Searching for Catch2...
-- Found Catch2: /home/travis/build/mutationpp/Mutationpp/thirdparty/catch/include  
-- ParseAndAddCatchTests: Started parsing run_tests
-- ParseAndAddCatchTests: Found the following sources:

You need to check the compiler after the cmake command, not before. That command is run by travis I don't know why. (I really like better Gitlab CI!). So the compiler should be correct.

Also, the coverage should be only on the library part, so only on src. And I should have taken care of that. If you check the make coverage output, at the end:

Filename                                       |Rate    Num|Rate  Num|Rate   Num
================================================================================
[/home/travis/build/mutationpp/Mutationpp/src/]
general/Errors.h                               |94.4%   124|41.9% 241|    -    0
general/GlobalOptions.h                        | 100%    22| 100%   9|    -    0
general/Mixture.cpp                            |40.4%    47|80.0%   5|    -    0
general/Mixture.h                              | 100%     4| 100%   2|    -    0
general/MixtureOptions.cpp                     |76.4%    89|84.6%  13|    -    0
general/MixtureOptions.h                       | 100%    35| 100%  16|    -    0
gsi/DiffusionVelocityCalculator.cpp            |77.8%    27| 100%   6|    -    0
gsi/GSIRateLaw.h                               |83.3%     6|50.0%   4|    -    0
gsi/GSIRateLawGammaConst.cpp                   |96.7%    60| 100%   9|    -    0
gsi/GSIRateLawGammaT.cpp                       | 100%    21| 100%   6|    -    0
gsi/GSIRateLawSublimation.cpp                  | 100%    24| 100%   6|    -    0
gsi/GSIRateManager.h                           |83.3%     6|50.0%   4|    -    0
gsi/GSIRateManagerPhenomenological.cpp         |78.4%    37|87.5%   8|    -    0
gsi/GSIReaction.h                              |86.2%    29|80.0%  10|    -    0
gsi/GSIReactionAblation.cpp                    |88.9%    99| 100%   6|    -    0
gsi/GSIReactionCatalysis.cpp                   |88.9%    81| 100%   6|    -    0
gsi/GSIStoichiometryManager.cpp                |58.3%    24|47.8%  23|    -    0
gsi/GSIStoichiometryManager.h                  |50.0%    42|37.5%  24|    -    0
gsi/GasFourierHeatFluxCalculator.cpp           |69.0%    29| 100%   6|    -    0
gsi/GasSurfaceInteraction.cpp                  |81.6%    87|82.4%  17|    -    0
gsi/MassBlowingRate.h                          |66.7%     3|50.0%   4|    -    0
gsi/MassBlowingRateAblation.cpp                | 100%    12| 100%   7|    -    0
gsi/MassBlowingRateNull.cpp                    |28.6%     7|28.6%   7|    -    0
gsi/SolidProperties.h                          |80.0%     5|80.0%   5|    -    0
gsi/SolidPropertiesNull.cpp                    | 100%     3| 100%   3|    -    0
gsi/SolidPropertiesSteadyState.cpp             | 100%    11| 100%   5|    -    0
gsi/Surface.h                                  | 6.5%    31|18.2%  11|    -    0
gsi/SurfaceBalanceSolverMass.cpp               |90.0%   130|95.2%  21|    -    0
gsi/SurfaceBalanceSolverMassEnergy.cpp         |89.9%   179|87.0%  23|    -    0
gsi/SurfaceChemistry.cpp                       |89.7%    29|85.7%   7|    -    0
gsi/SurfaceChemistry.h                         | 100%     3| 100%   1|    -    0
gsi/SurfaceProperties.h                        |25.0%     8|28.6%   7|    -    0
gsi/SurfacePropertiesAblation.cpp              |77.8%    54|88.9%   9|    -    0
gsi/SurfacePropertiesNull.cpp                  | 100%     4| 100%   5|    -    0
gsi/SurfaceRadiation.cpp                       | 100%    15| 100%   5|    -    0
gsi/SurfaceRadiation.h                         | 0.0%     3| 0.0%   1|    -    0
gsi/SurfaceState.cpp                           |81.5%    54| 100%   9|    -    0
gsi/SurfaceState.h                             | 100%     7| 100%   5|    -    0
kinetics/JacobianManager.cpp                   |44.1%   179| 8.4%  83|    -    0
kinetics/JacobianManager.h                     |38.8%   121|19.5% 297|    -    0
kinetics/Kinetics.cpp                          |76.2%   151|94.1%  17|    -    0
kinetics/Kinetics.h                            | 100%     4| 100%   2|    -    0
kinetics/RateLawGroup.h                        |98.4%    61|93.8%  32|    -    0
kinetics/RateLaws.cpp                          | 100%    44| 100%   6|    -    0
kinetics/RateLaws.h                            | 100%    18|91.7%  12|    -    0
kinetics/RateManager.cpp                       |96.2%    52|97.1%  34|    -    0
kinetics/RateManager.h                         | 100%     4| 100%   3|    -    0
kinetics/Reaction.cpp                          |86.9%   191|87.5%   8|    -    0
kinetics/Reaction.h                            |94.4%    36|92.9%  14|    -    0
kinetics/ReactionType.cpp                      |95.2%    42| 100%   1|    -    0
kinetics/StoichiometryManager.cpp              |81.8%    22| 100%  23|    -    0
kinetics/StoichiometryManager.h                |82.8%    58|82.6%  23|    -    0
kinetics/ThirdBodyManager.h                    | 100%    29| 100%   8|    -    0
numerics/Functors.h                            |57.9%    19|63.6%  11|    -    0
numerics/Interpolators.cpp                     | 100%     7| 100%   2|    -    0
numerics/Interpolators.h                       |96.5%    86|45.5%  33|    -    0
numerics/NewtonSolver.h                        |82.5%    40|85.7%  14|    -    0
numerics/lp.h                                  |81.8%   121| 100%   4|    -    0
thermo/ChemNonEqStateModel.cpp                 |68.8%    80|86.7%  15|    -    0
thermo/ChemNonEqTTvStateModel.cpp              |72.3%   141|85.7%  14|    -    0
thermo/Composition.cpp                         |62.7%    67|75.0%   8|    -    0
thermo/Composition.h                           | 100%    12| 100%  11|    -    0
thermo/EquilStateModel.cpp                     |33.6%   128|66.7%  15|    -    0
thermo/MultiPhaseEquilSolver.cpp               |62.2%   802|66.7%  39|    -    0
thermo/MultiPhaseEquilSolver.h                 |58.5%    65|66.7%  21|    -    0
thermo/Nasa7DB.cpp                             | 100%    64| 100%   9|    -    0
thermo/Nasa7Polynomial.cpp                     |63.4%   101|60.0%  10|    -    0
thermo/Nasa7Polynomial.h                       |78.9%    19|55.6%   9|    -    0
thermo/Nasa9DB.cpp                             |97.4%    78|84.6%  13|    -    0
thermo/Nasa9Polynomial.cpp                     |69.1%   162|71.4%  14|    -    0
thermo/Nasa9Polynomial.h                       | 0.0%     4| 0.0%   2|    -    0
thermo/NasaDB.h                                |79.5%    73|65.0%  20|    -    0
thermo/ParticleRRHO.cpp                        |97.6%    42| 100%   4|    -    0
thermo/ParticleRRHO.h                          | 100%    17| 100%  10|    -    0
thermo/RrhoDB.cpp                              |82.5%   309|84.8%  46|    -    0
thermo/Species.cpp                             |92.9%   113| 100%  11|    -    0
thermo/Species.h                               | 100%    48| 100%  26|    -    0
thermo/SpeciesListDescriptor.cpp               |86.6%   142| 100%  19|    -    0
thermo/SpeciesListDescriptor.h                 | 100%     1| 100%   1|    -    0
thermo/SpeciesNameFSM.cpp                      |97.6%    83| 100%   8|    -    0
thermo/SpeciesNameFSM.h                        | 100%     3| 100%   2|    -    0
thermo/StateModel.h                            |74.3%   101|57.7%  26|    -    0
thermo/ThermoDB.cpp                            |57.7%    71|45.5%  11|    -    0
thermo/ThermoDB.h                              |75.0%    12|62.5%   8|    -    0
thermo/Thermodynamics.cpp                      |37.0%   511|53.8%  80|    -    0
thermo/Thermodynamics.h                        |86.5%   111|91.7%  36|    -    0
transfer/MillikanWhite.cpp                     |93.1%    72| 100%   6|    -    0
transfer/MillikanWhite.h                       | 100%    25| 100%  12|    -    0
transfer/OmegaCE.cpp                           | 100%    14| 100%   6|    -    0
transfer/OmegaCElec.cpp                        | 100%    19| 100%   6|    -    0
transfer/OmegaCV.cpp                           |89.3%    28| 100%   7|    -    0
transfer/OmegaET.cpp                           | 100%    21| 100%   6|    -    0
transfer/OmegaI.cpp                            | 100%    36| 100%   6|    -    0
transfer/OmegaVT.cpp                           |98.1%    53| 100%   9|    -    0
transfer/TransferModel.h                       |80.0%     5|50.0%   4|    -    0
transport/CapitelliIntegrals.cpp               |82.2%   230|64.7%  17|    -    0
transport/CapitelliIntegrals.h                 |64.3%    14|46.7%  15|    -    0
transport/CollisionDB.cpp                      |77.9%   136|86.7%  15|    -    0
transport/CollisionDB.h                        |95.5%    44|92.0%  25|    -    0
transport/CollisionGroup.cpp                   |51.1%    47| 100%   4|    -    0
transport/CollisionGroup.h                     | 100%    13|85.7%   7|    -    0
transport/CollisionIntegral.cpp                |79.7%   316|88.1%  67|    -    0
transport/CollisionIntegral.h                  |89.5%    19|78.6%  14|    -    0
transport/CollisionPair.cpp                    |92.7%    82| 100%  10|    -    0
transport/CollisionPair.h                      | 100%     5| 100%   7|    -    0
transport/CoulombIntegrals.cpp                 |97.4%   114| 100%  13|    -    0
transport/CoulombIntegrals.h                   | 100%     3| 100%   1|    -    0
transport/DiffusionMatrix.h                    |83.3%     6|50.0%   4|    -    0
transport/ElectronSubSystem.cpp                |15.7%   121|26.3%  19|    -    0
transport/ElectronSubSystem.h                  |21.3%   150|18.0%  50|    -    0
transport/ExactDiffMat.cpp                     |86.0%    43| 100%   6|    -    0
transport/GuptaYos.h                           | 100%    15| 100%   3|    -    0
transport/LangevinIntegrals.cpp                |96.7%    30|88.9%   9|    -    0
transport/RamshawDiffMat.cpp                   | 100%    15| 100%   6|    -    0
transport/ThermalConductivityAlgorithm.h       |44.4%     9|40.0%   5|    -    0
transport/ThermalConductivityChapmannEnskog.cpp| 100%    60|93.8%  16|    -    0
transport/ThermalConductivityWilke.cpp         | 100%    13| 100%   6|    -    0
transport/Transport.cpp                        |36.5%   400|58.3%  36|    -    0
transport/Transport.h                          | 100%    23| 100%   5|    -    0
transport/ViscosityAlgorithm.h                 |80.0%     5|50.0%   4|    -    0
transport/ViscosityChapmannEnskog.cpp          |96.8%    31|83.3%  12|    -    0
transport/ViscosityGuptaYos.cpp                | 100%    24| 100%   6|    -    0
transport/ViscosityWilke.cpp                   | 100%    12| 100%   6|    -    0
transport/Wilke.h                              | 100%    14| 100%   3|    -    0
utilities/AutoRegistration.h                   |80.4%    46|78.0% 418|    -    0
utilities/LookupTable.h                        |78.0%   132|83.3%  12|    -    0
utilities/StringUtils.cpp                      |72.4%    58|62.5%   8|    -    0
utilities/TemporaryFile.cpp                    |84.4%    32|85.7%   7|    -    0
utilities/TemporaryFile.h                      | 100%     4| 100%   6|    -    0
utilities/Units.cpp                            |97.2%   145| 100%  17|    -    0
utilities/Units.h                              |85.0%    20|85.7%   7|    -    0
utilities/Utilities.h                          |92.3%    39| 100%   4|    -    0
utilities/XMLite.cpp                           |90.5%   169|90.9%  11|    -    0
utilities/XMLite.h                             |98.7%    76| 100%  33|    -    0
================================================================================
                                         Total:|73.5%  9084|66.3%  2k|    -    0

Now, the problem is I don't understand how codecov works. It redoes the coverage analysis by itself. I would like to understand how can I tell it to just use the .info file that is generated by lcov (where I already stripped all the unneeded things like system headers). I will investigate a bit on that (again, I prefer Gitlab approach to coverage parsing...)

I think I know why only the release is tested. I'm using YAML anchors and probably they get overridden.

rdbisme commented 5 years ago

Tracking here todos:

[x] Fix anchors override
[x] Forward coverage info file to codecov

jbscoggi commented 5 years ago

OK yes I see what your talking about for the compiler version, that seems good. Thanks for looking into the other stuff!

rdbisme commented 5 years ago

I should have fixed everything we said. :)

jbscoggi commented 5 years ago

OK everything looks great now. I will squash and merge.

mutationpp / Mutationpp

Cmake revamp #67

Codecov Report

Why did you add this file? Everything here is in test_collision_integrals.cpp I think.

Install prefix settings

I'm not sure I understand why this is important, can you provide your reasoning?