NOAA-EMC / NCEPLIBS-bacio

This library performs binary I/O for the NCEP models.
Other
2 stars 6 forks source link

Update develop with latest CMake and other changes #14

Closed kgerheiser closed 4 years ago

kgerheiser commented 4 years ago

These changes will serve as a template for all other NCEPLIBS. They will update the develop branch with the latest CMake, add tests, add CI, and some minor CMake tweaks.

Merges release/public-v1 into develop (keeping the old build system too).

Adds @aerorahul's code that changes the way CMake exports its targets by using CMakePackageConfigHelpers and adds namespaces to exported targets (e.g. bacio::bacio_4_static)

Adds option to build static or shared libraries, or both. Defaults to static only.

Adds pfunit tests (testing disabled by default, use -DENABLE_TESTS=ON to enable)

Adds CI with Github Actions on commits and PRs which will build and test the library. No real tests yet, only placeholder. Can easily be added in tests/test_bacio_mod.pf.

kgerheiser commented 4 years ago

What are your thoughts on adding a default install prefix? I don't like how it defaults to /usr/local and I forget to set it a lot.

# set default install path if not provided
if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
 message(STATUS "CMAKE_INSTALL_PREFIX not set, installing to ./install")
  set(CMAKE_INSTALL_PREFIX
      "${CMAKE_BINARY_DIR}/install"
      CACHE PATH "default install path" FORCE)
endif()
aerorahul commented 4 years ago

What are your thoughts on adding a default install prefix? I don't like how it defaults to /usr/local and I forget to set it a lot.

# set default install path if not provided
if(CMAKE_INSTALL_PREFIX_INITIALIZED_TO_DEFAULT)
 message(STATUS "CMAKE_INSTALL_PREFIX not set, installing to ./install")
  set(CMAKE_INSTALL_PREFIX
      "${CMAKE_BINARY_DIR}/install"
      CACHE PATH "default install path" FORCE)
endif()

I don't think that is a good idea. There is no reason to spoon feed every single nuance. In containers where there is usually a single install, it is in the default path. Also, on some personal laptops. This is going against the standard CMake practice. The general user who does not want to look into the CMakeLists.txt will now have to know that the developer changed the default path. Besides, you are placing the install directory inside the build directory. Every option we add is a burden on the user.

kgerheiser commented 4 years ago

Ok

Is this good to go then?

kgerheiser commented 4 years ago

Tested it out and tried building and finding various combinations of static and shared libraries and it worked.