Feature/eckit geo

[pmaciel]

This is an integration of the eckit::geo grids and iterators into eccodes. This needs to be switched at configuration time with both:

• CMake variables ENABLE_GEOGRAPHY=ON and ENABLE_ECKIT_GEO=ON (configuration time), and
• env variable ECCODES_ECKIT_GEO=1 (runtime)

in which case any request for a eccodes "geo iterator" will create a eckit::geo:Grid and iterate from it -- that is, there only one eckit::geo-based iterator, which should support all GRIB grids.

This version already supports the corrections to known GRIB problems from a yaml, and some other corrections hardcodes, as fixed in mir (the "GribSpec" class is imported from MIR's "GribInput").

Also, note that not all grids are supported or tested just yet, though the regular_ll, regular_gg, reduced_gg, HEALPix, and similar should work minus some fixes. The projection-based grids not so, there's no link to the "ProjectionSpec" yet.

[pmaciel]

I've changed the code to minimise the intrusion in the original code. The building of eckit::geo-based iterators is now placed at the earliest possible call.

[pmaciel]

Before anyone asks (!) the change in grib_tools.cc is to call eckit::Main::initialise, a requirement, compiled out in case of disabled feature. It also supports puture packing from eckit (when that happens.)

[shahramn]

We won't need to call:
eckit::Main::initialise(argc, argv);
in the tools. As the tools will not need to change.
We are only changing the implementation of the src/geo_iterators

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.80%. Comparing base (af10b60) to head (6cc614d).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #275   +/-   ##
========================================
  Coverage    87.80%   87.80%           
========================================
  Files          808      808           
  Lines        62095    62096    +1     
  Branches     11032    11032           
========================================
+ Hits         54523    54524    +1     
  Misses        7572     7572           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pmaciel]

We won't need to call: eckit::Main::initialise(argc, argv); in the tools. As the tools will not need to change. We are only changing the implementation of the src/geo_iterators

This is a requirement on using eckit, and is part of its design. Main holds a singleton to access command-line options, and "resources" (specially formatted arguments and environment variables), and is responsible for initialising low-level infrastructure like logging channels. The natural place for it to be is right next to the main() function, hence the change.

But well pointed, I added an initialisation if you trigger the eckit::geo functionality not from a tool that it will do a blng initialisation (ie. initialise eckit, regardless.) We don't have a use for that just yet, but will soon.

[pmaciel]

Note that this version already supports ORCA and FESOM grids (!), among other functionality that is difficult to check at the moment, so I can call grib_get_data on a GRIB and it will download the necessary files and run properly.

[iainrussell]

This test did not work for me, so I pushed some changes, which now makes the detection of eckit and test of options work for me. Have a look and see what you think!

[pmaciel]

I've changed the detection a little to use HAVE_ instead of ENABLE_ for handling ecbuild features, and directly testing the intended presence of the target (eckit_geo) as a required condition

[pmaciel]

I do find it a bit confusing that there is mixed use of ecbuild_find_package and find_package, but that's I suppose something different to discuss

[pmaciel]

We won't need to call: eckit::Main::initialise(argc, argv); in the tools. As the tools will not need to change. We are only changing the implementation of the src/geo_iterators

Using eckit::Main::initialise is a requirement on eckit, to enable access to env variables, initialise logging channels, initialise library(ies) from files, etc..

eckit::Main::initialise is as close as possible to the main(argc, argv), so for command-line tools it should be placed right there, but even more, that every eckit-based command-line tool should derive from eckit::Tool that does this at construction, but the change here would be much bigger.

Now, the other change if eccodes is to be used strictly as a library (which is definitely the general case), I've added a one-time static initialisation right before eckit::geo is/would be needed, but it won't take into account any comand line options and be inconsistent with eckit. This is on line 124 of this file: src/geo_iterator/grib_iterator.cc. This is the absolutely minimum impact, smallest change that can be done to still be consistent, even if not fully eckit-compliant.

[pmaciel]

This is a problem. It will be difficult to remove the usage of the eckit ASSERT macro from not only eckit, atlas and mir, but also metkit, pgen, mars-client, mars-client-c++ (and very likely metview Regrid). Internally, it calls global namespace "Assert" which clashes with the same symbol in eccodes.

In my opinion, libraries such as eckit should NOT export any macros, but this one does. And so does eccodes, and so there is a conflict. I believe the changin of this function in "the right way" is a very large refactoring which should be out of scope of this one.

[pmaciel]

Anyway, has everyone contributed their opinion? We should not maintain open PRs for long

[shahramn]

Please also add the try/catch around any code that calls:
1/ eckit
2/ STL