gh-Cyan4973/xxHash

Watch

Fork 0

Support for older versions of CMake #682

New issue

Closed

opened 2022-02-09 12:33:23 -06:00 by MikhailGorobets · 4 comments

MikhailGorobets commented

2022-02-09 12:33:23 -06:00

(Migrated from github.com)

The add_compile_definitions command was added to CMake in version 3.12 but we use it

https://github.com/Cyan4973/xxHash/blob/d8aff149b5b9b9461f4b01fc5ab4842112d2c867/cmake_unofficial/CMakeLists.txt#L47

I suggest using the command add_definitions for older versions and wrap this command like here

https://github.com/Cyan4973/xxHash/blob/d8aff149b5b9b9461f4b01fc5ab4842112d2c867/cmake_unofficial/CMakeLists.txt#L23

The `add_compile_definitions` command was added to CMake in version 3.12 but we use it ``` https://github.com/Cyan4973/xxHash/blob/d8aff149b5b9b9461f4b01fc5ab4842112d2c867/cmake_unofficial/CMakeLists.txt#L47 ``` I suggest using the command `add_definitions` for older versions and wrap this command like here ``` https://github.com/Cyan4973/xxHash/blob/d8aff149b5b9b9461f4b01fc5ab4842112d2c867/cmake_unofficial/CMakeLists.txt#L23 ```

👍 1

Cyan4973 commented

2022-05-06 17:21:44 -05:00

(Migrated from github.com)

What is wanted here is to set XXH_DEBUGLEVEL=1 in order to enable assert() in debug builds,
which would allow to catch issues like #588 during CI tests.
add_compile_definitions was added just for this purpose. It's the only place where it's employed.

The cmake recipe is supposed to be compatible with cmake versions as old as v2.8.12.
So it's a concern that adding this single line broke that requirement.

A few questions :

How to know the minimum viable version for add_definitions ?
- Looking at cmake documentation, it doesn't go farther than v3.0. That's fine for add_compile_definitions, which clearly states being introduced in v3.12, but there is no such information for add_definitions, which is only assumed being there "before v3.0".
Looking at the documentation of add_definitions, it's not clear to me that it would work properly for MSVC compiler. It seems tied to the -D prefix convention of gcc.
Finally, how could we test the cmake recipe for v2.8.12 compatibility in CI ?
If we have no error signal, it will be easy for any future contributions to break that requirement again.

We may also question if v2.8.12 is a good minimum version, but that doesn't change the underlying issue : another minimum version will have to selected, and we are still lacking a test to ensure that updates cmake recipes are still compatible with this version.

What is wanted here is to set `XXH_DEBUGLEVEL=1` in order to enable `assert()` in debug builds, which would allow to catch issues like #588 during CI tests. `add_compile_definitions` was added just for this purpose. It's the only place where it's employed. The `cmake` recipe is supposed to be compatible with `cmake` versions as old as `v2.8.12`. So it's a concern that adding this single line broke that requirement. A few questions : - How to know the minimum viable version for `add_definitions` ? - Looking at `cmake` documentation, it doesn't go farther than `v3.0`. That's fine for `add_compile_definitions`, which clearly states being introduced in `v3.12`, but there is no such information for `add_definitions`, which is only assumed being there "before `v3.0`". - Looking at the [documentation of `add_definitions`](https://cmake.org/cmake/help/latest/command/add_definitions.html), it's not clear to me that it would work properly for MSVC compiler. It seems tied to the `-D` prefix convention of `gcc`. - Finally, how could we test the `cmake` recipe for v2.8.12 compatibility in CI ? If we have no error signal, it will be easy for any future contributions to break that requirement again. We may also question if `v2.8.12` is a good minimum version, but that doesn't change the underlying issue : another minimum version will have to selected, and we are still lacking a test to ensure that updates `cmake` recipes are still compatible with this version.

t-mat commented

2022-05-06 19:34:21 -05:00

(Migrated from github.com)

Finally, how could we test the cmake recipe for v2.8.12 compatibility in CI ?

I don't test it yet, but there's convenient GH-Action called lukka/get-cmake which enables GH-Actions to install specific version of CMake.

    - name: Get specific version CMake, v3.23.0
      uses: lukka/get-cmake@v3.23.0

Basically, get-cmake downloads prebuild archive from official releases. It seems their oldest version is v2.4.8.

