Require opm parser

[joakim-hove]

With this PR opm-core will depend on opm-parser. No changes to the opm-core code, but a test is added.

[bska]

Looks good and I appreciate the work. I'll merge into opm-parser-integrate.

That said, there is a follow-on discussion that's coming from this that I didn't realise until now. As opm-parser (currently?) uses version 3 of the Boost.Filesystem, eventually merging the -integrate branch into master will necessitate raising OPM's minimum Boost requirements from 1.39 to (at least) 1.44 (although 1.46 is more likely as v3 was not made the default until 1.46).

Do you intend to maintain the v3 minimum requirement in opm-parser? I do suppose supporting v2 is ultimately fruitless, because v2 is not included in Boosts 1.48 or later.

[bska]

I'm curious about the ordering here. Doesn't this imply that we're looking for opm-parser before cJSON? Wouldn't it be more natural to look for cJSON prior to opm-parser as cJSON is a prerequisite for opm-parser?

I also wonder if the cJSON might not be better placed in opm-parser-prereqs.cmake and not in opm-core-prereqs.cmake. Do you intend to make opm-core directly dependent upon cJSON?

@rolk: What do you think?

[joakim-hove]

The cJSON dependency / build configuration has been quite painful; I will defend the current setup in roughly five seconds before I bend over to a better solution.

But anyway - the situation is as follows:

1. opm-parser requires cJSON, but build system-wise this is not a required dependency. If cJSON is not found during the opm-parser configure phase opm-parser will build and install libcjson itself.
2. So - after the opm-parser build process is complete our system must have opm-parser (obviously), and cJSON - where the latter either is found on the system (no - it is never found); or built as part of the opm-parser build process.

So - all in all it is the first user of opm-parser which will also require cJSON. I agree that the two mentioned lines could be interchanged; or even better:

"opm-parser cJSON REQUIRED"

To clearly indicate that they typically come as lump. But as I said - will not fight for this.

[bska]

Thanks a lot for explaining the situation. From a distance it appears as if opm-parser's dependency relation with cJSON is similar to opm-autodiff's dependency relation with Eigen. In other words, "needed, but can be downloaded from an external source if not already available". I'm afraid I don't know how to best express that relation in a CMake rule so I'll defer to someone else (@rolk? @andlaus? @akva2?) to give technical advice.

At least I now know what problem is being solved here.

[rolk]

cJSON might [...] be better placed in opm-parser-prereqs.cmake

I think the situation is analogous to the way opm-core depends on TinyXML (use system version if present, otherwise use embedded). So I think that it should be a prerequisite of opm-parser, and it should be opm-parser which adds it to its list of libraries, to be picked up indirectly by the others.

(BTW; I don't think the build system supports putting two package names together on one line; there is some parsing of these lines going on to figure out if we have already seen the package, and I think it would be confused)

[joakim-hove]

Yes - I also think the situation is analogous to TinyXML. I can add it to opm-parser as a not required dependency; and if it is not found opm-parser will build it - that is OK.

However when it comes to opm-core the dependency on cJSON is Required - could really need some help here in passing the CJSON dependency from opm-parser (where it is not required) to opm-core where it is required.

[rolk]

when it comes to opm-core the dependency on cJSON is Required

The dependencies are not transitive; opm-core depends only on cJSON if it actually includes one of its headers directly. Findopm-parser.cmake should (anyway) put whereever it found cJSON in ${opm-parser_LIBRARIES} and/or ${opm-parser_INCLUDE_DIRS}, so it'll be included from there at least. (And the OpmFind machinery takes care of building a complete list of whatever is required totally).

[joakim-hove]

OK - thank you:

1. Have removed explicit CJSON dependency in opm-core
2. Have added CJOSN_INCLUDE_DIRS to the OPM_PARSER_INCLUDE_DIRS variable - I guess that was the root problem.

Sorry about the massive whitespace changes in the CMakeLists.txt files - where do they come from? Looks nice in emacs.

[bska]

Sorry about the massive whitespace changes in the CMakeLists.txt files - where do they come from? Looks nice in emacs.

Tab vs. space. I think CMakeLists.txt uses (but does not enforce?) tabs for indentation. @rolk: Is that it? @joakim-hove: If your Emacs is new enough, you may wish to use M-x whitespace-mode to see the before and after shots.

[joakim-hove]

@bska: regarding boost and boost filesystem versions; have limited amount of concious avareness here. I seem to remember that @atgeirr had problems compiling the parser code at one stage and recommended to hike up the boost fs version. Would be cool if @atgeirr remembers?

[bska]

regarding boost and boost filesystem versions [...] I seem to remember that @atgeirr [...] recommended to hike up the boost fs version

I see. Well, as I stated earlier, we clearly need to support v3 as that is the only interface that's shipping with Boost 1.48 or later. On the other hand, if we could find a way to also support v2 without incurring substantial overhead (either mental or processing) then that would be a metric lorry-load of awesome[tm].

Disclaimer: I haven't studied the opm-parser code so I have no idea if supporting both v2 and v3 is possible.

[atgeirr]

I am sorry, I do not remember the details of the boost file v2 and v3 questions I dealt with. I think it concerned basenames and path vs. string or something like that, but I am not sure.

[rolk]

basenames and path vs. string or something like that

There was a similar situation in issue 424, for which @bska found an elegant solution: #424 (comment)

[rolk]

@joakim-hove

Sorry about the massive whitespace changes in the CMakeLists.txt files - where do they come from? Looks nice in emacs.

@bska

Tab vs. space. I think CMakeLists.txt uses (but does not enforce?) tabs for indentation.

Whoever wrote cmake-mode in Emacs did not think it through -- it seems to mix tabs and spaces in various levels of indentation(!); it wouldn't surprise me if it feels an urge to reindent spuriously too.

[bska]

Okay, I think this should go in now to get the ball rolling on integrating the parser. The minimum Boost requirement is fine for the integration work.

I'll merge this into master opm-parser-integrate. Thanks a lot.