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 #33

Closed
wants to merge 532 commits into from
Closed

Prune export libraries and package lists #33

wants to merge 532 commits into from

Conversation

nschloe
Copy link
Contributor

@nschloe nschloe commented Oct 7, 2014

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.

Roscoe A. Bartlett and others added 30 commits August 6, 2014 16:03
This is just manually tested for now.  But this is coming along nicely.
This is not completely finished but it does the basic pass through and back.
…ESTAMP()

This includes regexes that exclude known build-related files should not
trigger a rebuild.
For some reason, this was printing different numbers of "'" at the beginning.
Seems I was formating the print -printf argument wrong.
With this, you can now make any change to the source tree and trigger the
build exteranal_func/ directory to get blow away and rebuilt again.
This explains that %T@ gives the seconds since Jan. 1, 1970, 00:00 GMT.
This new function determines if a package needs to be rebuilt at configure
time in one call.

I updated the WrapExteranl package to use this.

I will add autoamted tests as soon as I am sure this is not impractical for
VERA MOOSEExt/MOOSE for which is was designed.
When the build dir does not exist, you can't search it for files.  Also, if
you filter out all of the modified files there is nothing to do.

This is just more of an example of how badly I need an automated test.  Soon
...
…inos into raw_modified_check_3071

Conflicts:
	cmake/tribits/package_arch/TribitsGetMostRecentFileTimestamp.cmake
I changed the filters to filter out all *.cmake files from the binary
directory.  This is needed to filter out the <test-name>.cmake files that are
currently being written and updated every time by the
TRIBITS_ADD_ADVANCED_TEST() script.
When you have git repos in your source tree and you do some git stuff, this
was triggering a rebuild even though no source changed.  This was discovered
as part of the MOOSEExt/MOOSE build (CASL_MOOSE package).
…ternal

I have updated this test to copy TribitsExampleProject to the test dir and to
do several several reconfigure and rebuilds testing out rebuilds with changes
upstream files in source and build dir.

This software is not unit tested right now so that is the only test we have.
However, it is a pretty nice acceptance test and it does not take that much
longer to run.
…Kanban #3071)

Build/Test Cases Summary
Enabled Packages: TriBITS, CASL_MOOSE
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=218,notpassed=0 (40.65 min)
1) SERIAL_RELEASE => passed: passed=136,notpassed=0 (3.01 min)
Other local commits for this build/test group: 849dbd1, 109047b, 35f115a, 278e0c4, 6099eb2, 1f33aa3, b92aff9, 61ea060, 7d72328, 68bca69, 6baa40e, 87081d7, e547baa, 63ecceb, 70a9334, 9016445, 356cc8c
The update was protected in a print if statement.  I need better unit tests
for this part.

Build/Test Cases Summary
Enabled Packages: TriBITS
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=136,notpassed=0 (0.94 min)
1) SERIAL_RELEASE => passed: passed=136,notpassed=0 (0.87 min)
… improvements

Here I did:

* Added detailed doc on the HAVE_XXX_YYY vars for usage as preprocessor macros

* Added "howtos" for adding optional package and TPL dependencies

* Documented the <packageName>_config.h file explicitly

* Other small improvements and edits for typos I saw

Build/Test Cases Summary
Enabled Packages: TriBITS
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=136,notpassed=0 (0.89 min)
1) SERIAL_RELEASE => passed: passed=136,notpassed=0 (0.86 min)
Other local commits for this build/test group: 1f4c519
Getting ready to add unit testing of SET_TEST_PROPERTIES().
Here I added the wrapper macros TRIBITS_SET_TESTS_PROPERTIES() and
TRIBITS_SET_TEST_PROPERTIES() to wrap SET_TESTS_PROPERTIES() and
SET_PROPERTY(TEST ...), respectively.  These functions are then used to
capture the calls to these functions in unit test mode instead of actually
setting test properties.

I also added the new unit test helper function UNITTEST_HAS_SUBSTR_CONST().

