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

Decoupling include/link dirs from 'TRIBITS_ADD_LIBRARY' #182

Closed
sethrj opened this issue Mar 2, 2017 · 4 comments
Closed

Decoupling include/link dirs from 'TRIBITS_ADD_LIBRARY' #182

sethrj opened this issue Mar 2, 2017 · 4 comments

Comments

@sethrj
Copy link
Contributor

sethrj commented Mar 2, 2017

I'm working on ForTrilinos with Andrey Prokopenko. I've got SWIG hooked up provisionally into CMake/Tribits: SWIG takes a .i file and generates a .cxx and a .f90 file, which get linked (alongside whatever sources the user provides) into a package library using TRIBITS_ADD_LIBRARY.

There is a problem though, in that SWIG needs the dependency information (include directories) as the library compilation, but that information is only built inside the TRIBITS_ADD_LIBRARY call (but before the actual CMake ADD_LIBRARY call). As a workaround, I'm explicitly calling

    TRIBITS_SORT_AND_APPEND_PACKAGE_INCLUDE_AND_LINK_DIRS_AND_LIBS(
      ${PACKAGE_NAME}  LIB  LINK_LIBS)
    TRIBITS_SORT_AND_APPEND_TPL_INCLUDE_AND_LINK_DIRS_AND_LIBS(
      ${PACKAGE_NAME}  LIB  LINK_LIBS)

before calling SWIG_ADD_MODULE. But I think this hack ends up causing the TRIBITS_ADD_LIBRARY logic to retrace the package dependency graph and possibly even duplicate all the upstream includes.

The only other workaround I can think of is to create a stub TRIBITS_ADD_LIBRARY before the SWIG call, but that ends up unnecessarily doubling the number of libraries in the one package.

Is there a way to decouple the upstream include directory/link traversal from the library creation? Is there a reason this is done inside the "add_library" command as opposed to TRIBITS_PACKAGE or TRIBITS_SUBPACKAGE ?

@bartlettroscoe
Copy link
Member

@SethJ,

So the idea is that you need to call SWIG_ADD_MODULE() to actually generate the C++ header and source files that get passed to ADD_LIBRARY()? Can you show me the full actual code? Do you have this on a GitHub branch that i can look at? I need to see the details before I can suggest a way to address this.

Is there a way to decouple the upstream include directory/link traversal from the library creation? Is there a reason this is done inside the "add_library" command as opposed to TRIBITS_PACKAGE or TRIBITS_SUBPACKAGE ?

Yea, I guess that would make sense. I am not sure why it is done in this way (it has been years since this code has been touched). But if that would solve your problem, can you give this a try in a branch? If you run the native TriBIITS test suite, it will tell you if you have broken anything. If you clone the TriBITS repo, then running the native TriBITS test suite is as easy as:

$ cd TriBITS/
$ mkdir build/
$ cd build/
$ cmake ..
$ make
$ ctest -j12

And you can build Trilinos against this TriBITS version using -DTrilinos_TRIBITS_DIR=<abs-base-path>/TriBITS/tribits. For more details on doing Trilinos and TriBITS co-development, see:

If moving these calls to TRIBITS_PACKAGE_DEF() and TRIBITS_ADD_SUBPACKAGE() (i.e. the same function they both call) does not work for some reason, then I guess we can add a call-back function in the middle of TRIBITS_ADD_LIBRARY() that would be called that would allow you to call your SWIG_ADD_MODULE() in the right place and do what you need. This may not be supper clean to do in CMake but I can think how it can work. But again, I need to see the details before I know for sure.

Is ForTrilinos going to be under automated testing with Trilinos? I think it would be good if it could be. The sooner the better I suspect. That way, any changes to TriBITS will be tested against ForTrilinos before I update the snapshot the TriBITS in Trilinos.

NOTE: Once I do the refactoring in #63, all of this internal library and include dir management code will change pretty dramatically to use TARGET_INCLUDE_DIRECTORIES(). This should simplify this TriBITS code quite a bit, eliminate a lot of it (yea!), and fix other problems. I don't know how that will impact SWIG_ADD_MODULE() but we can cross that bridge when we come to it.

@bartlettroscoe
Copy link
Member

@sethrj, was this the motivation for dropping TriBITS?

@sethrj
Copy link
Contributor Author

sethrj commented Jan 22, 2019

@bartlettroscoe Sorry that I didn't ever see your earlier reply! Looks like the wrong Seth was tagged and I must not have seen the notification of a reply. :(

This is a separate project from our Exnihilo work so it's mostly tangential to our thoughts of moving away from TriBITS. It is related in a way though.

TriBITS' strength as a package architecture is that it's very automatic and full-featured when working within the constraints of its design, but extending or modifying it is almost impossible (for anyone but you, of course). The indirection and complexity of TriBITS (a lot of which is I think necessary for the unit testing) makes it difficult to even pinpoint the source of any unexpected behavior.

Since I wrote this issue, the SWIG situation has actually changed quite a bit: the upstream CMake UseSWIG implementation changed significantly in order to move to newer CMake paradigms (properties of targets and source files as opposed to global/directory properties). I don't even know how the newer script will interact with TriBITS, so we're keeping basically a copy of an old UseSWIG.cmake. Another thing that's been an issue with TriBITS at times is that it is somewhat fixed to older versions of cmake -- for a while it even had a number of TPL scripts that were copies from CMake 2.x.

Anyway, I haven't looked at this issue in a while, and the SWIG stuff seems to be working for the moment so I don't want to mess with it :) I'm going to close the issue. Thanks for the detailed response that I missed!

@sethrj sethrj closed this as completed Jan 22, 2019
@bartlettroscoe
Copy link
Member

@sethrj, thanks for the detailed response. These are all valid concerns. The plan for a long time has been to refactor TriBITS to strip out most of the guts for library and executable linking and use the newer CMake target-based approach. Just recently Trilinos upgraded the minimum CMake version form 2.8.11 to 3.10. That was the major impediment to refactoring TriBITS. Now that the minimum CMake version for TriBITS is 3.10, I think we can move forward with major refactorings and extensions like #63 and #10.

I would like to make sure that the refactored TriBITS can smoothly handle use cases like you describe with SWIG so it I will ping you once we get that effort going.

Hopefully we can preserve the advantages of a package-based architecture for CMake projects and address these and related concerns. Examples like Kitware's own VTK Modules and Google's Catkin shown the clear need for a package-based architecture for large amounts of CMake-based code. It would be a shame to have to give up and just go back to trying to glue together various packages that systems like Spack and CMake ExternalProject have you do (and throws away many of the advantages of CMake, CTest, and CDash).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants