Packages required only for tests are built even when tests are disabled

[kddevin]

Ross and I have discussed this issue already, but I wanted to record it so that I don't forget about it.

Packages that are required only for tests (via TEST_REQUIRED_DEP_PACKAGES) are built even when all tests are disabled. These required dependencies should be built only if tests are enabled.

The following Trilinos configuration demonstrates this problem:

cmake \
-D CMAKE_BUILD_TYPE:STRING="DEBUG" \
-D TPL_ENABLE_MPI:BOOL=ON \
-D MPI_EXEC_MAX_NUMPROCS:STRING=11 \
-D TPL_ENABLE_BinUtils:BOOL=OFF \
-D TPL_ENABLE_Pthread:BOOL=OFF \
\
-D Trilinos_ENABLE_ALL_OPTIONAL_PACKAGES:BOOL=OFF \
-D Trilinos_ENABLE_TESTS:BOOL=OFF \
-D Trilinos_ENABLE_EXAMPLES:BOOL=OFF \
-D Trilinos_ENABLE_Fortran:BOOL=OFF \
\
-D Trilinos_ENABLE_Zoltan2:BOOL=ON \
-D Zoltan2_ENABLE_Experimental:BOOL=ON \
\
-D Teuchos_ENABLE_STACKTRACE:BOOL=OFF \
-D Teuchos_ENABLE_LONG_LONG_INT:BOOL=ON \
..

Only Zoltan2 is enabled. Packages pamgen and galeri are needed only for Zoltan2 tests, which are disabled. Yet these two packages are both built anyway.

In my build directory,

% ls packages/
epetra/         kokkos/         teuchos/        xpetra/
galeri/         pamgen/         tpetra/         zoltan2/

[bartlettroscoe]

I think I know what happened to cause this. I will take a look at this when I get around to refactoring the dependency management system (hopefully in the next few months).

[bartlettroscoe]

This is irritating. I will get to this soon.

[bartlettroscoe]

I just realized that I will need to do a lot of testing before I can update TriBITS with this change in any TriBITS project like Trilinos or CASL VERA. That is because with this change in behavior, packages will no longer be enabled that might have been enabled before. For example, suppose package Y defines a required test dependency on package X but really the library of package Y depends on package X. With the old behavior, whenever package Y was enabled, TriBITS would always enable package X. With this new behavior, package X may not be enabled when package Y is enabled but not the tests for package Y.

The best way to test this before pushing is to do a package-by-package build and test and make sure that all of the same tests and examples are enabled and pass before and after the change. This can be done by enabling all packages and then running make dashboard which will submit an experimental build to CDash.

For CASL VERA this is type of testing is no problem. However, for Trilinos, I am not set up on any machine to fully test all of the Secondary Tested Trilinos packages. Therefore, I think that I can't safely update TriBITS in Trilinos for this change until I can get access to a full build of Trilinos. The easiest way to get there is to set up a standard SEMS build (see trilinos/Trilinos#158).

[bartlettroscoe]

Now that we have a standard SEMS build of Trilinos that builds a lot of Trilinos package, I think this would be easier and safer to do. But this is a very difficult change to make and could take days to fully update all TriBITS projects.

[sethrj]

Yep, just ran into this too. Everything that happens to LIB_REQUIRED_DEP_PACKAGES happens to TEST_REQUIRED_DEP_PACKAGES.

[sethrj]

@bartlettroscoe Since simply changing the behavior outright would affect so many projects, couldn't you make a project-wide option to enable optional/required test dependencies after the tests are enabled? I could also imagine this requiring multiple iterations of sweeping through packages, then sweeping through enable tests...

[bartlettroscoe]

@sethrj, yes, fixing this will break backward compatibility and likely break some packages. I think the fix for this is pretty easy and I did it in commit 7bb0d960d603aba3828d601976a5934aae34e92e some time ago on a branch. It was just that I could not deploy it because I can't upgrade all of the packages in all of the projects that may be impacted (you could not even build all of Trilinos a few years ago). And in that time, people have used workarounds like making packages used by tests/examples optional optional dependencies, since then they can still be disabled and therefore avoid enabling upstream packages not required to build the libraries (or executables) for downstream packages. But that is not really a very good solution.

We could ease the transition to fixing this behavior by adding an option called something like <prefix>_PACKAGE_TEST_DEPENDENCIES_NOT_LIB_DEPENDENCIES and then allow you to set this at perhaps that TriBITS Project (e.g. all of SCALE or VERA), TriBITS Repository (e.g. Trilinos packages only), even at the TriBITS package level (i.e. Thyra cleaned up but not other Trilinos packages). The latter option could ease refactoring to fix packages one at a time and fix dependencies incrementally (if desired). But adding support for this option would complicate the logic some (but not a lot).

What do you think?

[sethrj]

I'd go for whatever is the least complicated option that would allow for a gradual transition to the new logic, probably a global option. I'd envision the gradual transition as:

1. Single developer enables TRIBITS_IGNORE_DISABLED_TEST_DEPS (or whatever) on the command line or in their cmake configure cache. Fix whatever dependency issues arise.
2. Roll out to all developers; developer adds code in top-level CMake file that enables the option if ${PROJECT_NAME}_ENABLE_DEVELOPMENT_MODE is on.
3. Roll out to users; developer removes the condition on development mode and sets the flag to be on always.

[bartlettroscoe]

After thinking about this more, I fear that a change in logic like this might have far reaching impact even on packages in other TriBITS Repositories and downstream customers. For example, has MueLu an optional test dependency on Belos. Therefore, with the current (incorrect) logic, if you enable just the MueLu libs, you will get Belos enabled by default. Then, if a downstream user is getting Belos turned on that way and is using it through the TrilinosConfig.cmake file, then if we apply this fix, then Belos will no longer get enabled and therefore this will break their usage of Trilinos.

Therefore, I think if we add an option like ${PROJECT_NAME}_PACKAGE_LIB_ONLY_ENABLE_NOT_TRIGGER_TEST_DEPENDENCIES and make it FALSE by default to provide the current behavior, then we can turn it off in our development work and clean up dependencies like you say. It seems like enabling more is a safe bet but as we clean up the Dependencies.cmake files I just know that fixing the behavior is going to result in several strange side cases that will surprise developers and users as they enable subsets of packages and test suites. But I guess we just need to bite the bullet and get started on this.

I will add the option ${PROJECT_NAME}_PACKAGE_LIB_ONLY_ENABLE_NOT_TRIGGER_TEST_DEPENDENCIES to the commit 7bb0d96 and see if I can't get this merged into the main TriBITS 'master' branch and then try this out with Trilinos just to see what this looks like.

@sethrj, what project are you working on where you would try this? What relationship does that project have to Trilinos?

[hugary1995]

We just ran into this issue and I'm glad to see an activity 2 days ago!

We have a project neml2 which depends on wasp which uses TriBITS.

I glanced through the conversation in this issue, and it seems to me that you have a backward-compatible patch ready. I don't have write access to wasp but @brandonlangley does. So let us know if you want us to try your fix.

[bartlettroscoe]

@hugary1995, it looks like I never followed through with adding ${PROJECT_NAME}_PACKAGE_LIB_ONLY_ENABLE_NOT_TRIGGER_TEST_DEPENDENCIES to https://github.com/TriBITSPub/TriBITS/commit/7bb0d960d603aba3828d601976a5934aae34e92e and merging it to the master branch. I can add this if it would help.

[hugary1995]

That would definitely help, especially this won't break anything with a default of FALSE.