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

Prune export libraries and package lists #60

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Prune export libraries and package lists #60

wants to merge 11 commits into from

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Apr 8, 2015

This pull request cleans up the cluttered export variables.

In TribitsWriteClientExportFiles.cmake, great lengths are gone to capture the entire dependency tree of every single package. None of this is actually necessary: Much of the dependency handling can be done by CMake itself. There is much room for cleaning up the export functionality, and this pull request makes a start by creating leaner export files.

Fixes bug #32.

This commit makes sure that the exported *_LIBRARIES variable only contains
libraries from the current package, not all of its dependencies. The dependency
management is done my CMake's target export files already.

The same goes for the TPL variables.
Before, each INCLUDE() dependency was listed individually.

This commit significantly shortens the export files.
This variable is intrinsic to CMake since 2.8.3.
We really only need the direct dependencies of the respective package.
@bartlettroscoe
Copy link
Member

Nico,

Since I don't currently have any projects that use Config.cmake files and there is very little automated testing for them in TriBITS, I am reluctant to merge this branch without further testing against. In particular, we have to know if this works with Albany. You can access Albany at:

 https://github.com/gahansen/Albany

@nschloe
Copy link
Contributor Author

nschloe commented Apr 10, 2015

I've been involved with Albany before and will look at it. First thing I can say by looking at https://github.com/gahansen/Albany/blob/master/CMakeLists.txt is that I wonder how this ever worked without setting project(Albany CXX) (it doesn't). I'll work at it...

@nschloe
Copy link
Contributor Author

nschloe commented Apr 10, 2015

Looks like fixing Albany won't be too easy, see, e.g., https://github.com/gahansen/Albany/issues/2.

@nschloe
Copy link
Contributor Author

nschloe commented Apr 12, 2015

I managed to compile, link, and test Albany and Nosh successfully against this without related changes in the consumer code. Note that Albany's build system contained some other errors fixes by https://github.com/gahansen/Albany/pull/5.
Another large open-source project based on Trilinos I could think of is https://github.com/lifev/lifev.

@bartlettroscoe
Copy link
Member

Nico,

I just posted a new Issue #63 to extend TriBITS in a pretty significant way. I would like to discuss with you how the <Package>Config.cmake files can fit into this plan. Do you know of a good documentation source explaining all the details of how <Package>Config.cmake files are supposed to work? I can't seem to find good documentation.

@nschloe
Copy link
Contributor Author

nschloe commented Apr 14, 2015

@bartlettroscoe
Copy link
Member

I will review this further and see how this looks. If it looks good, I will merge and push.

bartlettroscoe pushed a commit to bartlettroscoe/TriBITS that referenced this pull request May 7, 2015
…BITS TriBITSPub#60)

This trims down the generated <Package>Config.cmake files to only contain the
direct libraries and include dirs for each SE package.

This commit is a squash of several smaller commits shown below.

1) start cleaning up the export files

2 export files: INCLUDE() dependencies in FOREACH loop

3) export files: remove redundant definition of CMAKE_CURRENT_LIST_DIR

This variable is intrinsic to CMake since 2.8.3.

4) don't use ALL dependencies in PACKAGE_LIST

We really only need the direct dependencies of the respective package.

5) typo in variable name

6) fix for Makefile exports, adapt cmake export tests

7) fix unit test

8) fix unit test 125

9) fix unit test 127
@bartlettroscoe
Copy link
Member