> Finally, how could we test the cmake recipe for v2.8.12 compatibility in CI ? I don't test it yet, but there's convenient GH-Action called [`lukka/get-cmake`](https://github.com/lukka/get-cmake#quickstart=) which enables GH-Actions to install specific version of CMake. ```YAML - name: Get specific version CMake, v3.23.0 uses: lukka/get-cmake@v3.23.0 ``` Basically, `get-cmake` [downloads prebuild archive from official releases](https://github.com/lukka/get-cmake/blob/main/src/get-cmake.ts#L38-L41=). It seems their oldest version is [`v2.4.8`](https://github.com/Kitware/CMake/releases/tag/v2.4.8).

👍 1

Cyan4973 commented

2022-05-06 20:08:13 -05:00

(Migrated from github.com)

Basically, get-cmake downloads prebuild archive from official releases.

Thanks @t-mat ,
indeed, that's the same method used in the newly added cmake version compatibility test.

> Basically, `get-cmake` [downloads prebuild archive from official releases](https://github.com/lukka/get-cmake/blob/main/src/get-cmake.ts#L38-L41=). Thanks @t-mat , indeed, that's the same method used in the [newly added cmake version compatibility test](https://github.com/Cyan4973/xxHash/pull/711/files#diff-b803fcb7f17ed9235f1e5cb1fcd2f5d3b2838429d4368ae4c57ce4436577f03fR279).

👍 1

t-mat commented

2022-05-06 21:04:47 -05:00

(Migrated from github.com)

Looking at the documentation of add_definitions, it's not clear to me that it would work properly for MSVC compiler. It seems tied to the -D prefix convention of gcc.

add_definitioins() interpret both of -D and /D as a preprocessor definition.

When I add the following line to the CMakeLists.txt,

add_definitions(-DXXH_DEBUGLEVEL=1 -DXXH_TMAT_TEST=4321)

cmake emits the following definitions into the .vcxproj file.

<PreprocessorDefinitions>%(PreprocessorDefinitions);...;XXH_EXPORT;XXH_DEBUGLEVEL=1;XXH_TMAT_TEST=4321;...</PreprocessorDefinitions>

But the following block doesn't work with MSVC (and Xcode, etc) project.

if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug")
  add_compile_definitions(XXH_DEBUGLEVEL=1)
endif()

Because CMAKE_BUILD_TYPE only works with "single-configuration generators" (Makefile, etc).

Also, there're a lot of advice that we should avoid to use CMAKE_BUILD_TYPE.

So it seems using target_compile_definitions() is the most simple way to define the debug build symbol.

        # libxxhash
        add_library(xxhash "${XXHASH_DIR}/xxhash.c")
        add_library(${PROJECT_NAME}::xxhash ALIAS xxhash)

+       target_compile_definitions(xxhash
+         PRIVATE
+           $<$<CONFIG:Debug>:
+               XXH_DEBUGLEVEL=1
+           >
+       )

But it also has introduced at CMake 3.12 :(

> Looking at the documentation of add_definitions, it's not clear to me that it would work properly for MSVC compiler. It seems tied to the -D prefix convention of gcc. `add_definitioins()` interpret both of `-D` and `/D` as a preprocessor definition. When I add the following line to the `CMakeLists.txt`, ``` add_definitions(-DXXH_DEBUGLEVEL=1 -DXXH_TMAT_TEST=4321) ``` `cmake` emits the following definitions into the `.vcxproj` file. ``` <PreprocessorDefinitions>%(PreprocessorDefinitions);...;XXH_EXPORT;XXH_DEBUGLEVEL=1;XXH_TMAT_TEST=4321;...</PreprocessorDefinitions> ``` ---- But the following block doesn't work with MSVC (and Xcode, etc) project. ``` if("${CMAKE_BUILD_TYPE}" STREQUAL "Debug") add_compile_definitions(XXH_DEBUGLEVEL=1) endif() ``` Because [`CMAKE_BUILD_TYPE`](https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html) only works with "single-configuration generators" (Makefile, etc). Also, there're a lot of advice that [we should avoid to use `CMAKE_BUILD_TYPE`](https://stackoverflow.com/a/24470998). So it seems using `target_compile_definitions()` is the most simple way to define the debug build symbol. ``` # libxxhash add_library(xxhash "${XXHASH_DIR}/xxhash.c") add_library(${PROJECT_NAME}::xxhash ALIAS xxhash) + target_compile_definitions(xxhash + PRIVATE + $<$<CONFIG:Debug>: + XXH_DEBUGLEVEL=1 + > + ) ``` But it also has introduced at [CMake 3.12](https://cmake.org/cmake/help/latest/release/3.12.html#id5) :(

👍 1

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-Cyan4973/xxHash#682

No description provided.

Rows
Columns