I added a basic unit tests for the default test properties set by
TRIBITS_ADD_TEST() and TRIBITS_ADD_ADVANCED_TEST().  I also added a basic unit
test for the TIMEOUT property.

This is now ready to support the addition of TIMEOUT scaling (PHI Kanban
This was harder than I expected.  In order to allow for fractional scale
factors and not changing eixsting TIMEOUT factors, I had to implement some
pretty specialized code (see TRIBITS_SCALE_TIMEOUT() for details).

See the updated documentation for details.
… Kanban #3423)

My unit tests could not catch this because you don't actually check that the
test is added.

Build/Test Cases Summary
Enabled Packages: TriBITS, VERAIn, VERAIn_CTF, VERAIn_XML
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=332,notpassed=0 (37.95 min)
1) SERIAL_RELEASE => passed: passed=264,notpassed=0 (23.10 min)
Other local commits for this build/test group: 7831ba5, bdf91d8, 5baa998, 6ec6a32
…IMEOUT argument

Build/Test Cases Summary
Enabled Packages: TriBITS
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=136,notpassed=0 (0.93 min)
1) SERIAL_RELEASE => passed: passed=136,notpassed=0 (0.87 min)
Other local commits for this build/test group: 2d421b1
In some files that haven't been touched in many years, Windows line terminators
(CRLF) were present. This commit replaces those by standard Unix line
terminators (LF) as used by most Unix and Unix-like systems such as GNU/Linux,
and Mac OS X.

See bug #6026.
Darn, I had forgotten to actually scale TIMEOUT for the
TRIBITS_ADD_ADVANCED_TEST().  I added a unit test and added the scaling.

Build/Test Cases Summary
Enabled Packages: TriBITS
Enabled all Forward Packages
0) MPI_DEBUG => passed: passed=136,notpassed=0 (0.90 min)
1) SERIAL_RELEASE => passed: passed=136,notpassed=0 (0.86 min)
Before, all array args got jumbled together and were unreadable.  With this
change, you can read the output very well.

Build/Test Cases Summary
Enabled Packages: TriBITS, Teuchos
0) MPI_DEBUG => passed: passed=249,notpassed=0 (5.23 min)
1) SERIAL_RELEASE => passed: passed=240,notpassed=0 (2.90 min)
This gives TRIBITS_ADD_LIBRARY() a LINKER_LANGUAGE argument just like
TRIBITS_ADD_EXECUTABLE().  Also, a new function Created function
TRIBITS_SET_LINKER_LANGUAGE_FROM_ARG() to eliminate duplication.

For motivation and details, see Trilinos #6179.
Roscoe A. Bartlett and others added 19 commits October 5, 2014 09:56
To make consistent with the outer TriBITS project build-type, here I have made
general for all CMAKE_BUILD_TYPE, not just CMAKE_BUILD_TYPE=RELEASE.

Build/Test Cases Summary
Enabled Packages:
Enabled all Packages
0) MPI_DEBUG => passed: passed=652,notpassed=0 (39.98 min)
1) SERIAL_RELEASE => passed: passed=560,notpassed=0 (6.21 min)
Other local commits for this build/test group: edcd8e5, 07fe96f, e475583, a955807
If TRIBITS_ADD_LIBRARY() is called with this keyword and a respective value,
the value is set to the library name that was added, and set to nothing if no
library was added.

Fixes part of bug #4.
Largely fixes bug #4.

Still missing: TRIBITS_ADD_ADVANCED_TEST() and tests.
This helps keeping future commits clean in this respect.
…es (dir move)

This is the first step, moving the directories.  Git likes it when you move
files first, then change them.

I also wanted to use lower case names for the subpackage dir names.
This makes the substitutions:

  MixedLanguage => MixedLang
  PackageWithSubpackages => WithSubpackages
  SubpackageA => A
  SubpackageB => B
  SubpackageC => C

I also I changed the directory names:

  mixed_language => mixed_lang
  package_with_subpackages => with_subpackages
  package_with_subpackages/A => with_subpackages/a
  package_with_subpackages/B => with_subpackages/b
  package_with_subpackages/C => with_subpackages/c

The names of the subpackages are now shorter and easier to type and read.  The
directory names are shorter and are now all lower-case as is convention
adopted from Trilinos.

All of the tests have been updated too.
…LIBRARY_DIRS (TriBITS #30)

The rare VUQDemos package in CASL VERA has subpackage that don't define any
libraries.

I don't have a test case for this in TriBITS but adding the if guards fixes
this and I do have tests to ensure that duplicates are removed when there are
INCLUDE_DIRS and LIBRARY_DIRS.

Fixes #30

Build/Test Cases Summary
Enabled Packages: TriBITS, COBRA_TF, CTeuchos
0) MPI_DEBUG => passed: passed=293,notpassed=0 (4.57 min)
1) SERIAL_RELEASE => passed: passed=286,notpassed=0 (3.03 min)
Other local commits for this build/test group: e908f96, c51b77e, 3ea1cd3, 08a9c8f, bfa3094, 7bbcc8d, bda695e, 799c6ee, a12526f, 21910be, b5036bf
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 commot 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.
@nschloe
Copy link
Contributor Author

nschloe commented Oct 7, 2014

I'm having problems understanding the failing unit tests

    143 - TriBITS_TribitsExampleProject_ALL_ST_NoFortran (Failed)
    145 - TriBITS_TribitsExampleProject_ALL_PT_NoFortran (Failed)

They say something about WithSubpackagesC.WithSubpackagesB.WithSubpackagesA.SimpleCxx. @bartlettroscoe, can you help out?

@bartlettroscoe
Copy link
Member

Nico,

I think we need to wait until the Trilinos release branch is created and then look into this issue. Removing these variables will break backward compatibility which TriBITS should almost never be doing at this point as it would classified as late-stage Production Growth or early stage Production Maintenance code. See:

At the very least, we need to find a way to deprecate the usage of these variables. Also, this change will require and update to the documentation here. Every change involves updating documentation and automated tests.

As for WithSubpackagesC.WithSubpackagesB.WithSubpackagesA.SimpleCxx, that is just a regex for the list of dependent packages for the WithSubpackages example package. Of course this change removes the SimpleCxx package from this list so that is why it is breaking (which is good).

Let's revisit this in a few weeks.

@nschloe
Copy link
Contributor Author

nschloe commented Oct 7, 2014

I think we need to wait until the Trilinos release branch is created and then look into this issue.

Of course. I wasn't planning on pushing this in today.

At the very least, we need to find a way to deprecate the usage of these variables.

Yup; I wasn't really sure how to do that, though. Anyhow, there are more things wrong with the export system, so this whole thing needs to be revisited at some point.

Let's revisit this in a few weeks.

Agreed.

@nschloe
Copy link
Contributor Author

nschloe commented Apr 6, 2015

Not sure at all what happened with this PR. Has history been rewritten at some point?

@bartlettroscoe
Copy link
Member

Repo got filtered as part of #26, as per your request :-)

@nschloe
Copy link
Contributor Author

nschloe commented Apr 7, 2015

Oh well, I suppose we'll have to filter my fork in the same way to make some sense of the existing PRs. Alternatively, I could try and go through the commits of mine and see if I can create a new PR from those.

@bartlettroscoe
Copy link
Member

Can't you just create new branches by cherry-picking the commits? Or, you can use git format-path and git am to move the commits.

@nschloe
Copy link
Contributor Author

nschloe commented Apr 7, 2015

cherry-picking is probably the best idea. I'll just have to go through the above list and see what commits are actually meant to be in this PR.

@nschloe
Copy link
Contributor Author

nschloe commented Apr 8, 2015

Closing in favor of PR #60.

@nschloe nschloe closed this Apr 8, 2015
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