I have done an initial review. I have a few questions about how this is supposed to work and there needs to be a few test cases added to make me feel comfortable merging in these changes (see the 'ToDo' list at the bottom of this comment). I am willing to help with the todos but I feel like I need to understand what is going on at this point (expecially given the expanded importance of the <Package>Config.cmake files with the big refactoring and extensions in #63).

Let me know if when you might have time to discuss some of this some.


Detailed Review:

Reviewing the pull request ...

First I checkout the branch and rebase it on top of origin/master:

$ cd TriBITS/
$ git fetch origin
$ git fetch nschloe
$ git checkout -b prune-export-libraries nschloe/prune-export-libraries
$ git rebase origin/master

I then pushed that branch to my github repo:

To [email protected]:bartlettroscoe/TriBITS
 * [new branch]      prune-export-libraries -> prune-export-libraries-60

I then look at the files that are changed:

[8vt@u235 TriBITS (prune-export-libraries)]$ git diff --name-status HEAD ^origin/master
M       test/core/ExamplesUnitTests/CMakeLists.txt
M       test/core/TribitsWriteClientExportFiles_UnitTests.cmake
M       tribits/ci_support/TribitsDependencies.py
M       tribits/core/installation/TribitsPackageConfigTemplate.cmake.in
M       tribits/core/package_arch/TribitsWriteClientExportFiles.cmake
M       tribits/python_utils/generic-looping-demon.py
M       tribits/python_utils/mailmsg.py
M       tribits/python_utils/mockprogram.py

The changes to the files:

M       tribits/ci_support/TribitsDependencies.py
M       tribits/python_utils/generic-looping-demon.py
M       tribits/python_utils/mailmsg.py
M       tribits/python_utils/mockprogram.py

where only to change '/usr/bin/' to '/usr/bin/env'. I tested this on my machine and it seemed to work.

The remaining changed files are:

M       test/core/ExamplesUnitTests/CMakeLists.txt
M       test/core/TribitsWriteClientExportFiles_UnitTests.cmake
M       tribits/core/installation/TribitsPackageConfigTemplate.cmake.in
M       tribits/core/package_arch/TribitsWriteClientExportFiles.cmake

These files were changed in bunch of commits:

$ git log --oneline HEAD ^origin/master -- test/ tribits/core/
a8cb4fc fix unit test 127
93ba136 fix unit test 125
1d7c0f0 fix unit test
cd428c3 fix for Makefile exports, adapt cmake export tests
3c604a9 typo in variable name
b828e7a don't use ALL dependencies in PACKAGE_LIST
d58fec7 export files: remove redundant definition of CMAKE_CURRENT_LIST_DIR
4de6634 export files: INCLUDE() dependencies in FOREACH loop
45896d4 start cleaning up the export files

These are not atomic commits so it would be good to squash these together before pushing. When I do that, I will make Nico the author so that he maintains credit (or takes the blame :-), whatever the case).

Looking at the changes themselves with:

$ git diff -p HEAD ^origin/master -- test/ tribits/core/

I can see the effects of the change just from looking at the automated tests:

-      "WithSubpackages_LIBRARIES = 'pws_c.pws_b.pws_a.simplecxx'"
-      "WithSubpackages_TPL_INCLUDE_DIRS = '.+/tribits/examples/tpls/HeaderOnlyTpl'"
+      "WithSubpackages_LIBRARIES = 'pws_c.pws_b.pws_a'"
+      "WithSubpackages_TPL_INCLUDE_DIRS = ''"
...
-      "WithSubpackages_PACKAGE_LIST = 'WithSubpackagesC.WithSubpackagesB.WithSubpackagesA.SimpleCxx'"
-      "WithSubpackages_TPL_LIST = 'HeaderOnlyTpl'"
+      "WithSubpackages_PACKAGE_LIST = 'WithSubpackagesA.WithSubpackagesB.WithSubpackagesC'"
+      "WithSubpackages_TPL_LIST = ''"

It seems the export files are now only listing the direct SE package dependencies and only the libraries owned by that given SE package.

We also see this change in the unit tests for the generated Config.cmake files:

diff --git a/test/core/TribitsWriteClientExportFiles_UnitTests.cmake b/test/core/TribitsWriteClientExportFiles_UnitTests.cmake
index 5426cd0..210a391 100644
--- a/test/core/TribitsWriteClientExportFiles_UnitTests.cmake
+++ b/test/core/TribitsWriteClientExportFiles_UnitTests.cmake
@@ -129,12 +129,12 @@ FUNCTION(UNITTEST_WRITE_SPECIALIZED_PACKAGE_EXPORT_MAKEFILE_RTOP_BEFORE_LIBS)
       "SET.RTOp1_CONFIG_INCLUDED TRUE."
       "SET.RTOp1_INCLUDE_DIRS .teuchos/core/include.teuchos/numeric/include.."
       "SET.RTOp1_LIBRARY_DIRS .teuchos/core/src.teuchos/numeric/src.."
