General overhaul of the build and test system #217

tmbgreaves · 2018-10-14T21:48:22Z

This is a major overhaul of the build and test system, starting with the foundations @jrper laid in setting up CMake , updating that to the current build environment, and adding similar capabilities for using system spud, Judy, and spatialindex to the GNU configure setup. With this in place and to reduce maintenance requirements on the fluidity codebase going forwards, local copies of spatialindex, Judy, and spud are removed. At the same time, local hacks for the Archer build are removed -- these were integrated into the Judy configuration, and not mirrored into CMake.

With the need to test CMake configuration and building, Jenkins needed to be modified, and rather than add to a less than optimal existing setup the Jenkins configuration is entirely reworked to give a xenial-only test, the GNU configure branch being essentially as before but adding a CMake second branch. The currently non-passing CentOS 7 tests are removed, and will be addressed in a further PR, as will bionic.

Dockerfiles for the standard Fluidity test environment are updated to reflect the new system packages, and the xenial environment is further streamlined by moving to a 'fluidity-base' package which is intended to replace fluidity-dev going forwards, in particular if and when we switch to system petsc and non-petsc-dependent zoltan. The last two are intended for a PR in the near future to complete the system package migration.

Whilst fixing the build system, a missing VTK library as per issue #216 is added.

…configure' is that 'make test' is no longer permitted, and has changed to 'make shorttest'. This is intended to be a useful second-line configure option as opposed to the main configure method, but will be tested and maintained in Jenkins.

support for system versions. This also removes FLLINKER from the build system, added for the now-obsolete Archer system build.

This commit adds bionic support, and adds system packages for Judy, spatialindex, and spud

Add in a cmake Jenkins test and rename 'make test' to 'make shorttest' which replaces the misleading 'serialtest' tag. Test packages have been merged into the fluidity-base package; this is added as the dockerfile metapackage.

tmbgreaves · 2018-10-14T21:56:29Z

One outstanding issue needs to be resolved before merging - there is a failing test on both build streams, python_diagnostic_field - giving:

Traceback (most recent call last):
File "", line 10, in
SpudKey.error: Error: The specified option is not present in the dictionary in get option
Python error, Python string was:
X=state.vector_fields["Coordinate"]

for i in range(field.node_count):
X_n=X.node_val(i)
field.set(i, X_n[0]*X_n[1])

import sys
if sys.version_info[1] >= 7:
import libspud
name = libspud.get_option("/simulation_name")
assert name == "python"
*** ERROR ***
Error message: Dying

tmbgreaves · 2018-10-14T22:17:03Z

Any suggestions for fixes on the failing test would be much appreciated - this is not my area of expertise!

To reproduce:

docker pull fluidity/baseimages:xenial
docker run -it fluidity/baseimages:xenial bash -il
git clone -b build_overhaul https://github.com/fluidityproject/fluidity.git
cd fluidity
./configure --enable-2d-adaptivity
make && make fltools
./tools/testharness.py -f python_diagnostic_field.xml

jrper · 2018-10-18T16:43:59Z

Regarding the python libspud problem, I think I understand the basic story of what's going on, but I'm not sure what the best fix is. At the moment the libspud package includes both a static archive and a shared object version of the library. For preference we link Fluidity to the shared object version, while the libspud python package uses the static version. As such, we have two different spud_manager objects, and the python libspud.get_object call fizzles into uninitialised memory.

One option is to specify the libspud.a library by absolute path when linking (I've checked by hand, that works), a second is to make the right incantation for cmake to switch the order of preference to static for that library. It might also be possible to use the shared object version with the python libspud.

jrper · 2018-10-18T16:47:03Z

The cmake stuff all looks fine to me, but I'd suggest that Stephan does the review there, since I contributed to that side.

stephankramer

The stuff to do with excising libjudy, libspatialindex and libspud all looks good to me in principle. Although I must admit I probably have missed/forgotten about the discussion leading up to this decision. On the one hand this does clean things up a fair bit in the fluidity code, which is nice. On the other hand, it leads to some extra work for people that regularly need to build things from scratch - remember we're not all on Ubuntu workstations/docker containers. This was I thought the reason we fairly recently decided on using git subtree for spud, rather than just removing it. I don't have any strong opinions either way, just want to make sure everyone is in the loop about this (e.g. @drhodrid , @cianwilson ).

The cmake stuff feels a little less ready. It doesn't actually work on systems I've tried: it can't find PETScConfig.cmake on a Xenial system with an AMCG build of petsc 3.6.3 (even if I set PETSC_DIR). But more generally PETScConfig.cmake doesn't seems to be something that's installed in 3.7 or 3.8. The CMakeLists.txt files in the various directories seem to have different approaches to how to the specify the source files, some explicitly list them, others use wildcards. I'm not really keen on adding yet another step in the already quite convoluted procedure to add new fortran modules. So in general I'm a little worried about the maintainability of it all. We will need fixes for petsc 3.7 and 3.8, but then we're already 2 versions behind (3.10 has been out for a while now), and somebody needs to spent time on figuring out how to make all that work. I think we just don't have the maintainer time to maintain two different buildsystems. So I think we need to choose one. You probably know my thoughts on cmake (which I won't repeat here out of politeness), but I do admit the "Makefile depencies" system is a right pile of *** as well. So happy to have a discussion about this. I'm keen to point out however that regardless of which buildsystem we choose, and regardless of how many systems you try it out personally, there will be an enormous amount of unforeseen cases on the systems of users. In my experience even a small change in the buildsystem, will typically lead to things breaking for a number of users. So I'm very hesitant to start from scratch as we would be when choosing cmake - apart from the abysmal debugging experience I usually have with it.

stephankramer · 2018-10-19T12:40:34Z

Makefile.in

-	cd libspud; $(MAKE) install-spudtools; cd ../..
-	cd libspud; ./configure --prefix=$(PWD); cd ../..
-
-install-user-schemata:
 	mkdir -p $(HOME)/.diamond/schemata/


This seems to now be part of install rather than the separate install-user-schemata. Is this intentional? As I'm not sure this is correct. Installing things in $(HOME) should not be part of install which should only be installing things in $(DESTDIR). In particular make install is typically run with sudo in which case $(HOME) is ambiguous or might not even be writable by root. I wonder whether we can get rid of make install-user-schemata; I have never used it personally.

Agreed - good point.

stephankramer · 2018-10-19T12:48:49Z

configure.in

@@ -1596,6 +1475,7 @@ if test "$enable_vtk" != "no" ; then
            AC_CHECK_LIB(vtkFiltering, main)
            AC_CHECK_LIB(vtkGraphics, main)
            AC_CHECK_LIB(vtkIO, main)
+            AC_CHECK_LIB(vtkIO, main)


Is this checking the same things twice?

Whoops, yes.

stephankramer · 2018-10-19T12:50:26Z

docker/Dockerfile.bionic

+RUN apt-get update
+
+# Install Fluidity development environment
+RUN echo "Europe/London" > /etc/timezone


Just curious: why was this necessary?

This is an annoying issue where, in a minimal system, when you install tzdata it needs either to interact to determine the timezone or to have a pre-defined timezone on the system. It's a little while since I looked at this but I think if you haven't pre-defined the timezone and you've specified a non-interactive install the package install just stops with an unresolveable state.

stephankramer · 2018-10-19T12:55:10Z

error_measures/tests/CMakeLists.txt

+set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin/tests)
+
+file(GLOB tests RELATIVE ${CMAKE_CURRENT_SOURCE_DIR} *.F90)
+list(REMOVE_ITEM tests test_anisotropic_adaptivity.F90 test_anisotropic_adaptivity_two test_gradation.F90 test_isotropic_gradation.F90 test_annulus_gradation.F90 test_directional_gradation.F90 test_directional_gradation_annulus.F90 test_pseudo2d_gradation.F90 test_assemble_metric.F90 test_anisotropically_bounded_metric.F90 test_backstep.F90 test_quadratic_interpolation.F90 test_recovery_estimator.F90 test_elementwise_error_adaptivity.F90 test_boundary_layer_adaptivity.F90 test_boundary_layer_adaptivity_two.F90 test_anisotropic_bounds_equivalence.F90 compute_interpolation_error_noadapt.F90 compute_interpolation_error_adapt.F90 compute_enstrophy_goal.F90 compute_temperature_goal.F90 compute_hessian_error.F90 compute_chimney_adapt.F90 compute_les_temp_goal.F90 compute_les_velocity_goal.F90 compute_mesh_conformity.F90 compute_driven_cavity_adapt.F90 test_mba compute_residual.F90 test_metric_advection.F90 test_geometric_constraints.F90 compute_compare_interpolation.F90 compute_compare_interpolation_3d.F90 test_anisotropic_gradation.F90 compute_anisotropic_gradation.F90 test_anisotropic_zz_3d.F90 test_anisotropic_zz.F90)


