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

Allow making CMake options dependent on others #356

Merged
merged 1 commit into from
Aug 18, 2022

Conversation

msimberg
Copy link
Contributor

@msimberg msimberg commented Aug 8, 2022

Fixes #317.

Also makes the PIKA_WITH_TESTS_* and PIKA_WITH_EXAMPLES_* options dependent on PIKA_WITH_TESTS and PIKA_WITH_EXAMPLES, respectively. If someone spots a possibility to use dependent options elsewhere don't hesitate to just change it.

I've kept the old pika_option implementation for non-BOOL types, and added new variants based on option and cmake_dependent_option. The pika_option function is now a macro because of the scope required for the "hidden" (helper) variables used by cmake_dependent_option.

@msimberg msimberg added this to the 0.8.0 milestone Aug 8, 2022
@msimberg
Copy link
Contributor Author

msimberg commented Aug 8, 2022

bors try

@pika-bot
Copy link
Collaborator

pika-bot commented Aug 8, 2022

Performance test report

pika Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2022-05-17T10:12:00+00:002022-08-08T11:33:39+00:00
pika Commitd45937da17686b
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Datetime2022-05-17T12:20:04.953138+02:002022-08-08T13:41:52.430164+02:00
Envfile
Clusternamedaintdaint
Hostnamenid01398nid01198

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch+

Info

PropertyBeforeAfter
pika Datetime2022-06-24T10:19:16+00:002022-08-08T11:33:39+00:00
pika Commit453f6a4a17686b
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Datetime2022-06-24T12:43:43.615347+02:002022-08-08T13:42:07.071886+02:00
Envfile
Clusternamedaintdaint
Hostnamenid00025nid01198

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
Stream Benchmark - Add(=)-=
Stream Benchmark - Scale+++(=)+++
Stream Benchmark - Triad+++++
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2022-05-17T10:12:00+00:002022-08-08T11:33:39+00:00
pika Commitd45937da17686b
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Datetime2022-05-17T12:20:34.006375+02:002022-08-08T13:42:21.734254+02:00
Envfile
Clusternamedaintdaint
Hostnamenid01398nid01198

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (>10%)
++/--Large performance improvement/degradation (>10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

@msimberg msimberg force-pushed the cmake-dependent-options branch from 9e4a8c5 to c82f8c9 Compare August 8, 2022 11:46
@pika-bot
Copy link
Collaborator

pika-bot commented Aug 8, 2022

Performance test report

pika Performance

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
For Each(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2022-05-17T10:12:00+00:002022-08-08T11:46:02+00:00
pika Commitd45937d5741308
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid01398nid01376
Clusternamedaintdaint
Datetime2022-05-17T12:20:04.953138+02:002022-08-08T13:53:56.116093+02:00
Envfile

Comparison

BENCHMARKNO-EXECUTOR
Future Overhead - Create Thread Hierarchical - Latch+

Info

PropertyBeforeAfter
pika Datetime2022-06-24T10:19:16+00:002022-08-08T11:46:02+00:00
pika Commit453f6a45741308
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid00025nid01376
Clusternamedaintdaint
Datetime2022-06-24T12:43:43.615347+02:002022-08-08T13:54:10.865282+02:00
Envfile

Comparison

BENCHMARKFORK_JOIN_EXECUTORPARALLEL_EXECUTORSCHEDULER_EXECUTOR
Stream Benchmark - Add(=)(=)--
Stream Benchmark - Scale+++(=)+++
Stream Benchmark - Triad++(=)+
Stream Benchmark - Copy(=)(=)(=)

Info

PropertyBeforeAfter
pika Datetime2022-05-17T10:12:00+00:002022-08-08T11:46:02+00:00
pika Commitd45937d5741308
Compiler/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1/apps/daint/SSL/pika/spack/lib/spack/env/clang/clang++ 11.0.1
Hostnamenid01398nid01376
Clusternamedaintdaint
Datetime2022-05-17T12:20:34.006375+02:002022-08-08T13:54:25.662659+02:00
Envfile

Explanation of Symbols

SymbolMEANING
=No performance change (confidence interval within ±1%)
(=)Probably no performance change (confidence interval within ±2%)
(+)/(-)Very small performance improvement/degradation (≤1%)
+/-Small performance improvement/degradation (>10%)
++/--Large performance improvement/degradation (>10%)
+++/---Very large performance improvement/degradation (>10%)
?Probably no change, but quite large uncertainty (confidence interval with ±5%)
??Unclear result, very large uncertainty (±10%)
???Something unexpected…

bors bot added a commit that referenced this pull request Aug 8, 2022
@bors
Copy link
Contributor

bors bot commented Aug 8, 2022

try

Timed out.

@msimberg msimberg self-assigned this Aug 9, 2022
@msimberg
Copy link
Contributor Author

msimberg commented Aug 9, 2022

bors try

bors bot added a commit that referenced this pull request Aug 9, 2022
@bors
Copy link
Contributor

bors bot commented Aug 9, 2022

try

Build failed:

@biddisco
Copy link
Contributor

bors try

bors bot added a commit that referenced this pull request Aug 10, 2022
Copy link
Contributor

@aurianer aurianer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thanks!

option("${option}" "${description}" "${default}")
else()
cmake_dependent_option(
"${option}" "${description}" "${default}" "${PIKA_OPTION_DEPENDS}" OFF
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why don't we set the default to ON here? It should be ON if the PIKA_OPTION_DEPENDS is ON right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the default to default. The last OFF is the <force> variable from https://cmake.org/cmake/help/latest/module/CMakeDependentOption.html?highlight=cmake_dependent_option#command:cmake_dependent_option:

Makes <option> available to the user if the [semicolon-separated list](https://cmake.org/cmake/help/latest/manual/cmake-language.7.html#cmake-language-lists) of conditions in <depends> are all true. Otherwise, a local variable named <option> is set to <force>.

i.e. it's the value that is set if the PIKA_OPTION_DEPENDS condition is not true, which in all cases we have so far should just turn off the option. In theory there could be options that want the inverse, but I think we can add that when we need it.

@msimberg msimberg force-pushed the cmake-dependent-options branch from c82f8c9 to d5cbaa4 Compare August 18, 2022 07:45
@msimberg
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Aug 18, 2022
356: Allow making CMake options dependent on others r=msimberg a=msimberg

Fixes #317.

Also makes the `PIKA_WITH_TESTS_*` and `PIKA_WITH_EXAMPLES_*` options dependent on `PIKA_WITH_TESTS` and `PIKA_WITH_EXAMPLES`, respectively. If someone spots a possibility to use dependent options elsewhere don't hesitate to just change it.

I've kept the old `pika_option` implementation for non-`BOOL` types, and added new variants based on `option` and `cmake_dependent_option`. The `pika_option` function is now a macro because of the scope required for the "hidden" (helper) variables used by `cmake_dependent_option`.

Co-authored-by: Mikael Simberg <mikael.simberg@iki.fi>
@bors
Copy link
Contributor

bors bot commented Aug 18, 2022

Build failed:

@msimberg
Copy link
Contributor Author

bors merge

bors bot added a commit that referenced this pull request Aug 18, 2022
356: Allow making CMake options dependent on others r=msimberg a=msimberg

Fixes #317.

Also makes the `PIKA_WITH_TESTS_*` and `PIKA_WITH_EXAMPLES_*` options dependent on `PIKA_WITH_TESTS` and `PIKA_WITH_EXAMPLES`, respectively. If someone spots a possibility to use dependent options elsewhere don't hesitate to just change it.

I've kept the old `pika_option` implementation for non-`BOOL` types, and added new variants based on `option` and `cmake_dependent_option`. The `pika_option` function is now a macro because of the scope required for the "hidden" (helper) variables used by `cmake_dependent_option`.

Co-authored-by: Mikael Simberg <mikael.simberg@iki.fi>
@bors
Copy link
Contributor

bors bot commented Aug 18, 2022

Build failed:

@msimberg
Copy link
Contributor Author

bors merge

@bors bors bot merged commit 35664f2 into pika-org:main Aug 18, 2022
@msimberg msimberg deleted the cmake-dependent-options branch August 18, 2022 10:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Use CMake's dependent options for options that are conditionally enabled
4 participants