-      "SET.RTOp1_LIBRARIES .teuchoscore.teuchosnumeric.."
-      "SET.RTOp1_TPL_INCLUDE_DIRS .lapackhpath/include.blaspath/include.."
-      "SET.RTOp1_TPL_LIBRARY_DIRS .lapackhpath/lib.blashpath/lib.."
-      "SET.RTOp1_TPL_LIBRARIES .lapackpath/lib/liblapack.a.blaspath/lib/libblas.a.."
-      "SET.RTOp1_PACKAGE_LIST .Teuchos.."
-      "SET.RTOp1_TPL_LIST .LAPACK.BLAS.."
+      "SET.RTOp1_LIBRARIES .."
+      "SET.RTOp1_TPL_INCLUDE_DIRS .."
+      "SET.RTOp1_TPL_LIBRARY_DIRS .."
+      "SET.RTOp1_TPL_LIBRARIES .."
+      "SET.RTOp1_PACKAGE_LIST Teuchos."
+      "SET.RTOp1_TPL_LIST .."
     )

This is the generated list of libraries before the package 'RTOp' libraries are defined. The unit test for the generated export makefiles is unchanged.

The unit tests for after the package 'RTOp' libraries are defined shows the diff:

@@ -207,12 +207,12 @@ FUNCTION(UNITTEST_WRITE_SPECIALIZED_PACKAGE_EXPORT_MAKEFILE_RTOP_AFTER_LIBS)
       "SET.RTOp2_CONFIG_INCLUDED TRUE."
       "SET.RTOp2_INCLUDE_DIRS .rtop/include.teuchos/core/include.teuchos/numeric/include.."
       "SET.RTOp2_LIBRARY_DIRS .rtop/src.teuchos/core/src.teuchos/numeric/src.."
-      "SET.RTOp2_LIBRARIES .rtop.teuchoscore.teuchosnumeric.."
-      "SET.RTOp2_TPL_INCLUDE_DIRS .lapackhpath/include.blaspath/include.."
-      "SET.RTOp2_TPL_LIBRARY_DIRS .lapackhpath/lib.blashpath/lib.."
-      "SET.RTOp2_TPL_LIBRARIES .lapackpath/lib/liblapack.a.blaspath/lib/libblas.a.."
-      "SET.RTOp2_PACKAGE_LIST .RTOp.Teuchos.."
-      "SET.RTOp2_TPL_LIST .LAPACK.BLAS.."
+      "SET.RTOp2_LIBRARIES .rtop."
+      "SET.RTOp2_TPL_INCLUDE_DIRS .."
+      "SET.RTOp2_TPL_LIBRARY_DIRS .."
+      "SET.RTOp2_TPL_LIBRARIES .."
+      "SET.RTOp2_PACKAGE_LIST Teuchos."
+      "SET.RTOp2_TPL_LIST .."
     )

The list of TPLs is empty because the package RTOp has no direct TPL dependencies.

The rest of the unit tests are untouched, including the tests that check the export makefiles.

However, I feel like these unit tests are now somewhat lacking because they don't show TPL dependencies anymore.

Looking at the changes in the TriBITS code itself, summarized as:

git diff --stat HEAD ^origin/master -- tribits/core/
 .../TribitsPackageConfigTemplate.cmake.in          |   12 ++--
 .../TribitsWriteClientExportFiles.cmake            |   60 ++++++++++++++------
 2 files changed, 48 insertions(+), 24 deletions(-)

the changes look very reasonable given the goal of removing implicit upstream dependencies from the Config.cmake files.

It looks like there might be a minor (negative) performance impact for this change.

However, if you only generate <Package>Config.cmake files and not Makefile.export.<Package> files, then it looks like we might be able to avoid having to generate the variables ${PACKAGE_NAME}_FULL_ENABLED_DEP_PACKAGES which is quite expensive. I can look into this later.

Now to test this with CASL VERA building the CASL_MOOSE package (which currently requires the generation of a Makefile.export.* file).

I got the branch with:

$ cd VERA/TriBITS/
$ git fetch rab_github
$ git checkout --track rab_github/prune-export-libraries-60

I then configured, built, and tested with:

$ cd ???/
$ rm -r CMake*

$ time ./do-configure -DVERA_ENABLE_CASL_MOOSE=ON &> configure.out

real    4m44.160s
user    2m43.525s
sys 0m57.150s

$ time make -j16 &> make.out

real    41m35.636s
user    253m8.741s
sys 68m46.623s

$ time ctest -j16 &> ctest.out

real    1m3.834s
user    4m35.102s
sys 0m4.290s

That built and returned all passing tests:

100% tests passed, 0 tests failed out of 6

Label Time Summary:
CASL_MOOSE    =  79.91 sec

I then squashed this down to two commits (with git rebase -i origin/master):

b73d119 "Prune export libraries and package lists from Config.cmake files (TriBITS #60)"
Author: Nico Schlömer <[email protected]>
Date:   Wed Apr 8 10:43:26 2015 +0200 (20 minutes ago)

M       test/core/ExamplesUnitTests/CMakeLists.txt
M       test/core/TribitsWriteClientExportFiles_UnitTests.cmake
M       tribits/core/installation/TribitsPackageConfigTemplate.cmake.in
M       tribits/core/package_arch/TribitsWriteClientExportFiles.cmake

bb71ab0 "/bin/env  ->  /usr/bin/env"
Author: Nico Schlömer <[email protected]>
Date:   Thu Apr 9 08:33:18 2015 +0200 (20 minutes ago)

M       tribits/ci_support/TribitsDependencies.py
M       tribits/python_utils/generic-looping-demon.py
M       tribits/python_utils/mailmsg.py
M       tribits/python_utils/mockprogram.py

The commit bb71ab0 "/bin/env -> /usr/bin/env" has nothing to do with the export makefiles so I will cherry-pick that commit and just push it right away ... After pushing that commit, and rebasing on top of origin/master, the remaing commit is just:

fb5fdb8 "Prune export libraries and package lists from Config.cmake files (TriBITS #60)"
Author: Nico Schlömer <[email protected]>
Date:   Wed Apr 8 10:43:26 2015 +0200 (28 seconds ago)

M       test/core/ExamplesUnitTests/CMakeLists.txt
M       test/core/TribitsWriteClientExportFiles_UnitTests.cmake
M       tribits/core/installation/TribitsPackageConfigTemplate.cmake.in
M       tribits/core/package_arch/TribitsWriteClientExportFiles.cmake

I pushed this to the updated branch:

To [email protected]:bartlettroscoe/TriBITS
 + 3b670a8...fb5fdb8 prune-export-libraries -> prune-export-libraries-60 (forced update)

Looking at the generated <Package>Config.cmake files, I see that they are including all of the upstream package <UpstreamPackage>Config.cmake files, not just the ones for the direct upstream packages. For example, for the generated file WithSubpackagesConfig.cmake as part of the test TriBITS_TribitsExampleProject_ALL_ST_NoFortran, we see only the direct packages listed:

## The packages enabled for this project                                                                                                                       
SET(WithSubpackages_PACKAGE_LIST WithSubpackagesA;WithSubpackagesB;WithSubpackagesC)                                                                           

but we see the includes for all of the upstream packages (including the implicit upstream package SimpleCxx):

INCLUDE(".../packages/with_subpackages/c/WithSubpackagesCConfig.cmake")
INCLUDE(".../packages/with_subpackages/b/WithSubpackagesBConfig.cmake")
INCLUDE(".../packages/with_subpackages/a/WithSubpackagesAConfig.cmake")
INCLUDE(".../packages/simple_cxx/SimpleCxxConfig.cmake")

Why would you include the the file SimpleCxxConfig.cmake since it is an indirect package? Would not one of the direct package files like WithSubpacakgesAConfig.cmake include this?

Also, from looking at the updated generated <Package>Config.cmake files, it seems they are worthless (for building client code) without also including the generated files <Package>Targets.cmake and <Package>Targets-*.cmake. How is a client code supposed to know where these files are and which ones to include? If a package does not have any direct libraries, like a parent package with subpackages, these files are not even generated. And these files are not included by the <Package>Config.cmake file. Why not?

