Skip to content

Add BUILD_TESTING to standard CMake arguments#30374

Merged
chuckatkins merged 1 commit intospack:developfrom
fsimonis:patch-1
Apr 28, 2022
Merged

Add BUILD_TESTING to standard CMake arguments#30374
chuckatkins merged 1 commit intospack:developfrom
fsimonis:patch-1

Conversation

@fsimonis
Copy link
Copy Markdown
Member

@fsimonis fsimonis commented Apr 28, 2022

CTest determines whether to enable tests using the BUILD_TESTING variable.
This should be used by projects to conditionally enable the compilation of tests.
Spack knowns which packages are requested to run tests and can thus automatically define this variable for CMake packages.

This addition should

  • have no observable consequences, and
  • reduce compile time for every CMake project using BUILD_TESTING.

@fsimonis
Add BUILD_TESTING to standard CMake arguments
2afcc72 
CTest determines whether to enable tests using the BUILD_TESTING variable.
This should be used by projects to conditionally enable the compilation of tests.
Spack knowns which packages have to run tests and can thus automatically define this variable.
@chuckatkins
Copy link
Copy Markdown

chuckatkins commented Apr 28, 2022

reduce compile time for every CMake project using BUILD_TESTING.

I think many packages already implement this in their cmake_args but O definitely support moving common / standardized patterns to the base package.

@chuckatkins chuckatkins enabled auto-merge (squash) April 28, 2022 15:08
@chuckatkins chuckatkins merged commit 1006dd5 into spack:develop Apr 28, 2022
@fsimonis fsimonis deleted the patch-1 branch April 28, 2022 21:58
@albestro albestro mentioned this pull request May 9, 2022
Merged
@haampie
Copy link
Copy Markdown
Member

haampie commented May 9, 2022

have no observable consequences, and

except it does:

Maybe it's better to leave it to the package to decide? There's probably more cmake packages that expect include(CTest) to set BUILD_TESTING=ON by default.

@fsimonis
Copy link
Copy Markdown
Member Author

fsimonis commented May 10, 2022

There's probably more cmake packages that expect include(CTest) to set BUILD_TESTING=ON by default.

You are right, this is the default of this option. Always expecting the default behaviour of a user-definable option (without asserting it), however, is a bug in the build system of the software.

Using BUILD_TESTING as intended will result in more robust packages (as in the Eigen case) and uncover upstream bugs (as in the DLA-Future case).

We weren't aware of BUILD_TESTING in preCICE either, until one of our users pointed out that we didn't implement it correctly.

@haampie haampie mentioned this pull request Aug 2, 2022
Closed
@adamjstewart adamjstewart mentioned this pull request May 27, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chuckatkins chuckatkins chuckatkins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

build-systems

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fsimonis @chuckatkins @haampie