Looks like it was introduced by @fsimonis in #30374 in case he wants to comment.

The arguments for defining it still stands. It saves compile time for projects that use ctest and conditional test compilation.

The warning may be annoying, but it's not a problem.

I can think of the following possible solutions to get rid of the warning:

• deactivate CMake developer warnings by default, passing -Wno-dev.
• execute CMake without the variable, then grep the CMakeCache for BUILD_TESTING. If it exists, rerun cmake with BUILD_TESTING=OFF. This doesn't work if the tests require dependencies that are not defined as requirements in the spack package.

What percentage of packages use CTest? Can we manually add it to those packages as needed?

What percentage of packages use CTest? Can we manually add it to those packages as needed?

If there are quite a few, and all following the same pattern, we can also extract a mixin. Packages that are known to use CTest would just inherit from it.

The CMakePackage class uses the test target for running tests in check, which is a reserved alias for running ctest.

Even if packages don't implement tests using ctest, it's the current expectation of spack that they do.

Defining BUILD_TESTING follows the current package defaults and can safe a lot of computing resources for CMake packages that use CTest and heavily invest in testing.

At the end of the day it is a warning, which can be disabled or ignored.
Wasting compute resources to silence a warning doesn't sound right to me. Especially as in practise, spack packages are built multiple times for various specs in a deployment.

Anyway, I don't have the full picture here. Take this as feedback from the perspective of a package maintainer. :)

Disabling the current feature of enabling the default testing flag for CTest in CMake automatically seems a bit heavy handed to fix a configuration warning in a subset of CMake packages.

What percentage of packages use CTest? Can we manually add it to those packages as needed?

From my experience nearly all of the CMake packages that implement unit tests are using CTest unless CMake is a secondary build system.

If there are quite a few, and all following the same pattern, we can also extract a mixin.

I am not opposed to this, but I think it is maybe a bit overkill if the only motivation is to drop a configuration warning.

What I meant with "this was a mistake":

Some project conditionally include CTest and instead of repeating that same condition everywhere, they then test whether BUILD_TESTING is on, because that variable is defined by the include. So, defining this variant globally breaks that invariant.

To me setting BUILD_TESTING is like overriding a private variable.

See also #30374 (comment)

Disabling the current feature of enabling the default testing flag for CTest in CMake automatically seems a bit heavy handed to fix a configuration warning in a subset of CMake packages.

Adding a feature to enable a flag for CTest in CMake automatically seems a bit heavy handed to support testing in a subset of CMake packages.

I guess I'm not very familiar with the CMake ecosystem so I'm not sure how common CTest is. But I'm still in favor of adding this flag to packages that use it instead of adding it globally.

BUILD_TESTING=OFF only (formally) makes add_test do nothing. BUILD_TESTING=OFF will suppress the automatic call to enable_testing() in the CTest module. Other than that, it formally does nothing specific. Some projects may use a variable in their namespace (e.g., VTK_BUILD_TESTING). I think it's something that each package should do as needed, personally.

What percentage of packages use CTest?

Of the ~80 packages I have installed, more than half issued this warning, meaning less than half use CTest if I'm understanding correctly.

Taking a step back from this, there are 4 cases of how CMake projects define tests:

• Completely custom testing: requires overwriting check() and cmake_args()
• CTest with the loosely conventional BUILD_TESTING: currently requires not extra config
• CTest with potentially multiple custom variables to control test build: requires overwriting cmake_args()
• Project without tests: 🙈

Before the default BUILD_TESTING variable, a package maintainer should have done the following (replace BUILD_TESTING with the project specific variable):

def cmake_args(self):
    return [define("BUILD_TESTING", self.run_tests)]

This, however, means that tests always compile in case a package maintainer doesn't add this statement, even if they aren't requested by spack.

If we agree that BUILD_TESTING is an acceptable default in need of an overwrite, then we can make this behaviour overridable by introducing a separate function for test-specific cmake arguments.
The default being:

def cmake_test_args(self):
    return [define("BUILD_TESTING", self.run_tests)]

This way, a package that uses BUILD_TESTING, doesn't need to configure anything.

Other packages that don't use BUILD_TESTING need to configure something anyhow, so they can overwrite this function:

def cmake_test_args(self):
    return [define("ACTIVATE_TESTING", self.run_tests),
            define("PLEASE_BUILD_MY_TEST_BINARIES", self.run_tests)]

The last problem with this suggestion are CMake projects without any unit tests.
They will still show the warning unless they define a do-nothing cmake_test_args.

The main benefit of the default is that packages not using the BUILD_TESTING will run into the debated warning, triggering the maintainers to configure project-specific testing.

I personally wasn't aware of this run_tests until I accidentally stumbled across it.
Whatever way this goes, I volunteer to extend the documentation of managing tests in packages to make this clearer to package maintainers and creators.

I personally wasn't aware of this run_tests until I accidentally stumbled across it.

If you introduce cmake_test_args you now have two attributes that are hard to discover.

I'd rather have packages enable tests explicitly like this PR is requiring.

1. The build options are clear if you just look at package.py
2. There is no uniform way of enabling tests in cmake, and "modern" cmake probably advocates <PKGNAME>_ENABLE_TESTS=ON prefixes or something
3. Opt-in > opt-out
4. It's annoying to get warnings about unused defines
5. There have been reports of package build errors, because the invariant "if include(CTest) then BUILD_TESTING=ON" was broken by Spack
6. Package hash is based on package.py, not on its build system, so better to put defines in the package, not in the build system.

If you introduce cmake_test_args you now have two attributes that are hard to discover.

I absolutely agree. It needs documentation.

I'd rather have packages enable tests explicitly like this PR is requiring.

The build options are clear if you just look at package.py
Opt-in > opt-out

Currently, spack provides default variants for ipo and build_type, and the test runner defaults to ctest, hence this is already not case. Adding another function dedicated to the test setup keeps the info in the package.py and gives the package additional structure.

There is no uniform way of enabling tests in cmake, and "modern" cmake probably advocates _ENABLE_TESTS=ON prefixes or something

Sadly, this is the case for many things in CMake.

It's annoying to get warnings about unused defines

Absolutely agree. I personally think that this inconvenience is worth saving potentially many CPU hours per build. Now that the default build_type changed to release, packages using IPO will waste a long time linking test binaries, especially if they use the default linker. Even if energy bills aren't an issue, blocking compute time may be.

There have been reports of package build errors, because the invariant "if include(CTest) then BUILD_TESTING=ON" was broken by Spack

As mentioned before, BUILD_TESTING is defined as an option, which purpose is to be user-overridable. These are/were bugs in the build-systems of the packaged projects.

Package hash is based on package.py, not on its build system, so better to put defines in the package, not in the build system.

Not sure if I understand your point. Packages would override cmake_test_args() in their package.py, so this shouldn't be an issue.

Another possibility to move on could be to:

• Merge this PR
• Extend documentation regarding test configuration in packages (as mentioned above).
• Add the following in the CMake package template to make new packages aware of this and let package maintainers run into the warning before they even commit the package:

  def cmake_args(self):
    return [define("BUILD_TESTING", self.run_tests)]

• Manually add test configuration to existing CMake packages that use CTest and don't yet use conditional build of their test suite.

Manually add test configuration to existing CMake packages that use CTest and don't yet use conditional build of their test suite.

I think this is the biggest sticking point of my PR. No one really wants to download and extract tarballs for all 1.1K CMake packages in Spack, grep for CTest in the CMakeLists.txt, and update those.

Here's an alternative idea that may make everyone happy. What if we grep the root CMakeLists.txt file and look for evidence of CTest usage, then append that flag if it's used? Not sure if CTest is always mentioned in the root CMakeLists.txt or if this would require us to recursively search for all of them. But if there's a consistent string it shouldn't be that hard to find. Are there any downsides to this solution?

For a variety of…arcane wart-shaped reasons, include(CTest) only really works reliably when included in the top-level directory scope. However, include() will hide it from a literal code grep.

I think this is the biggest sticking point of my PR. No one really wants to download and extract tarballs for all 1.1K CMake packages in Spack, grep for CTest in the CMakeLists.txt, and update those.

Spack packages list their maintainers, so one could also mention them in a new issue and ask them if they could have a look at their packages. This distributes the work nicely and adjusting package configs should be way easier for people knowing the software.

For 41d2161 that's 1096 packages and a total of 382 maintainers:

$ rg CMakePackage -l | grep "builtin/packages" | wc -l
1096
$ rg CMakePackage -l | grep "builtin/packages" | cut -d/ -f6 | xargs spack maintainers | wc -l
382

Not sure if GitHub has any mention limitations in place though.

There are mention limitations but you can distribute across multiple comments. The problem is that most packages don't have maintainers.

@spackbot rebuild everything

I've started that pipeline for you!

@spackbot rebuild everything

I've started that pipeline for you!

@spackbot run pipeline

I've started that pipeline for you!

I'm fine with reverting back to just dropping the flag, but will give others time to comment first.

I agree with @haampie. If the choice is between adding magic logic to CMakePackage or dropping the flag entirely, I would prefer to just drop it. I think people like @eugeneswalker and @wspear who depend on the Spack testing heavily may be able to inform which packages to prioritize in a followup.

Sounds like we have a consensus. Should be ready to merge!