Changes for Blast #92
Conversation
… until we figure out another approach.
… as well until we figure out another approach." This reverts commit bf34257.
| if (CMAKE_BUILD_TYPE STREQUAL "Debug") | ||
| set(${lib_name}_libs libqhullstatic_d.a) | ||
| else() | ||
| set(${lib_name}_libs libqhullstatic.a) | ||
| endif() |
There was a problem hiding this comment.
I can't find anything in the README for qhull that mentions libqhullstatic_d.a, I only see libqhullstatic_r.a, but I'm assuming it exists when you need to build this in debug. The TPL scripts currently build all of our TPLs as release by default. Do you find you need to build local debug TPLs like this often?
There was a problem hiding this comment.
In Blast when we build debug, all the packages are built in debug, including qhull. When debugging, it makes things much easier, since you can dive into the third-party libraries. I can't remember where I say libqhullstatic_d, but if I run into it again I'll send you a link.
| libconduit.a | ||
| libconduit_blueprint.a | ||
| libconduit_relay.a |
There was a problem hiding this comment.
I like this fix over changing the library rpaths. We will need to rebuild the LC TPL directories first before merging this however.
There was a problem hiding this comment.
This is also how Blast installs the libraries and is consistent with what axom does in Spheral. I'm not sure why the defaults are different between the two packages.
| // typedef NodeListRegistrar<Dimension> Registrar; | ||
| // Registrar& registrar = Registrar::instance(); | ||
| // CONTRACT_VAR(registrar); | ||
| // REQUIRE((int)numNodesRemoved.size() == registrar.numNodeLists()); |
There was a problem hiding this comment.
Any reason to keep this around?
There was a problem hiding this comment.
No, no reason to keep it around. I can remove it.
| // auto& registrar = NodeListRegistrar<Dimension>::instance(); | ||
| // REQUIRE((int)numNodesRemoved.size() == registrar.numNodeLists()); |
There was a problem hiding this comment.
I noticed when building in debug w/ the C++ libs as static that this would fail, the fix for registrar is addressed in an upcoming PR but I'm wondering if you saw this fail too?
There was a problem hiding this comment.
It built fine for me in debug mode. Maybe it is a case where the library name changes or something as is the case for qhull?
The largest change in this is to include the standard NodeLists in the connectivity calculation. There are several other changes here that support using a DataBase with different NodeLists than the Registrar, that add missing headers, and that make conduit build using static libraries similarly to axom.