New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CMake] Remove external projects #1907

Merged

bilke merged 25 commits into ufz:master from bilke:rm-ext-project

Sep 12, 2017

Member

bilke commented Aug 9, 2017 •

edited

Loading

This PR removes the possibility to automatically build required third-party packages as this is already integrated by Conan.

Now you have two choices to handle third-party libs:

Installing manually / via system packages
Installing with Conan (by enabling the CMake option OGS_USE_CONAN=ON) which is fully integrated into CMake, see docs

Conan installs prebuilt binaries. As not all compiler / os combinations are available the default behavior is to build missing binaries locally.

Points to discuss

1. Should header-only libs (i.e. Eigen) be integrated with git submodule? This allows for easily changing libraries version (with e.g. cd ThirdParty/Eigen; git checkout 3.3) and we have less Conan packages to manage. After discussion: No for the moment.

Conan migration guide

DELETED, not required anymore

TODOs

Update docs.opengeosys.org -> https://docs.opengeosys.org/docs/devguide/advanced/conan-package-manager

bilke added the WIP 🏗️ label

bilke force-pushed the rm-ext-project branch 3 times, most recently from c1b632c to 78959e6 Compare

August 14, 2017 07:57

bilke requested a review from endJunction

August 14, 2017 10:27

bilke added please review and removed WIP 🏗️ labels

endJunction reviewed

View reviewed changes

scripts/cmake/CompilerSetup.cmake Outdated

@@ @@ -148,3 +160,4 @@ endif() @@
               # preceding cxx flags definition in order to override earlier flags, e.g. for
               # optimization.
               set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${OGS_CXX_FLAGS}")
+              add_compile_options(${OGS_CXX_FLAGS})

Member

endJunction Aug 17, 2017

Just curious, why is this add_compile_options needed beside the setting of the CMAKE_CXX_FLAGS?

endJunction approved these changes

View reviewed changes

bilke force-pushed the rm-ext-project branch from 78959e6 to cdd32ec Compare

August 17, 2017 12:58

chleh approved these changes

View reviewed changes

Collaborator

chleh left a comment

Mostly comments about old compiler flags. One comment about ssh security.
Where do we put the Conan migration guide after this PR is merged?

scripts/cmake/CompilerSetup.cmake

                   endif()
                   if(NOT CMAKE_BUILD_TYPE STREQUAL "Debug")
                       message(STATUS "Set GCC release flags")
                       set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -O3 -DNDEBUG")
                   else()
                       # Enable assertions in STL in debug mode.
-                      if (NOT STL_NO_DEBUG)
-                          set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_ASSERT -D_GLIBCXX_DEBUG_PEDASSERT -D_GLIBCXX_DEBUG_VERIFY")
+                      if(NOT STL_NO_DEBUG)

Collaborator

chleh Aug 17, 2017

Do we need to set this up manually?

Member

endJunction Aug 21, 2017

This is an extra option introduced in #807.

Collaborator

chleh Aug 21, 2017

Ah, I see. Thanks for telling me. But actually we could ask ourselves if that option shouldn't be better removed from our cmake config, because (i) it's rather special, i.e., it is not an OGS feature in any way (ii) every pro-user can set it in their custom cmake config.
But I agree, STL_NO_DEBUG is a useful option. If we wanted to remove it, we should put it into some "How to debug" document.

scripts/cmake/CompilerSetup.cmake

                       endif()
                   endif()
-                  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CPU_FLAGS} -Wno-deprecated -Wall -Wextra -fext-numeric-literals")
+                  set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${CPU_FLAGS} -Wno-deprecated -Wall \
+                      -Wextra -fext-numeric-literals")

Collaborator

chleh Aug 17, 2017

Is "-fext-numeric-literals" necessary?

Member Author

bilke Aug 22, 2017

Somehow this was necessary but I don't remember the details..

scripts/cmake/CompilerSetup.cmake

                   if(CMAKE_BUILD_TYPE STREQUAL "Debug")
                       # Enable assertions in STL in debug mode.
-                      if (NOT STL_NO_DEBUG)
-                          set(CMAKE_CXX_FLAGS_DEBUG "${CMAKE_CXX_FLAGS_DEBUG} -D_GLIBCXX_DEBUG -D_GLIBCXX_DEBUG_ASSERT -D_GLIBCXX_DEBUG_PEDASSERT -D_GLIBCXX_DEBUG_VERIFY")
+                      if(NOT STL_NO_DEBUG)

Collaborator

chleh Aug 17, 2017

??

Member Author

bilke Aug 22, 2017

Was introduced here: 6a0dcaa

scripts/cmake/CompilerSetup.cmake

                   endif()
                   set(PROFILE_FLAGS "-pg -fno-omit-frame-pointer -O2 -DNDEBUG")
                   # clang compiler does not know the following flags
                   if(NOT COMPILER_IS_CLANG)
-                      set(PROFILE_FLAGS "${PROFILE_FLAGS} -fno-inline-functions-called-once -fno-optimize-sibling-calls")
+                      set(PROFILE_FLAGS "${PROFILE_FLAGS} -fno-inline-functions-called-once \
+                          -fno-optimize-sibling-calls")

Collaborator

chleh Aug 17, 2017

Is profiling used anywhere?

Member Author

bilke Aug 22, 2017

We had a Jenkinks job in the past with this enabled...

scripts/jenkins/gcc.groovy Outdated

+                  if (helper.isOriginMaster(this)) {
+                      sshagent(credentials: ['www-data_jenkins']) {
+                          sh 'rsync -a --delete --stats -e "ssh -o StrictHostKeyChecking=no"' +

Collaborator

chleh Aug 17, 2017

Is it wise to put StrictHostKeyChecking=no here? In this case it might not be a problem, because we just copy some public data there, probably using public key authentication. But in general host keys should be checked. If editing the usual known_hosts file is not an option, there also the possibility to specify a -oUserKnownHostsFile on the command line.

Member Author

bilke Aug 22, 2017

No. Initially this comes from the times as we had no ssh access to the Jenkins server ... Will remove it.

Collaborator

chleh Aug 22, 2017

Thanks.

Member

endJunction commented Aug 21, 2017

In the ctests the Adaptive_dt_ThermalConvectionFlow2D benchmark is failing for some reasons but there were no changes to the actual FEM code.
Could someone have a look on these? ( @wenqing or @TomFischer )?

Member

wenqing commented Aug 21, 2017

I am going to take a look at the ctest.

Member

wenqing commented Aug 21, 2017 •

edited

Loading

@bilke @endJunction
I ran the tests (with branch bilke:rm-ext-project ) on Jenkins at my account on envinf1. All the tests are passed.
The failed test in PR #1907 is

 [gcc] 170/199 Test #145: ogs-Adaptive_dt_ThermalConvectionFlow2D_Constant_Viscosity-time-vtkdiff ...................***Failed

Since the directory of /data/ogs/jenkins/workspace/7-ELU3QZ43AJ/build/ is gone, I cannot find any clue to the failure. Could you please restart tests on the jenkins?

Member

endJunction commented Aug 21, 2017 •

edited

Loading

Started new jenkins build.

bilke force-pushed the rm-ext-project branch from 18a2dcf to 826d085 Compare

August 22, 2017 07:27

Member Author

bilke commented Aug 22, 2017 •

edited

Loading

Again everything is green except:

145 - ogs-Adaptive_dt_ThermalConvectionFlow2D_Constant_Viscosity-time-vtkdiff (Failed)

Maybe the Eigen version from Conan differs from the one installed with ExternalProject (although both are 3.2.9)? Will check that..

Member Author

bilke commented Aug 23, 2017

@chleh Conan migration guide is hosted here.

Member

endJunction commented Sep 1, 2017 •

edited by bilke

Loading

Still conan problems:
New conan, ~/.conan removed.

conan remote list                    
ogs: https://api.bintray.com/conan/ogs/conan [Verify SSL: True]
conan-center: https://conan.bintray.com [Verify SSL: True]
conan-transit: https://conan-transit.bintray.com [Verify SSL: True]

then building with conan install ../s -u --build=missing builds VTK and bzip2, but fails to build zlib:

PROJECT: 
zlib/1.2.8@lasote/stable: ERROR: Package 'bfe54884bf4e83adb9a2749ddbd047b02be54c43' build failed
zlib/1.2.8@lasote/stable: WARN: Build folder /home/naumov/.conan/data/zlib/1.2.8/lasote/stable/build/bfe54884bf4e83adb9a2749ddbd047b02be54c43
ERROR: zlib/1.2.8@lasote/stable: Error in build() method, line 39
        if self.settings.compiler != "gcc" or self.settings.compiler.version > "4.1":
        TypeError: '>' not supported between instances of 'SettingsItem' and 'str'

Is it possible to use system's VTK and zlib and bzip? ✅

See 138276f

Member Author

bilke commented Sep 4, 2017 •

edited

Loading

@endJunction zlib and bzip2 should not be necassary, see https://github.com/lasote/conan-boost/issues/52. ✅

bilke force-pushed the rm-ext-project branch 6 times, most recently from 53d4f53 to decf9f7 Compare

September 6, 2017 07:21

bilke mentioned this pull request

Various fixes from #1907 #1943

Merged

bilke added a commit that referenced this pull request


          Merge pull request #1943 from bilke/various-fixes2

b38d19b

Various fixes from #1907

bilke force-pushed the rm-ext-project branch from 24f8ca1 to 13a5c09 Compare

September 8, 2017 08:13

Member Author

bilke commented Sep 8, 2017

@endJunction This is now ready to merge. The issues on envinf1 with gcc 6.2 could not be resolved so far, people on the internet even mention a bug in ld and suggest to upgrade it but this is not possible at envinf1 at least I think ..

bilke added 9 commits

September 11, 2017 10:28


          [CMake] Fixed Win ninja 64-bit in conan.cmake.

32c4307


          [Jenkins] Removed -DOGS_LIB_XXX=Sytem."

90ef799


          [Appveyor] Fixing Conan usage.

c53869b


          [CMake] Moved ConanSetup to after CMakeSetup.

623d322

This makes sure that CMAKE_BUILD_TYPE variable is set which is
required by Conan.


          [Appveyor] Same build order as on Jenkins.

d47d49b


          [Conan] Removed obsolete Boost option add VTK:fPIC on BUILD_SHARED_LIBS.

cb9b237

The latter was reported N. Grunwald and should be fixed with the added
fPIC option in the VTK Conan package.


          [Conan] Switching to Artifactory repo.

736f67c


          [CMake] Automatically add ogs conan remote repo.

11d0035


          [Conan] Switch to Boost 1.64.0 with fewer dependencies.

9b8f128

My Boost fix was integrated in 1.64.0 only.

Added Conan remote conan-community where the package is hosted.

bilke force-pushed the rm-ext-project branch from 1feb1ea to 9b8f128 Compare

September 11, 2017 08:28

Member

endJunction commented Sep 11, 2017

If using system libraries is still possible ⏩ from me.
@TomFischer will check it on his computer once more and merge it.

Collaborator

chleh commented Sep 11, 2017

@bilke thank you for the clarification.


          [CMake] Added option OGS_CONAN_BUILD to control local lib builds.

c15a230

Possible values: missing (default), all, never, list of lib names

Examples:

cmake . -DOGS_CONAN_BUILD="Eigen3;Boost" -> Builds Eigen and Boost locally
cmake . -DOGS_CONAN_BUILD=all            -> Builds all libs locally

This basically reimplements all functionality we had with ExternalProject.

Member Author

bilke commented Sep 11, 2017

@endJunction @chleh @wenqing Thanks for your feedback! I have added another last commit (finally) which gives complete control over what is used directly from Conan binary packages and what is build locally. So for example if Conan VTK on envinf1 does not work with gcc 6.2 one can still use Conan but just build VTK locally:

cmake ../ogs -DOGS_USE_CONAN=ON -DOGS_CONAN_BUILD=VTK

Member

TomFischer commented Sep 12, 2017 •

edited

Loading

I checked out the branch and created a new build folder. Then in the build folder I did:
cmake ../sources/ -DOGS_USE_CONAN=On
and obtained the following error message:
-- Conan adding ogs remote repositoy (https://api.bintray.com/conan/ogs/conan) usage: conan remote [-h] {list,add,remove,update,list_ref,add_ref,remove_ref,update_ref} ... conan remote: error: unrecognized arguments: -i -- Conan adding community remote repositoy (https://api.bintray.com/conan/conan-community/conan) usage: conan remote [-h] {list,add,remove,update,list_ref,add_ref,remove_ref,update_ref} ... conan remote: error: unrecognized arguments: -i -- Third-party libraries: -- Conan ** WARNING** : This detection of settings from cmake is experimental and incomplete. Please check 'conan.cmake' and contribute -- Conan executing: conan install -g cmake -s build_type=Debug -s os=Linux -s compiler=gcc -s compiler.version=7.1 -s compiler.libcxx=libstdc++11 --build=missing --update ERROR: Invalid setting '7.1' is not a valid 'settings.compiler.version' value. Possible values are ['4.1', '4.4', '4.5', '4.6', '4.7', '4.8', '4.9', '5.1', '5.2', '5.3', '5.4', '6.1', '6.2', '6.3'] Read "http://docs.conan.io/en/latest/faq/troubleshooting.html#error-invalid-setting" CMake Error at scripts/cmake/conan/conan.cmake:240 (message): Conan install failed='1'

What is wrong here? It seems the problem is that my compiler is gcc 7.1.
@bilke Some time ago there was a small tutorial in this PR how to use the new scheme when conan was used before. Unfortunately, I can not find it anymore.

Member Author

bilke commented Sep 12, 2017 •

edited

Loading

@TomFischer Thanks for testing! Your Conan is way too old! Please upgrade it (via pip install conan --upgrade) to at least 0.26.0 (with pip you should get the latest version which is 0.26.1).

gcc 7.1 should be ok with the upgraded Conan but please be aware that for the moment there are no prebuilt packages for gcc 7.x so it will build them locally.

add a Conan version check
build gcc 7.x Conan packages

bilke added 2 commits

September 12, 2017 08:58


          [CMake] Added Conan version check (>= 0.26.0).

e47aab4


          [CMake] Formatting ConanSetup.cmake.

201c0ea

Collaborator

chleh commented Sep 12, 2017 •

edited

Loading

~~Just out of curiosity: Why do we need gcc 7.1 if conan provides older versions of gcc prebuilt?~~

Collaborator

chleh commented Sep 12, 2017

@chleh Conan migration guide is hosted here.

Collaborator

chleh commented Sep 12, 2017

Btw: Did anybody test this PR under Windows?

Member Author

bilke commented Sep 12, 2017

@chleh I have to update the docs. The migration should not be necessary anymore. Jenkins tested this PR on Windows 😜..

@TomFischer I have built gcc 7.1 packages for VTK. A gcc 7.2 build environment (a Docker container) for creating the packages is not yet available from Conan. Although 7.1 packages should be compatible with the 7.2 compiler I guess Conan will complain about this version mismatch so maybe you have to built them locally anyway...

wenqing approved these changes

View reviewed changes

Member

wenqing left a comment

No test failed on jenkins now. Jenkins shows only one failure

Log parsing has failed

Collaborator

chleh commented Sep 12, 2017

I'm so glad, Jenkins always cares for us 😄 (and @bilke always cares for Jenkins!).

bilke merged commit 0facd3b into ufz:master

bilke deleted the rm-ext-project branch

September 12, 2017 12:42

bilke removed the please review label

Member

endJunction commented Sep 12, 2017 •

edited

Loading

@bilke What was the necessary conan remote for the VTK and other OGS specialities? I don't see it neither here nor in the docs page any longer... ;(

Another error using intel compilers cmake ../ogs -DCMAKE_CXX_COMPILER=icpc -DCMAKE_C_COMPILER=icc -DOGS_USE_PCH=OFF -DCMAKE_BUILD_TYPE=RelWithDebInfo -DOGS_USE_CONAN=On -DOGS_USE_PETSC=On is:

-- Conan adding community remote repositoy         (https://api.bintray.com/conan/conan-community/conan)
-- Third-party libraries:
-- Conan ** WARNING** : This detection of settings from cmake is experimental and incomplete. Please check 'conan.cmake' and contribute
CMake Error at scripts/cmake/conan/conan.cmake:138 (message):
  Conan: compiler setup not recognized
Call Stack (most recent call first):
  scripts/cmake/conan/conan.cmake:316 (conan_cmake_settings)
  scripts/cmake/ConanSetup.cmake:91 (conan_cmake_run)
  CMakeLists.txt:41 (include)

Member Author

bilke commented Sep 15, 2017

@endJunction You can find the remote here: https://github.com/ufz/ogs/blob/master/scripts/cmake/ConanSetup.cmake#L67.

I have to check what we can do if we use a compiler which is not yet known by Conan...

Member

ogsbot commented Jun 19, 2020

OpenGeoSys development has been moved to GitLab.

See this pull request on GitLab.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet