Untangle export files #11

nschloe · 2014-09-11T23:14:46Z

ATTENTION: This pull request is tied to Trilinos bug #6228 which must be resolved first.

This pull request is concerned with the structure of the export files.

Up until now, all package export files included the main project configuration export file to get access to their dependent packages. Also, there was only one target export file that incorporated all targets built by the project.

The present pull request changes this:

Every package export file now has a accompanying target export file.
All packages include the configuration files of their dependencies, and nothing more.

Everything that worked previously still works, in particular the project export files contain the same information as they did before.

One exception is that the pull requires CMake's deprecated SUBDIRS() command not to be used. The reason for this is that we need to be sure now that ADD_LIBRARY() is called before the EXPORT files are installed. SUBDIRS() works unpredictably (or probably rather in unexpected ways).

Fixes bug #1.

This accommodates packages with subpackages (e.g, Trilinos::Teuchos).

Conflicts: package_arch/TribitsWriteClientExportFiles.cmake

This reverts commit 4f36e2c. Explicitly including the subpackages' configurations isn't necessary anymore since commit 56a93b1 which added inclusion of *dependent* packages. Since a package always depends on its subpackages, the functionality remains the same.

nschloe · 2014-09-22T23:09:03Z

The referenced Trilinos bug is gone now, so this is ready to be merged in.

bartlettroscoe · 2014-09-22T23:14:26Z

We need some type of test for export files in TriBITS proper. We have the example TriBITS project like TribitsExampleProject that gets built and tested as part of the TriBITS package build. We can set up a test that configures, builds, and installs TribitsExampleProject and then we can configure and build an example client CMake project that uses the export files.

nschloe · 2014-09-22T23:20:33Z

I suppose if we just FIND_PACKAGE(... REQUIRED) and then MESSAGE(...) some of the variables that should be defined, we're good.
I don't know enough about the TriBITS test system to quickly put something in place.

bartlettroscoe · 2014-09-22T23:24:56Z

Yes, I am thinking that FIND_PACKAGE() and MESSAGE() in a few dummy test *.cmake files would do it. We would then grep the output for expected strings.

What types of use cases should be tested? Including the entire project's Config.cmake file? Including only selected components from Config.cmake? Including the a set of individual packages Config.cmake?

nschloe · 2014-09-22T23:37:47Z

What types of use cases should be tested? Including the entire project's Config.cmake file? Including
only selected components from Config.cmake?

We could do both. First finding all packages à la

FIND_PACKAGE(Belos REQUIRED)
MESSAGE(${Belos_LIBRARIES})

(or whatever the packages are called), then at the end (or in a separate CMake file

FIND_PACKAGE(Trilinos REQUIRED)
MESSAGE(${Belos_LIBRARIES})
MESSAGE(${Trilinos_LIBRARIES})

bartlettroscoe · 2014-09-24T23:50:44Z

Given that these changes touch a lot of parts of TriBITS, I need to merge this in before I continue with my work to improve the configure times for large TriBITS projects. Therefore, I will look over this branch tomorrow see about merging it in (or some modified version of it).

When used in flexible mode, PACKAGE_NAME is passed in and set, it is not just pulled out of the current scope. Therefore, the call to TRIBITS_SET_DEFINE_CMAKE_CURRENT_LIST_DIR_CODE_SNIPPET() had to be moved down. I add a unit test next that protects this.

…BITS TriBITSPub#11) This provides the most basic protection of this functionality. It is not much but it is something.

…SPub#11) Before, the include guard `_INCLUDE` was getting used when used in flexible mode (not what you want) and the name of the last package was getting used in the <ProjectName>Config.cmake file. This would have caused it to conflict with the include guard from the package's include guard. For example, with TribitsExampleProject (TriBITSExProj), TriBITSExProjConfig.cmake was using the include guard PackageWithSubpackages_INCLUDED. That is not good. With this commit, this is fixed. I will not try to add some automated tests for the generated export files.

…b#11) Here I just turned on the generation of the export files but I am not testing them yet. At least this way I can see what these files look like and we can be assured that we are not crashing the configure. That alone would have stopped one of my previous bad pushes.

CMAKE_CURRENT_LIST_DIR is guaranteed from CMake 2.8.3 on, so since the requirement for 2.8.11, we don't need this anymore.

…riBITSPub#11) Here I have updated the test TribitsExampleProject_ALL_PT_NoFortran so that it installs the project into an install directory then it reads in one of the <Package>Config.cmake files from the install dir to make sure that it is setting the right variables. It was a bit of a pain to set up this test but test represents the very first automated test for generating, installing, and reading in <Package>Config.cmake files. I have not added a test for the project-level <Project>Config.cmake file. But we can just copy and paste the files to create a test of the project-level file. There is also no validate test for the Makefile.export.<Package> files but that test would be created in a similar way.

bartlettroscoe · 2014-09-25T14:55:13Z

Nico,

I have done quite a bit of work with this and I think I am ready to merge and push.

Detailed Review:

Reviewing the TriBITS branch nschloe:untangle_export_files ...

First, I get access to Nico's git repo:

$ cd TriBITS
$ git remote add nico-github git@github.com:nschloe/TriBITS 
$ git fetch nico-github

Here are the commits in that branch:

$ git log-oneline nico-github/untangle_export_files ^origin/master

6931092 "move include guard in the export file to the beginning of the file" <nico.schloemer@gmail.com> [Thu Sep 25 00:14:34 2014 +0200] (12 hours ago)
8ad456c "Merge branch 'master' into untangle_export_files" <nico.schloemer@gmail.com> [Fri Sep 19 16:11:31 2014 +0200] (6 days ago)
411a94b "export targets only once per package, not per library" <nico.schloemer@gmail.com> [Thu Sep 11 21:53:11 2014 +0300] (2 weeks ago)
fba17ea "don't explicitly include subpackage configs in export files" <nico.schloemer@gmail.com> [Thu Sep 11 01:42:54 2014 +0300] (2 weeks ago)
1a7830b "Merge branch 'master' into untangle_export_files" <nico.schloemer@gmail.com> [Thu Sep 11 00:55:56 2014 +0300] (2 weeks ago)
56a93b1 "include configuration of dependent packages in export files" <nico.schloemer@gmail.com> [Thu Sep 11 00:54:13 2014 +0300] (2 weeks ago)
a0799db "whitespace changes (avoid superfluous newline in export)" <nico.schloemer@gmail.com> [Thu Sep 11 00:15:31 2014 +0300] (2 weeks ago)
4f36e2c "include subpackage exports in superpackage export files" <nico.schloemer@gmail.com> [Tue Sep 9 22:57:20 2014 +0300] (2 weeks ago)
19f4605 "move installation of export files to add_library()" <nico.schloemer@gmail.com> [Tue Sep 9 18:09:06 2014 +0300] (2 weeks ago)
fad53d3 "don't reference ${PROJECT_NAME}Targets in config file" <nico.schloemer@gmail.com> [Tue Sep 9 14:33:44 2014 +0300] (2 weeks ago)
f4ef684 "install target export files on a per-package basis" <nico.schloemer@gmail.com> [Tue Sep 9 14:29:35 2014 +0300] (2 weeks ago)
609bfc8 "trailing whitespace" <nico.schloemer@gmail.com> [Tue Sep 9 13:05:13 2014 +0300] (2 weeks ago)

The full set of files that are changed is given by:

$ git diff --name-status nico-github/untangle_export_files ^`git merge-base nico-github/untangle_export_files origin/master`
M       installation/TribitsPackageConfigTemplate.cmake.in
M       package_arch/TribitsLibraryMacros.cmake
M       package_arch/TribitsWriteClientExportFiles.cmake

There is just one non-whitespace, non-comment change in the file TribitsLibraryMacros.cmake:

diff --git a/package_arch/TribitsLibraryMacros.cmake b/package_arch/TribitsLibraryMacros.cmake
index 60864c5..9a63408 100644
--- a/package_arch/TribitsLibraryMacros.cmake
+++ b/package_arch/TribitsLibraryMacros.cmake
@@ -585,7 +585,7 @@ FUNCTION(TRIBITS_ADD_LIBRARY LIBRARY_NAME_IN)
         "${CMAKE_INSTALL_PREFIX}/${${PROJECT_NAME}_INSTALL_LIB_DIR}")
       INSTALL(
         TARGETS ${LIBRARY_NAME}
-        EXPORT ${PROJECT_NAME}
+        EXPORT ${PACKAGE_NAME}
           RUNTIME DESTINATION "${${PROJECT_NAME}_INSTALL_RUNTIME_DIR}"
           LIBRARY DESTINATION "${${PROJECT_NAME}_INSTALL_LIB_DIR}"
           ARCHIVE DESTINATION "${${PROJECT_NAME}_INSTALL_LIB_DIR}"

I don't know the impliciations of changing the EXPORT argument but that looks reasonble to me.

The only change in the file TribitsPackageConfigTemplate.cmake.in is removing a comment.

The real changes are all in the one file TribitsWriteClientExportFiles.cmake. Rather than go through all of that line-by-line, I am going to start by just testing it, first to make sure what is currently tested in TribitsExampleProject is still working (and I would assume that it is since the travis-ci build showed this branch as passing).

I added some unit testing for the generation of the Config.cmake files. I also added generation of all the export files for the TribitsExampleProject (TriBITSExProj). I have found a few issues with the version of the code.

First, I see that include guards were added to the XXXConfig.cmake files. This include guard did not work when generating flexible export files (it was just _INCLUDE). Also, when generating the project's Config.cmake file, it was using the include guard from the last package processed. I fixed that so now it uses a unique name which is the prefix name of the varibles.

This is captured in the new commit:

a29bcaf "Fixing include guards in export XXXConfig.cmake files (TriBITS #11)"
Author: Roscoe A. Bartlett <bartlettra@ornl.gov>
Date:   Thu Sep 25 08:02:00 2014 -0400 (36 minutes ago)

M       package_arch/TribitsWriteClientExportFiles.cmake
M       package_arch/UnitTests/TribitsWriteClientExportFiles_UnitTests.cmake

I am noticing a defect right away in the project export files and that is that they are duplicating libraries. For example, in the generated file:

<buiddir>/tribits/doc/examples/UnitTests/TriBITS_TribitsExampleProject_ALL_PT_NoFortran/TribitsExProjConfig_install.cmake

it shows:

SET(TribitsExProj_LIBRARIES "pws_c;pws_b;pws_a;pws_c;pws_b;pws_a;simplecxx")

But we don't see duplicated libraries in the package-specific file:

<buiddir>/tribits/doc/examples/UnitTests/TriBITS_TribitsExampleProject_ALL_PT_NoFortran/packages/package_with_subpackages/PackageWithSubpackagesConfig.cmake

which shows:

SET(PackageWithSubpackages_LIBRARIES "pws_c;pws_b;pws_a;simplecxx")

or in:

/tribits/doc/examples/UnitTests/TriBITS_TribitsExampleProject_ALL_PT_NoFortran/packages/package_with_subpackages/Makefile.export.PackageWithSubpackages
which shows:

PackageWithSubpackages_LIBRARIES= -lpws_c -lpws_b -lpws_a -lsimplecxx

Frankly I don't care if the project-level export files are messed up because none of the projects I care about use them. However, CASL VERA does use the package-level exported makefiles to allow us to build MOOSE as a TriBITS project.

More automated test is needed for the generation of these files is needed. For that, I have created the commit:

c65be90 "Added a validation test for genrated <Package>Config.cmake (TriBITS #11)"
Author: Roscoe A. Bartlett <bartlettra@ornl.gov>
Date:   Thu Sep 25 10:37:52 2014 -0400 (9 minutes ago)

M       doc/examples/UnitTests/CMakeLists.txt
A       doc/examples/UnitTests/DummyPackageClientCMakeLists.txt
A       doc/examples/UnitTests/RunDummyPackageClientBulid.cmake

I have pushed the updated branch to:

To git@github.com:bartlettroscoe/TriBITS
   bc58517..c65be90  untangle_export_files_11 -> untangle_export_files_11

I think this branch is ready to merge and push to TriBITS.

nschloe · 2014-09-25T15:16:26Z

Just because TriBITS requires 2.8.11

We only need CMake 2.8.3 for this functionality, which is now 4 years old.

Just because TriBITS requires 2.8.11 does that imply that users must use the same CMake version?

The situation where this might occur is cross-compiling for a system with CMake older than 2.8.3. I think we can safely neglect this.

bartlettroscoe · 2014-09-25T15:22:45Z

As of commit c65be90, this should now be pushed to the github/master branch. I am working to sync this with Trilinos as we speak. Once this is synced with Trilinos, I will close this pull request. However, I don't know why Github does not know that this has already been merged?

nschloe · 2014-09-25T15:26:39Z

I don't know why Github does not know that this has already been merged?

Not sure. Didn't push the "Merge" button here?

As for the workflow: If you're adding tests to another branch (e.g., master), we could have merge that branch into this one, fix the defects that are revealed by the new tests, and then merge this branch into master. GitHub would have understood better I'm sure.

Once this is synced with Trilinos, I will close this pull request.

Feel free to close now.

bartlettroscoe · 2014-09-25T15:41:32Z

This branch is merged but github does not seem to realize this. I wonder what would happen if I clicked "Merge"?

bartlettroscoe · 2014-09-25T15:41:46Z

Closing.

When used in flexible mode, PACKAGE_NAME is passed in and set, it is not just pulled out of the current scope. Therefore, the call to TRIBITS_SET_DEFINE_CMAKE_CURRENT_LIST_DIR_CODE_SNIPPET() had to be moved down. I add a unit test next that protects this.

…BITS TriBITSPub#11) This provides the most basic protection of this functionality. It is not much but it is something.

…SPub#11) Before, the include guard `_INCLUDE` was getting used when used in flexible mode (not what you want) and the name of the last package was getting used in the <ProjectName>Config.cmake file. This would have caused it to conflict with the include guard from the package's include guard. For example, with TribitsExampleProject (TriBITSExProj), TriBITSExProjConfig.cmake was using the include guard PackageWithSubpackages_INCLUDED. That is not good. With this commit, this is fixed. I will not try to add some automated tests for the generated export files.

…b#11) Here I just turned on the generation of the export files but I am not testing them yet. At least this way I can see what these files look like and we can be assured that we are not crashing the configure. That alone would have stopped one of my previous bad pushes.

…riBITSPub#11) Here I have updated the test TribitsExampleProject_ALL_PT_NoFortran so that it installs the project into an install directory then it reads in one of the <Package>Config.cmake files from the install dir to make sure that it is setting the right variables. It was a bit of a pain to set up this test but test represents the very first automated test for generating, installing, and reading in <Package>Config.cmake files. I have not added a test for the project-level <Project>Config.cmake file. But we can just copy and paste the files to create a test of the project-level file. There is also no validate test for the Makefile.export.<Package> files but that test would be created in a similar way.

nschloe added 10 commits September 9, 2014 13:05

trailing whitespace

609bfc8

install target export files on a per-package basis

f4ef684

don't reference ${PROJECT_NAME}Targets in config file

fad53d3

move installation of export files to add_library()

19f4605

This accommodates packages with subpackages (e.g, Trilinos::Teuchos).

include subpackage exports in superpackage export files

4f36e2c

whitespace changes (avoid superfluous newline in export)

a0799db

include configuration of dependent packages in export files

56a93b1

Merge branch 'master' into untangle_export_files

1a7830b

Conflicts: package_arch/TribitsWriteClientExportFiles.cmake

export targets only once per package, not per library

411a94b

nschloe mentioned this pull request Sep 18, 2014

Implement CMake performance tips to reduce configure times #18

Closed

Merge branch 'master' into untangle_export_files

8ad456c

This was referenced Sep 24, 2014

escape CMake control characters in strings #16

Open

make package export files more modular #1

Closed

move include guard in the export file to the beginning of the file

6931092

remove superfluous setting of CMAKE_CURRENT_LIST_DIR

40ee02a

CMAKE_CURRENT_LIST_DIR is guaranteed from CMake 2.8.3 on, so since the requirement for 2.8.11, we don't need this anymore.

bartlettroscoe added this to the 3_in_progress milestone Sep 25, 2014

bartlettroscoe added the type: enhancement label Sep 25, 2014

bartlettroscoe closed this Sep 25, 2014

bartlettroscoe modified the milestones: 6_deployed, 3_in_progress Sep 26, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untangle export files #11

Untangle export files #11

nschloe commented Sep 11, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 22, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 22, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 24, 2014

bartlettroscoe commented Sep 25, 2014

nschloe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014

nschloe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014

Untangle export files #11

Untangle export files #11

Conversation

nschloe commented Sep 11, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 22, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 22, 2014

nschloe commented Sep 22, 2014

bartlettroscoe commented Sep 24, 2014

bartlettroscoe commented Sep 25, 2014

nschloe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014

nschloe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014

bartlettroscoe commented Sep 25, 2014