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.

Sign up for GitHub

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

Jump to bottom

Try to clean up logic for finding HDF5 to address Nico's concerns (TriBITS #67) #70

Merged
merged 6 commits into from
May 26, 2015
Merged

Try to clean up logic for finding HDF5 to address Nico's concerns (TriBITS #67) #70

merged 6 commits into from
May 26, 2015

Conversation

bartlettroscoe
Copy link
Member

@nschloe, take a look at this and see if this seems more clear to you. Hopefully this will address you concerns expressed in #67.

Try to clean up logic for finding HDF5 to address Nico's concerns (Tr…
3300ba4 
…iBITS TriBITSPub#67)

This should have *exactly* the same behavior as before but hopefully now will
make it more clear what is happening and why.
@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

Much clearer, thanks!

@bartlettroscoe bartlettroscoe mentioned this pull request May 22, 2015
Closed
@bartlettroscoe
Copy link
Member Author

Excellent. Then, if you don't mind, can you test the updated FindTPLHDF5.cmake for your needs and then I will do the same for CASL VERA?

Then, if that works out, can you update the FindTPLBLAS.cmake

I will test the new FindTPLHDF5.cmake module with

@bartlettroscoe
Copy link
Member Author

What I meant to say (before accidentally hitting 'Tab' and 'Enter') was:

Excellent. Then, if you don't mind, can you test the updated FindTPLHDF5.cmake for your needs and then I will do the same for CASL VERA?

Then, if that works out, can you update the FindTPLBLAS.cmake and FindTPLLAPACK.cmake in #69 to match this?

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

I'm getting Trilinos build errors for this right now. Investigating...

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

With this patch, the output from CMake is

Processing enabled TPL: HDF5 (enabled explicitly, disable with -DTPL_ENABLE_HDF5=OFF)
-- Using FIND_PACKAGE(HDF5 ...) ...
-- Found HDF5: /usr/lib/x86_64-linux-gnu/libhdf5.so;/usr/lib/x86_64-linux-gnu/libpthread.so;/usr/lib/x86_64-linux-gnu/libz.so;/usr/lib/x86_64-linux-gnu/libdl.so;/usr/lib/x86_64-linux-gnu/libm.so

Unfortunately, those libs aren't added on the link line of

libtrilinos_epetraext.so

which results in a link error.

Everything works fine on TriBits::master.

@bartlettroscoe
Copy link
Member Author

I know what the problem is. The issue is that TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() is set up to set include dirs and libs base on the arguments REQUIRED_HEADERS and REQUIRED_LIBS_NAMES.

Let me try to reorganize the file so that it works but is also more clear. I will push the new commit.

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

I'm hanging tight. :)

Move around and fix up so it works (TriBITS TriBITSPub#67)
45d6e1e 
This may not be ideal in Nico's eyes but it works.  I am afraid that to clean
this up more will require some refactoring (and unit testing) to change how
the function TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() works.
@bartlettroscoe
Copy link
Member Author

Nico,

Pull the updated commit and try it. I tested it locally and it should work. Basically, you can't let the variables REQUIRED_HEADERS and REQUIRED_LIBS_NAMES passed into TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES(). That function uses those variables to determine if the TPL is supposed to have include directories or libraries at all. If you leave them (or set them to) null, then it thinks there are no include directories or no libraries, respectively.

I can think of some ways to refactor this code to allow you to leave the variables REQUIRED_HEADERS and REQUIRED_LIBS_NAMES null when FIND_PACKAGE(HDF5 ...) but that will raise some other confusion.

Let me know if this is acceptable or not. Otherwise, we need to talk about this (over WebEx or Google Hangout) some.

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

I just tested it and can confirm it's working. Good to go by me.

@bartlettroscoe
Copy link
Member Author

Are you satisfied with the structure of the file?

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

It's not clear from the file how TriBits finds the libraries if FIND_PACKAGE() doesn't, but hey, I can live with that. The rest of the code is clear and descriptive.

@bartlettroscoe
Copy link
Member Author

It's not clear from the file how TriBits finds the libraries if FIND_PACKAGE() doesn't, but hey, I can live with that. The rest of the code is clear and descriptive.

Okay, how TriBITS finds libraries without FIND_PACKAGE() is something that needs to be better documented. But that has nothing to do with how FIND_PACKAGE() gets used. Therefore, I will test the updated FindTPLHDF5.cmake file some more then push it. I will also update the limited documentation to be consistent.

@nschloe
Copy link
Contributor

nschloe commented May 22, 2015

Sounds good to me.

@nschloe nschloe mentioned this pull request May 22, 2015
Closed
Roscoe A. Bartlett added 4 commits May 22, 2015 17:22
Tweak the documentation just a little (TriBITS TriBITSPub#67)
07c4a72 
Hopefully this will be even a little more clear.

I will now update the TriBITS docuemntation for this approach.
Move Copyright.txt to tribits/ so it gets snapshotted
57bcdac 
When you snapshot just the TriBITS/tribits/ directory, you need to have the
full copyright file.  I am doing this so that I can gut the redundant
copyright in a bunch of the TriBITS files to remove clutter, especially the
more trivial files.
Removing long copyright/license and just point to Copyright.txt
44ce048 
This file is being used in TribitDevelopersGuilde.rst so we don't want to see
this long copyright/license header.
Update documentation to correspond with updated structure (TriBITS Tr…
7f0d946 
…iBITSPub#67)

This structure meets Nico's approval.  Hopefully this will be a little more
clear when FIND_PACKAGE(<tplName> ...) gets called.

I still need to write better documentation for how
TRIBITS_TPL_FIND_INCLUDE_DIRS_AND_LIBRARIES() behaves.  That will come later.
@bartlettroscoe bartlettroscoe merged commit 7f0d946 into TriBITSPub:master May 26, 2015
@bartlettroscoe
Copy link
Member Author

Nico,

I have pushed these changes to the github/master branch and snapshotted to Trilinos in the Trilinos commit:

ommit 543e7f64e94edba66f955fedf1adc593f9d7e79c
Author: Roscoe A. Bartlett <[email protected]>
Date:   Tue May 26 12:40:52 2015 -0400

    Automatic snapshot commit from tribits at d953228

    Origin repo remote tracking branch: 'origin/master'
    Origin repo remote repo URL: 'origin = [email protected]:TriBITSPub/TriBITS'

    At commit:

    commit 7f0d946cf8451a6131d1c9d1ed313170ea84cf53
    Author:  Roscoe A. Bartlett <[email protected]>
    Date:    Fri May 22 17:57:43 2015 -0400
    Summary: Update documentation to correspond with updated structure (TriBITS #67)

A       cmake/tribits/Copyright.txt
M       cmake/tribits/common_tpls/FindTPLHDF5.cmake
M       cmake/tribits/doc/developers_guide/TribitsDevelopersGuide.rst

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bartlettroscoe @nschloe