Inconsistent naming of CMake options

ghost commented 5 years ago

Non-systematic naming makes it harder to run config from command line:

Most of the options start with CUKE_DISABLE_ but there is CUKE_ENABLE_EXAMPLES
There is VALGRIND_TEST that doesn't have any prefix along with BUILD_SHARED_LIBS
Inconsistent use of STATIC and SHARED in options that switch between static/dynamic linkage

Solution

Mark old options deprecated and replace them with new ones. Don't invert. Prefer STATIC when option is linkage-related, like in Boost_USE_STATIC_LIBS.

New naming №1

Shorter options that all start with CUKE_

Old name                   Default   |  New name                   Default   |  Note
-------------------------------------+---------------------------------------+------
BUILD_SHARED_LIBS          OFF       |  CUKE_STATIC_LIBS           ON        |
CUKE_USE_STATIC_BOOST      ${WIN32}  |  CUKE_STATIC_BOOST          ${WIN32}  |
CUKE_USE_STATIC_GTEST      ON        |  CUKE_STATIC_GOOGLE_TEST    ON        |  [1]
CUKE_DISABLE_BOOST_TEST    OFF       |  CUKE_BOOST_TEST            ON        |
CUKE_DISABLE_GTEST         OFF       |  CUKE_GOOGLE_TEST           ON        |  [1]
CUKE_DISABLE_UNIT_TESTS    OFF       |  CUKE_UNIT_TESTS            ON        |
CUKE_DISABLE_E2E_TESTS     OFF       |  CUKE_E2E_TESTS             ON        |
CUKE_ENABLE_EXAMPLES       OFF       |  CUKE_EXAMPLES              OFF       |
CUKE_DISABLE_QT            OFF       |  CUKE_QT                    ON        |
VALGRIND_TESTS             OFF       |  CUKE_VALGRIND_TESTS        OFF       |

  [1]  Replace `GTEST` with `GOOGLE_TEST` to match `BOOST_TEST`

New naming № 2

Options start with:

CUKE_ENABLE_ if feature can be enabled or disabled
CUKE_USE_ if feature can't be disabled but a different setup can be chosen

Old name                   Default   |  New name                         Default  |  Note
-------------------------------------+--------------------------------------------+------
BUILD_SHARED_LIBS          OFF       |  CUKE_USE_STATIC_LIBS             ON       |
CUKE_USE_STATIC_GTEST      ON        |  CUKE_USE_STATIC_GOOGLE_TEST      ON       |  [1]
CUKE_DISABLE_BOOST_TEST    OFF       |  CUKE_ENABLE_BOOST_TEST           ON       |
CUKE_DISABLE_GTEST         OFF       |  CUKE_ENABLE_GOOGLE_TEST          ON       |  [1]
CUKE_DISABLE_UNIT_TESTS    OFF       |  CUKE_ENABLE_UNIT_TESTS           ON       |
CUKE_DISABLE_E2E_TESTS     OFF       |  CUKE_ENABLE_E2E_TESTS            ON       |
CUKE_DISABLE_QT            OFF       |  CUKE_ENABLE_QT                   ON       |
VALGRIND_TESTS             OFF       |  CUKE_ENABLE_VALGRIND_TESTS       OFF      |

  [1]  Replace `GTEST` with `GOOGLE_TEST` to match `BOOST_TEST`

Doesn't change:
  CUKE_USE_STATIC_BOOST
  CUKE_ENABLE_EXAMPLES

paoloambrosio commented 5 years ago

Thanks for raising this.

Points where I disagree:

BUILD_SHARED_LIBS is a standard CMake property, so I'm not sure about that.
GTEST is used in CMake's standard module, so I would keep it that way.
CUKE_USE_STATIC_BOOST and CUKE_USE_STATIC_GTEST make sense to me, so I would keep them as they are.

On the DISABLE and ENABLE flags I agree that it makes sense to standardise them.

Features prefixed with CUKE_ENABLE_

Old name                   Default  |  New name                  Default
------------------------------------+-----------------------------------
CUKE_DISABLE_BOOST_TEST    OFF      |  CUKE_ENABLE_BOOST_TEST    ON
CUKE_DISABLE_GTEST         OFF      |  CUKE_ENABLE_GTEST         ON
CUKE_DISABLE_QT            OFF      |  CUKE_ENABLE_QT            ON
CUKE_ENABLE_EXAMPLES       OFF      |  CUKE_ENABLE_EXAMPLES      OFF

Test suites could be prefixed with CUKE_TESTS_ to make it clear that they are not features:

Old name                   Default  |  New name                  Default
------------------------------------+-----------------------------------
CUKE_DISABLE_UNIT_TESTS    OFF      |  CUKE_TESTS_UNIT           ON
CUKE_DISABLE_E2E_TESTS     OFF      |  CUKE_TESTS_E2E            ON
VALGRIND_TESTS             OFF      |  CUKE_TESTS_VALGRIND       OFF

ghost commented 5 years ago

@paoloambrosio Could you, please, review PR #216 for this issue?

cucumber / cucumber-cpp