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

Eleaver rpcmodel hdf5 #255

Merged
merged 3 commits into from
May 22, 2020
Merged

Eleaver rpcmodel hdf5 #255

merged 3 commits into from
May 22, 2020

Conversation

eleaver
Copy link
Contributor

@eleaver eleaver commented May 21, 2020

  1. Made ~ossimRpcModel() dtor public rather than protected. I assume this had been an oversight?
  2. CMakeLists.txt:
    a. Added an explicit OPTION(BUILD_OSSIM_HDF5_SUPPORT) that may be set from command line
    b. To improve MPI support, added
    include_directories( ${MPI_CXX_INCLUDE_PATH} ${MPI_CXX_INCLUDE_DIRS} )
    include_directories( ${MPI_C_INCLUDE_PATH} ${MPI_C_INCLUDE_DIRS} )
    These are in addition to the extant include_directories( ${MPI_INCLUDE_DIR} )
    MPI_INCLUDE_DIR is not set by my /usr/share/cmake/Modules/FindMPI.cmake, but might be by someone else's.
    c. Added some general CMake boilerplate to enable RPATH support by default. I tripped over this when I configured ossim with MPI and tried to load a shared library from a client application. It was easier to add RPATH to ossim's CMakeLists.txt once, than try to maintain disparate client LD_LIBRARY_PATHs in perpetuity; I hope you can accept it.

I've tested this build on Fedora-32, CentOS-7, and CentOS-8.
a. Fedora-32 and CentOS-8's /usr/share/cmake/Modules/FindMPI.cmake set MPI_C_INCLUDE_DIRS, while CentOS-7 sets MPI_C_INCLUDE_PATH. Same for CXX.
b. HDF5 doesn't build on CentOS-7 as it's /usr/include/H5Object does not define getObjName().
c. Otherwise CentOS-7 builds fine, with the caveate that on CentOS-7 one might need to manually edit /usr/include/jsoncpp/json/value.h and change "bool operator!() const;" to "bool operator!() const { return isNull(); };" -- at least with epel jsoncpp-devel version 0.10.5.

I've attached my configuration script, ConfigureOssim.sh.txt
ConfigureOssim.sh.txt

2. Added an explicit OPTION(BUILD_OSSIM_HDF5_SUPPORT "Set to ON to build OSSIM with HDF5 support." OFF) to CMakeLists.txt
3. To improve MPI support, to CMakeLists.txt added
   include_directories( ${MPI_CXX_INCLUDE_DIRS} )
   include_directories( ${MPI_C_INCLUDE_DIRS} )
4. As part of MPI but generally applicable, added CMakeLists.txt boilerplate to enable RPATH support as default.
@gpotts
Copy link
Member

gpotts commented May 21, 2020 via email

@eleaver
Copy link
Contributor Author

eleaver commented May 21, 2020

I've now built this PR on Ubuntu 18.04 using MPICH. An updated ConfigureOssim.sh is attached.
ConfigureOssim.sh.txt

@gpotts
Copy link
Member

gpotts commented May 22, 2020

Hello Ed. Yea, originally just ran out of time. The destructor was hidden for we were not going to allow any explicit deletes but instead use referenced pointers. I wanted to migrate our old ossimRefPtr to use the new standards but that rippled and never had a chance to migrate. We will keep the move of the destructor to public until we can migrate over to an official ref pointer standard.

@gpotts
Copy link
Member

gpotts commented May 22, 2020

Thank you for the pull request. It looks good and will merge everything into the baseline.

@gpotts gpotts merged commit 50acf3a into ossimlabs:dev May 22, 2020
@gpotts
Copy link
Member

gpotts commented May 22, 2020 via email

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.

2 participants