changed: switch to the shared opm build system

[akva2]

As requested, here's a change to the shared build system as a step in the unification process.

The main change is the change to a single CMakeLists.txt. File structure is kept as-is. I suspect we need an iteration or two to make it work on windows (again).

Upstream: https://github.com/OPM/opm-common/pull/324

[akva2]

jenkins build this with downstreams opm-common=323 opm-grid=304 please

[akva2]

@blattms mind giving it a brief look to see if something raises any eyebrows ?

[akva2]

jenkins build this with downstreams opm-common=323 opm-grid=304 opm-material=281 please

[blattms]

Well the installed opm-parser-config.cmake looks a bit suspicious.

opm-parser_INCLUDE_DIRS has the non-existing directories include/opm-parallel-debug/include, include/lib/eclipse/include, include/lib/json/include, and include/external/cjson in it. Not sure whether that is a problem, though.

All libraries in opm-parser_LIBRARIES have the full path except libopmjson. libecl is listed twice once with full path, once without. I probably should test this with a downstream module.

BTW I actually like the previous usage of targets in the package configuration files of opm-parser. They were very Cmaky and looked convenient and clean. Will they reappear?

[akva2]

we can add the targets for sure, but we should do it in a generic way, not in this scope.

[pgdr]

Up until now, opm-parser has been quite easy to build. Is there a reason behind this change?

[alfbr]

Up until now, opm-parser has been quite easy to build. Is there a reason behind this change?

It should still be easy to build, please report any issues you may see. The main reason is that opm-parser is no longer intended as an independent project, and integrating it closer with the build chain of flow then makes sense, at least if you are at the paying end of maintenance.

[pgdr]

Up until now, opm-parser has been quite easy to build. Is there a reason behind this change?

It should still be easy to build, please report any issues you may see.

Well, if opm-parser will depend on opm-common (with all its quirks), that means that all our users of opm-parser as a library need to go through a bigger hurdle to build it.

The main reason is that opm-parser is no longer intended as an independent project, and integrating it closer with the build chain of flow then makes sense, at least if you are at the paying end of maintenance.

That is very sad to hear. I don't want to start a discussion about the direction of the OPM project, but when we in Ert decided to rip libecl out of the bigger Ert system, that was a celebrated move by everyone that only used libecl functionality.

We have several users who only use opm-parser functionality, and this is going in the exact opposite direction; users of opm-parser will see a significant increase in complexity, and I fear as a biproduct, higher instability.

[andlaus]

@akva2: While I'm indifferent on whether this PR should be merged or not, it does not yet specify the dependency on opm-common mentioned by @pgdr in dune.module...

[akva2]

thanks, didn't get around to testing with dc yet. will amend.

[akva2]

jenkins build this with downstreams opm-common=324 opm-grid=304 please

[blattms]

Thanks for the clarification.I was just curious. Later cleanup is OK of course.

[akva2]

jenkins build this with downstreams opm-common=324 opm-grid=304 please

[alfbr]

Well, if opm-parser will depend on opm-common (with all its quirks), that means that all our users of opm-parser as a library need to go through a bigger hurdle to build it

Then let us make sure that does not happen. I believe we will be able to reach a solution that will be better for Sunbeam, and any users it may have. Have some faith :-)

[akva2]

jenkins build this with downstreams opm-common=324 please

[joakim-hove]

Is this considered ready to merge now?

[akva2]

from my side it's ready, though i would want @blattms to review some of it again, in particular the upstream pr.

[atgeirr]

I think that while this is what we want, we should make sure that the comments @pgdr and @alfbr are heeded:

all our users of opm-parser as a library need to go through a bigger hurdle to build it

Then let us make sure that does not happen

I am not sure we are there yet. One reason I am not sure is that it is unclear to me what quirks or hurdles are referred to. Could you tell us @pgdr? Otherwise they are hard to address...

We are considering making opm-parser and opm-common (as well as opm-output) a single module, which hopefully should lay most fears to rest (in that there would be no need to build multiple modules). The work done in this PR is a prerequisite for that unification.

[alfbr]

we should make sure that the comments @pgdr and @alfbr are heeded

From my end, I am simply stating that I believe all concerns will be taken care of. I did not intend to postpone this PR.

[blattms]

@akva2 Which upstream PR? Isn't OPM/opm-common#323 merged already?

[akva2]

things have moved on to https://github.com/OPM/opm-common/pull/324 after review comments :)

[blattms]

There are still a few things that strike me (maybe I am to picky here):

1. There are bogus include directories in an stalled opm-parser-config.cmake (e.g. /tmp/include/sibling-dir). But this should not break anything
2. Dependency on opm-common should go to dune.module, too.
3. Isn't there still a target missing for libopmjson?

[blattms]

Concerning 3, there is definitely an issue on a clean build.

-- Configuring done
CMake Error: install(EXPORT "opm-parser-targets" ...) includes target "opmparser" which requires target "opmjson" that is not in the export set.
-- Generating done
-- Build files have been written to: /home/mblatt/src/dune/opm-2.6/opm-parser/opm-parallel
--- Failed to build opm-parser ---

[blattms]

Please ignore my last comment. I messed up my local branch (missing --hard option when resetting). 3 is already fixed. Sorry for the noise.

[akva2]

It should be set in opm-parser_EXTRA_TARGETS and thus be included in the export. It worked just fine for me in the and in the jenkins run so i am a bit confused. Will look into it but not before monday.

[akva2]

i've fixed one final issue with the include directories but i have no idea where those /tmp paths come from. this is what i get now opm-parser-config.cmake (in-tree)

 set (opm-parser_INCLUDE_DIRS "/home/akva/kode/opm/sibling/opm-parser;/usr/include;/home/akva/kode/opm/sibling/libecl/lib/include;/home/akva/kode/opm/sibling/libecl/build/lib/include")
  list(APPEND opm-parser_INCLUDE_DIRS /home/akva/kode/opm/sibling/opm-parser/lib/eclipse/include;/home/akva/kode/opm/sibling/opm-parser/lib/json/include;/home/akva/kode/opm/sibling/opm-parser/build/include;/home/akva/kode/opm/sibling/opm-parser/external/cjson)

and installed (to /usr/local):

  set (opm-parser_INCLUDE_DIRS "/usr/local/include;/usr/include;/home/akva/kode/opm/sibling/libecl/lib/include;/home/akva/kode/opm/sibling/libecl/build/lib/include")

where i am building against a sibling for libecl so those dirs are expected broken. 2) confused. it has already been added there so not sure what i am missing.

it sounds like you tested old code when you got these 3 issues.

[andlaus]

I tested this one a bit. it seems like quite a few tests are missing: with this PR only 55 tests are run with ctest while the current master triggers 93.

[akva2]

with the upstream pr ? i get 93.

[andlaus]

maybe there were some issues with detecting opm-data. I'll have a look.

[andlaus]

hm. weird: if I specify opm-data_DIR instead of OPM_DATA_ROOT I only get 38 tests with make test-suite && ctest. That said, setting BUILD_TESTS=ON yields 93 tests, this probably indicates that add_test() instead of opm_add_test() is still used in some places?

I also noticed that when building the code generator for a fresh build, quite a few compiler warnings about non-existing include directories are produced:

cc1plus: warning: /home/and/src/opm-parser/build-cmake/include: No such file or directory [-Wmissing-include-dirs]

to make the build system jump through fewer hoops, it should maybe considered not to "embed" the code generator into the build anymore. this would be analogous to the way how the specializations of the Evaluation class in opm-material are handled.

[akva2]

• there is no add_test usage but i repeat you need the upstream opm-common PR or tests added from CMakeLists_files.txt are not added to the buildsystem if BUILD_TESTING=0. then you only get the tests added by opm_add_test() macro (ie ExtraTests.cmake) which is precisely 38 tests.
• there is no support for opm-data_DIR cause opm-data is no cmake module.
• the include paths warnings should be gone now. i cannot test as i cannot build with clang on my box due to boost stuff. required some extra nasty code but what can you do.
• i'm not getting into a discussion of whether to run codegen or not at this time. this is a port of the existing build system.

[blattms]

The /tmp path was my CMAKE_INSTALL_PREFIX.

[blattms]

Tried again (sibling build in opm-parallel-debug with CMAKE_INSTALL_PREFIX=/tmp). Baby does not build:

[ 11%] Building CXX object CMakeFiles/opmparser.dir/lib/eclipse/Deck/Section.cpp.o
/usr/bin/c++   -DHAVE_CONFIG_H=1 -I/home/mblatt/src/dune/opm-2.6/opm-parser/opm-parallel-debug -I/home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include -I/home/mblatt/src/dune/opm-2.6/opm-parser/lib/json/include -I/home/mblatt/src/dune/opm-2.6/opm-parser/external/cjson -I/home/mblatt/src/dune/opm-2.6/opm-parser -isystem /home/mblatt/src/dune/opm-2.6/libecl/lib/include -isystem /home/mblatt/src/dune/opm-2.6/libecl/opm-parallel-debug/lib/include -I/home/mblatt/src/dune/opm-2.6/opm-parser/build/include  -std=c++14 -DDEBUG -g -O0 -Wall -Wextra -DDEBUG -pipe -Wno-unknown-pragmas -pthread -mtune=native -DNDEBUG -fPIC   -pthread -std=c++14 -o CMakeFiles/opmparser.dir/lib/eclipse/Deck/Section.cpp.o -c /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/Deck/Section.cpp
In file included from /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include/opm/parser/eclipse/EclipseState/Tables/TableManager.hpp:44:0,
                 from /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include/opm/parser/eclipse/EclipseState/Eclipse3DProperties.hpp:31,
                 from /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include/opm/parser/eclipse/EclipseState/EclipseState.hpp:26,
                 from /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include/opm/parser/eclipse/Parser/Parser.hpp:30,
                 from /home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/Deck/Section.cpp:30:
/home/mblatt/src/dune/opm-2.6/opm-parser/lib/eclipse/include/opm/parser/eclipse/EclipseState/Tables/Tabdims.hpp:28:58: fatal error: opm/parser/eclipse/Parser/ParserKeywords/T.hpp: Datei oder Verzeichnis nicht gefunden
 #include <opm/parser/eclipse/Parser/ParserKeywords/T.hpp>
                                                          ^
compilation terminated.
CMakeFiles/opmparser.dir/build.make:611: die Regel für Ziel „CMakeFiles/opmparser.dir/lib/eclipse/Deck/Section.cpp.o“ scheiterte
make[2]: *** [CMakeFiles/opmparser.dir/lib/eclipse/Deck/Section.cpp.o] Fehler 1
make[2]: Verzeichnis „/home/mblatt/src/dune/opm-2.6/opm-parser/opm-parallel-debug“ wird verlassen
CMakeFiles/Makefile2:2397: die Regel für Ziel „CMakeFiles/opmparser.dir/all“ scheiterte
make[1]: *** [CMakeFiles/opmparser.dir/all] Fehler 2
make[1]: Verzeichnis „/home/mblatt/src/dune/opm-2.6/opm-parser/opm-parallel-debug“ wird verlassen
Makefile:138: die Regel für Ziel „all“ scheiterte
make: *** [all] Fehler 2
mblatt@smaug:~/src/dune/opm-2.6/opm-parser/opm-parallel-debug$ find .. -name T.hpp 
../build/lib/eclipse/include/opm/parser/eclipse/Parser/ParserKeywords/T.hpp
../opm-parallel/include/opm/parser/eclipse/Parser/ParserKeywords/T.hpp
../opm-parallel-debug/include/opm/parser/eclipse/Parser/ParserKeywords/T.hpp

~There is an include path /home/mblatt/src/dune/opm-2.6/libecl/opm-parallel-debug/lib/include set that does not exist. Maybe we should strip lib from it?~

[akva2]

that really should have been taken care of now, it's only set as a private include for the relevant targets, so it should not go into any global strings. can you find where it is added? i don't find any references to that left, ie, only https://github.com/OPM/opm-parser/pull/1199/commits/5b4e117b8a3b01839997280d5a879b1bfd5436b1#diff-af3b638bc2a3e6c650974192a53c7291R144 should refer to it the required additional reference to it in the config file for siblings.

oh wait. libecl. sorry, read too quick. yes, there are stuff from libecl that is less than optimal but i don't think it should be fixed here. we should fix it upstream. it's a bit problematic as the ecl build system is not setup to handle installed vs in-tree stuff so i had put it off. . i'll see if i can cook something they can accept upstream.

[blattms]

Indeed I use old code from january 25. Somehow I cannot get git to force fetch from your repository.

[akva2]

dang it, had some loose state in my tree, this branch has been rewritten. should fix the error you had above in downstreams with sibling builds.

you probably get issues because i force rewrite the branch instead of appending fix commits. just reset --hard on it instead of merging or such.

[blattms]

Here is what I did now. Added a new remote and checked out the branch as dangling head. Compilation is fine now. But for the installed opm-parser-config.cmake, I see references to the build tree if libecl:

set (opm-parser_LIBRARIES ${opm-parser_LIBRARY} "-lpthread;/usr/lib/x86_64-linux-gnu/libboost_system.a;/usr/lib/x86_64-linux-gnu/libboost_filesystem.a;/usr/lib/x86_64-linux-gnu/libboost_regex.a;/home/mblatt/src/dune/opm-2.6/libecl/opm-parallel-debug/lib/libecl.so.2.3;m;dl;-pthread;/usr/lib/liblapack.so;/usr/lib/libblas.so;/usr/lib/x86_64-linux-gnu/libz.so;opmjson;ecl")
set (opm-parser_INCLUDE_DIRS "/tmp/include;/usr/include;/home/mblatt/src/dune/opm-2.6/libecl/lib/include;/home/mblatt/src/dune/opm-2.6/libecl/opm-parallel-debug/lib/include")

libecl was configured and installed using the same CMAKE_INSTALL_PREFIX and sibling dir.

[akva2]

i cannot reproduce any issues when using installed libraries and sibling builds off. i only get expected paths.

i can only assume it picks up libecl from a sibling dir. then you will obviously get build- and source tree references since you are not building against an installed library.

[akva2]

i get exactly the expected entries when i use an installed libecl; installed to /tmp/foo - i get exactly 1 include path /tmp/foo/include and a link to /tmp/foo/lib/libecl.so.2.3

so i really think this is read for merge now.

[akva2]

jenkins build this with downstreams opm-common=324 please

[akva2]

jenkins build this with downstreams opm-common=324 opm-output=252 please

[atgeirr]

@blattms if you are satisified with the changes please indicate so by accepting them.

[atgeirr]

Locally this works well for me, for both regular and "super"-cmake build. Since the upstream PR changed a little I'll rerun jenkins.

[atgeirr]

jenkins build this opm-common=324 please

[atgeirr]

Jenkins is green, no new comments, all points raised have been addressed as far as I can tell: can we merge this now?

[joakim-hove]

Yes - I am not able to push button myself today, but feel free to do it. Or I will do it later.

[atgeirr]

In she goes, thanks for the effort!