I presume these corresponds to DISABLED_TESTS in error_measures/tests/Makefile.in ?

stephankramer · 2018-10-19T13:00:06Z

libjudy/src/JudyHS/CMakeLists.txt

@@ -0,0 +1 @@
+set(JUDY_SRCS_JUDYHS JudyHS.c)


I suspect this one should no longer be there?

stephankramer · 2018-10-19T13:15:46Z

tests/data/Makefile

-	rm -f cube_prismatic.face cube_prismatic.ele cube_prismatic.msh cube_prismatic.node
-	rm -f matrix2.mm test_halo_io_out_0.halo test_stream_io_out
-	rm -f adapt_?.vtu bound_field_out.vtu bounding_?.vtu cg_interpolation_?.vtu cubic_interpolation_?.vtu curl_out_?.vtu cylinder_hessian_?.vtu dg_interpolation_?.vtu differentiate_field_?.vtu div_out_?.vtu empty_populate_state.vtu laplacian_grid_2.vtu layered_mesh_?.vtu linear_interpolation_?.vtu metric_based_extrusion_?.vtu pseudo_supermesh_2.vtu quadratic_interpolation_?.vtu sam_integration_?.vtu solenoidal_interpolation_*.vtu strain_rate_out_?.vtu tensor_second_invariant_out_?.vtu test_adapt_mesh_out.vtu test_adapt_state_3d_out_?.vtu test_adapt_state_unittest_out_?.vtu test_potential_vorticity_out.vtu test_relative_potential_vorticity_out.vtu test_u_dot_nabla_out_?.vtu test_vtk_precision_out.vtu vfield_adapt_?.vtu z_mesh_?.vtu
+	cd /Users/origimbo/fluidity/fluidity-brew && $(MAKE) -f CMakeFiles/Makefile2 tests/data/clean


stephankramer · 2018-10-19T13:20:42Z

tests/data/Makefile

@@ -1,6 +1,166 @@
+# CMAKE generated file: DO NOT EDIT!


Yeah, this is problematic. We can't have both a cmake generated Makefile and a committed Makefile that works regardless of configuraiton (as we had before). So perhaps we should have a tests/data/Makefile.in as well, and not commit tests/data/Makefile ?

stephankramer · 2018-10-19T13:27:13Z

CMakeLists.txt

+# Check for PETSc
+if(APPLE)
+  if(DEFINED ENV{HOMEBREW_PREFIX})
+     set(PETSC_DIR "$ENV{HOMEBREW_PREFIX}/Cellar/petsc-fluidity/3.6.3-fluidity"


Why are we hardcoding a specific petsc version here?

stephankramer · 2018-10-19T13:37:36Z

CMakeLists.txt

+   HINTS "/usr/lib/petscdir/3.6.3/${PETSC_ARCH}/lib/petsc/conf"
+    "/usr/lib/petsc/conf")
+find_library (PETSC_LIB petsc 
+  HINTS ${PETSc_DIR}/../../


Should that be PETSC_DIR ?

stephankramer · 2018-10-19T13:38:11Z

CMakeLists.txt

+    "/usr/lib/petsc/conf")
+find_library (PETSC_LIB petsc 
+  HINTS ${PETSc_DIR}/../../
+  HINTS ${PETSc_DIR}/../lib)


Patol75 · 2018-10-23T01:22:57Z

Hi,

I may not have much input on this discussion, but I have seen in the multiple CMakeLists.txt in the commits that you aim at supporting CMake 2.X versions.

cmake_minimum_required(VERSION 2.6)
or
cmake_minimum_required(VERSION 2.8)

Building on Stephan comment, it looks that CMake was recently the subject of a lot of much welcomed rewriting and improvements as the community was complaining about multiple aspects like hard to read/understand features, way too complicated ways of doing simple things, useless doc, etc...
My concern here is the compatibility issues and basically the complexities that it would raise to support outdated versions of CMake (v2.6 released in 2008, v2.8 in 2009) when current version (v3.13) sounds potentially quite different. As an example, Paraview is currently built with CMake and requires v3.3 at least; Ubuntu Bionic is shipped with CMake v3.10.2.

Again I am no expert and I have just had a read on the topic (this blog post for example https://pabloariasal.github.io/2018/02/19/its-time-to-do-cmake-right/; I also link this RTD which may be useful https://cgold.readthedocs.io/en/latest/index.html), but I think building Fluidity through a multi-platform capable tool as CMake is indeed an interesting choice to make.

jrper · 2018-10-23T07:48:15Z

Many of those CMakeLists.txt files reflect the situation when the branch was first created back in April 2016, where the Ubuntu LTS version was 14.04 with CMake 2.8. However my ongoing experience with CMake is that they go to extreme lengths to maintain backwards compatibility, via the minimum_requirements and policy options. As such, my interpretation of best practise is that we should be requiring the lowest version number containing the features we're actually using, or the lowest version on which that file has been tested, as indeed we are.

tmbgreaves · 2019-01-31T11:06:01Z

Thanks for the feedback on this and sorry it got shelved for a while -- my jobs list got flooded with other projects for the last few months. Following up from Issue #219 I'll pull the supporting software bits of this PR into a separate branch and deal with them in isolation, then aim to address #219, and then come back to this PR which should by then be focused purely on CMake.

tmbgreaves · 2021-05-10T12:39:51Z

This is sufficiently stale and has had little enough continued interest that I'm closing it in favour of keeping the build system as it is.

jrper and others added 5 commits October 12, 2018 14:50

Removing local copies of judy, spud, and spatialindex and adding

7588d9e

support for system versions. This also removes FLLINKER from the build system, added for the now-obsolete Archer system build.

Updating dockerfiles

3f395a4

This commit adds bionic support, and adds system packages for Judy, spatialindex, and spud

Standardise testing; add cmake testing

03be901

Add in a cmake Jenkins test and rename 'make test' to 'make shorttest' which replaces the misleading 'serialtest' tag. Test packages have been merged into the fluidity-base package; this is added as the dockerfile metapackage.

Fixing GitHub issue #216 (vtk library missing)

9baab25

tmbgreaves added bug enhancement labels Oct 14, 2018

tmbgreaves requested review from stephankramer and jrper October 14, 2018 21:48

tmbgreaves self-assigned this Oct 15, 2018

stephankramer requested changes Oct 19, 2018

View reviewed changes

stephankramer mentioned this pull request Oct 24, 2018

Configure Fluidity in a separate build directory #219

Open

tmbgreaves mentioned this pull request Jan 31, 2019

Excise supporting software #220

Closed

tmbgreaves closed this May 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

General overhaul of the build and test system #217

General overhaul of the build and test system #217

tmbgreaves commented Oct 14, 2018

tmbgreaves commented Oct 14, 2018

tmbgreaves commented Oct 14, 2018

jrper commented Oct 18, 2018

jrper commented Oct 18, 2018

stephankramer left a comment

stephankramer Oct 19, 2018

tmbgreaves Jan 31, 2019

stephankramer Oct 19, 2018

tmbgreaves Jan 31, 2019

stephankramer Oct 19, 2018

tmbgreaves Jan 31, 2019

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

stephankramer Oct 19, 2018

Patol75 commented Oct 23, 2018

jrper commented Oct 23, 2018

tmbgreaves commented Jan 31, 2019

tmbgreaves commented May 10, 2021

General overhaul of the build and test system #217

General overhaul of the build and test system #217

Conversation

tmbgreaves commented Oct 14, 2018

tmbgreaves commented Oct 14, 2018

tmbgreaves commented Oct 14, 2018

jrper commented Oct 18, 2018

jrper commented Oct 18, 2018

stephankramer left a 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

Patol75 commented Oct 23, 2018

jrper commented Oct 23, 2018

tmbgreaves commented Jan 31, 2019

tmbgreaves commented May 10, 2021