ToDo:

  • Determine if the <Package>Config.cmake files should be including all of the <UpstreamPackage>Config.cmake files or just those that are direct dependencies.
  • Determine how to use <Package>Targets.cmake and <Package>Targets-*.cmake from the build and install trees. Is the current setup usable?
  • Add tests that actually builds a client CMake project that includes the file WithSubpackagesConfig.cmake (and perhaps other needed files) to prove it works and demonstrate how to use it. Create one test that uses WithSubpackagesConfig.cmake from the build tree and one that uses it from the install tree.
  • Add unit tests to TribitsWriteClientExportFiles_UnitTests.cmake that generate TeuchosConfig.cmake and make sure the TPLs are listed correctly. The new unit tests that test the generation of RTOpConfig.cmake no longer does this.

@bartlettroscoe
Copy link
Member

Nico,

The page cmake-packages(7) suggests that the <Package>Config.cmake files should include the <Package>Targets.cmake file. It says:

In this case, the ClimbingStatsConfig.cmake file could be as simple as:

include("${CMAKE_CURRENT_LIST_DIR}/ClimbingStatsTargets.cmake")

As this allows downstreams to use the IMPORTED targets.
So the idea is that the <Package>Config.cmake files should be self-contained. A downstream CMake client should just have to include that file and nothing else.

@nschloe
Copy link
Contributor Author

nschloe commented May 9, 2015

<Package>Config.cmake files should include the <Package>Config.cmake

I assume one of the two is supposed to include Target. Which one?

@bartlettroscoe
Copy link
Member

Nico,

Of course you are right. That was a mistake. I updated the comment to say " the <Package>Config.cmake files should include the <Package>Targets.cmake file".

So, do you agree with this? That is what the Kitware documentation says to do and that makes good sense I think. You should only have to include <Package>Config.cmake and the just add the include dirs and link links to you libraries and you should be done.

@nschloe
Copy link
Contributor Author

nschloe commented May 19, 2015

So, do you agree with this?

Yup. The target include line also appears in every config export in the last line.

@nschloe
Copy link
Contributor Author

nschloe commented Nov 21, 2015

@bartlettroscoe, I'd like to bring this back to attention.

@bartlettroscoe
Copy link
Member

Given that all of this export stuff is so poorly tested right now I am hesitant to make such a change. For example, how does this impact the generated Makefile.export. files? But if you are willing to respond to an problems that people encounter, then I will go ahead and rebase and merge this branch.

In any case, all of this will be revisited as part of the major refactoring #63. That is why I did not look into this too much. But that refactoring has been delayed for a long time.

@nschloe
Copy link
Contributor Author

nschloe commented Nov 21, 2015

I believe this PR is somewhat orthogonal to #63. Here, we try to keep to avoid the blowing up of the link line as it currently happens, #63 merges TPLs and packages (which I also strongly support).

so poorly tested

I agree. So let's get some tests in order. :) What would think is a reasonable approach?

@bartlettroscoe
Copy link
Member

so poorly tested

I agree. So let's get some tests in order. :) What would think is a reasonable approach?

TriBITS nor any of my projects uses these TriBITS-generated <Package>Config.cmake files at all so don't know the requirements or expected use cases. Once I get around to the refactoring in #63, these new use cases will depend on the TriBITS-generated <Package>Config.cmake files so I will see what needs to be in these files and how this is supposed to work. Until then, I am not sure.

So you can see there is some existing automated testing for the TriBITS-generated <Package>Config.cmake and Makefile.export.<Package> files I put in a while back defined in the file TriBITS/test/core/ExamplesUnitTests/CMakeLists.txt. See the tests that utilize DummyPackageClientCMakeLists.txt and RunDummyPackageClientBulid.cmake. I don't know if those existing use cases will cover the requirements or not. They might but I am not sure. Again, once I get around to #63, I will know for sure and will be an expert on export files at that point. But I am most concerned about how maintain the Makfile.export.<Package> files in this larger refactoring.

@bartlettroscoe
Copy link
Member

The only conflicts are in the tests file so this should be easy to resolve.

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