Skip to content
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.

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

Gsi update #8

Merged
merged 38 commits into from
Jul 13, 2020
Merged

Gsi update #8

merged 38 commits into from
Jul 13, 2020

Conversation

mark-a-potts
Copy link
Contributor

This PR references https://github.com/NOAA-EMC/NCEPLIBS/issues/81

I have made updates that I think are consistent with the other NCEPLIBS in the umbrella build as a starting point. I also introduced an approach to compiler-independent flags with variables set for functionality (e.g. using 8 byte reals) and flags set for each supported compiler (GNU and Intel, currently) based on the way that NASA does things for GEOS. I am open to not using that at this point, but I think if we can get a common set of flags across NCEP, it will simplify building with various compilers going forward.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@kgerheiser
Copy link
Contributor

I kinda like the CMake flags thing.

Do you think we should put them into the CMakeModules repo and maintain them centrally?

@kgerheiser
Copy link
Contributor

kgerheiser commented Jun 25, 2020

Do you know what the purpose of all these variations of the library with _DA are? It really makes the build system confusing.

Otherwise it built for me.

CMakeLists.txt Outdated Show resolved Hide resolved
@mark-a-potts
Copy link
Contributor Author

Just pushed an update that addresses most of these issues. I am not sure what to do with all the different "kinds". The static allocation does not seem to work with GNU, so that build defaults to dynamic for everything. Is there really a need for both Dynamic and Static allocation builds? I believe both 4 byte and double are used, but not sure about any of the others.

@DusanJovic-NOAA
Copy link

I asked several times NCEPLIBS group do we really need _8 libraries. Do we know for sure that there are applications that are built with all integers being promoted to 8 bytes. Why would anyone need all integers to be 64 bits wide. I think we should revisit that and hopefully stop building libraries nobody needs.

Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mark-a-potts
The NCEPLIBS have gone through an iteration where all the libraries are consistent in their structure.
Please use one of them to follow as a guide.

  • Do not glob
  • move library creation into src
  • remove extra files that are added and explicitly define those where needed. e.g. GNU.cmake, osx_extra_macros.cmake

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/GNU.cmake Outdated Show resolved Hide resolved
cmake/Intel.cmake Outdated Show resolved Hide resolved
cmake/PackageConfig.cmake.in Show resolved Hide resolved
cmake/osx_extras.cmake Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some comments went unaddressed

src/CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/PackageConfig.cmake.in Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/list_of_files.cmake Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
mark-a-potts and others added 3 commits June 25, 2020 16:07
@aerorahul
Copy link
Contributor

Here is an explanation from @jbathegit (Jeff Ator) on the differences between the DA and "non"DA variants of the bufr library:

The only variants in the latest version 11.3.0 of the library are *4 (4-byte reals and 4-byte ints), *8 (8-byte reals and 8-byte ints) and *d (8-byte reals and 4-byte ints), and then there's a DA (dynamic allocation) version of each, for a total of 6 library builds. I have no idea which application programs are using which builds on the WCOSS or elsewhere; rather, we've just supported all of the different variant builds for 20+ years (going back to the advent of the library itself). But the same version of the source code can be used to build any of the variants, using switches in the make script and conditional compile directives in the source code.

When an application program is linked to a DA build of the BUFRLIB, you do have to make additional calls to function ISETPRM to tell the library how large you want to set certain parameters and array sizes (that's kind of the whole point of being able to dynamically allocate ;-). The whole point of the DA builds is that it gives flexibility to the end user if, e.g. they have some ginormous BUFR messages, or messages containing unusually large bitmaps or other structures that might otherwise cause a memory fault when using the SA builds which have pre-set array sizes and other parameters. If users are fine with those settings, then there's no reason to use a DA build because then you end up paying an extra run-time price to do the dynamic memory allocation, and thus you'd be better off just using a corresponding SA build.

Now, having said all that, you can link a DA build to an application program without making any calls to ISETPRM. As explained in the documentation for this function (see https://emc.ncep.noaa.gov/emc/pages/infrastructure/bufrlib/docs/toc/other/#isetprm), if an application program doesn't make any calls to this function, then the library defaults to using the same parameter and array sizes as for the corresponding SA build. In other words, you only need to call ISETPRM if you want to change one or more parameters from their default settings; otherwise, there's no need to call this function, and then the only difference would be that you'd just have to pay the additional aforementioned price of having all of your memory allocation done at run-time (in the DA build) vs. at compile-time (in the SA build).

aerorahul and others added 2 commits July 8, 2020 15:45
…definitions. add missing defitions from bufrlib.prm

Change-Id: I368810281afc1089ce75afcbe3011afb176bc8f5
consistent with other NCEPLIBS.
@aerorahul aerorahul marked this pull request as ready for review July 9, 2020 19:07
Copy link
Contributor

@aerorahul aerorahul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tested with a standalone read of bufr files with Intel and GNU.
Tested on Orion and macOS

Copy link
Contributor

@kgerheiser kgerheiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't attest to the functionality, but it builds and the CMake looks good

@mark-a-potts mark-a-potts merged commit 252c90c into NOAA-EMC:develop Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants