Change to standard CMake way of handling parameter constants instead of a .prm file about nceplibs-bufr HOT 15 CLOSED

bufrlib.prm is read and used in the build of the library and cannot/should not be removed.

from nceplibs-bufr.

Why is it named .prm instead of .inc?

Also, where in CMakeLists.txt is it referenced?

from nceplibs-bufr.

No idea. @jbathegit may know the history.

Lines 9-16 src/CMakeLists.txt

from nceplibs-bufr.

OK, here's what CMakeLists.txt is doing:

# Extract BUFRLIB parameters
file(READ bufrlib.prm _bufrlib_prm_str)
foreach(_var IN ITEMS MAXNC MXNAF)
    if(_bufrlib_prm_str MATCHES "${_var} = ([0-9]+)")
      list(APPEND c_defs ${_var}=${CMAKE_MATCH_1})
    else()
      message(FATAL_ERROR "Unable to parse variable ${_var} value from file: src/bufrlib.prm")
    endif()
endforeach()

So this is apparently some way to set variables in CMake?

from nceplibs-bufr.

This was done in the build scripts and I simply translated that to cmake.
If there is an alternative or better way, please let me know.

from nceplibs-bufr.

Is the goal to get a cmake variable set like this:
MAXNC = 600

In other words, if we had:
set(MAXNC 600)

would that be what we are trying to achieve?

from nceplibs-bufr.

The information in the bufrlib.prm file is indeed needed. It contains global parameters that are needed within numerous Fortran subprograms throughout the library, and in particular two of them (i.e. MAXNC and MXNAF) always need to be identically defined within several C subprograms. That's what the snippet in CMakeLists.txt is doing (and what the makebufrlib.sh script before it was doing) - basically ferreting out the value of each of those two parameters from within bufrlib.prm at compile time, and then setting them as define flags to be input to the C compiler. The idea is that the only place these values are ever hardcoded is in the bufrlib.prm file, and the define flags for C are always automatically generated at compile time based on the values listed within bufrlib.prm. So we don't want to just do a "set(MAXNC 600)", unless you can do it in a way that always extracts the actual value from bufrlib.prm at compile time rather than just hardcoding it.

from nceplibs-bufr.

OK, so how about we make these numbers CMake options, with defaults as now in the .prm file.

Then we use variable substitution to generate the header files that need these values. Seems like one .h file should be enough, then all the other C code can just include it. If there's a need for these values in Fortran too, then we also have a .inc file.

So in the repo we will have a file like:
header.h.in

And then in that file there will be a define like this:

#define MAXNC @MAXNC@

Then CMake will generate a header.h file, substituting the value of MAXNC, using a command like this:

CONFIGURE_FILE(${CMAKE_CURRENT_SOURCE_DIR}/header.h.in ${CMAKE_CURRENT_BINARY_DIR}/header.h @ONLY)

This is how this is handled in all other cases, so we need to be like everyone else, instead of developing our own solutions. This is a well-solved problem, and does not require an original solution from us.

This meets the requirement of setting these values in one place (the CMakeLists.txt file) and having it automatically propagate to all the code files that need it.

from nceplibs-bufr.

A couple of questions:

1. First off, yes we absolutely need these MAXNC and MXNAF values defined in Fortran. So are you suggesting changing the filename from bufrlib.prm to bufrlib.inc. We could certainly do that, but then we need to correspondingly change all of the Fortran source files which currently contain an "INCLUDE 'bufrlib.prm'" directive, to make that "INCLUDE 'bufrlib.inc'" instead.
2. What about the other parameters MXS and MXCNEM? Those don't need to be defined within any of the C code, so we don't need any special preprocessor commands to generate those values in C define flags like we do for MAXNC and MXNAF, but again they still need to be available as global parameters within the Fortran source.
3. What about BMISS, which isn't a parameter (i.e. global constant) but which instead is a global variable. That as well also needs to be included in all of the Fortran source files, and as a single, shared global variable (which is why it's in a COMMON block), vs. being separately defined and allocated as a local variable within each individual Fortran source file.

If we can meet all of these criteria, and still have both MAXNC and MXNAF defined in exactly one place, then I'm fine with making changes. Otherwise, we could easily break the overall functionality of the library.

from nceplibs-bufr.

If we change the file to bufrlib.inc, and change all includes to that, then we would have a file in the repo bufrlib.inc.in. That file would have @MaxNC@ and any other substitutions. The .in file can also contain constants and variables that are not touched by CMake, so they will continue to work fine.

from nceplibs-bufr.

Will include a solution for this in the next pull request

from nceplibs-bufr.

OK, great; please submit pull requests incrementally, instead of in a big batch that's hard to review.

Also, before any more changes in the code, I would like to know that code is being tested. Run the code coverage analysis and see if the code you want to modify is run by any tests. If not, add tests in a separate PR before you do any changes to the code.

from nceplibs-bufr.

OK, I'll go ahead and submit a pull request for what I've got so far. I haven't made any changes to source code thus far other than changing bufrlib.prm to bufrlib.inc and using set variables within CMake to propogate parameter values from Fortran to C within bufrlib.h. I've tested everything successfully with the current tests, but I haven't dug into the code coverage analysis tool yet, and I also haven't added any new functionality in this bundle so I haven't added any new tests at this time. I'm mostly focusing on documentation right now, so please also have a look at my doxygen updates for openbf.F and bvers.f(.in) and let me know what you think.

from nceplibs-bufr.

Sounds great. Are you going to put your doc changes up as a PR?

from nceplibs-bufr.

The doc changes show up in the build directory under docs/html. What I've been doing to look at them is to tar up that entire directory and then put it on the emcrzdm server where I can look at everything as html via a browser. If you'd like to do that as well as part of your review (besides just looking at the doxygen markdown in the src files ;-), then just let me know and I'll share the URL with you.

from nceplibs-bufr.