gh-Cyan4973/xxHash

Watch

Fork 0

cmake: unofficial_cmake does not allow -DXXH_PRIVATE_API #163

New issue

Closed

opened 2019-01-08 18:52:24 -06:00 by emaxerrno · 6 comments

emaxerrno commented

2019-01-08 18:52:24 -06:00

(Migrated from github.com)

While adding xxhash to a cmake project using external project add

  EXTERNAL_PROJECT_ARGS
    CONFIGURE_COMMAND
      ${CMAKE_COMMAND}
      -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR>
      -DBUILD_XXHSUM=OFF
      -DBUILD_SHARED_LIBS=OFF
      <SOURCE_DIR>/cmake_unofficial
    INSTALL_COMMAND
      ${make_command} install && ${CMAKE_COMMAND} -E copy <SOURCE_DIR>/xxhash.c <INSTALL_DIR>/include/xxhash.c
    URL https://github.com/Cyan4973/xxHash/archive/v0.6.5.tar.gz
    URL_MD5 6af3a964f3c2accebce66e54b44b6446)

I had to override the install command to include the xxhash.c

If enabled, the install() the install target does not include xxhash.c

A secondary solution would be to allow for -DXXHASH_INLINE_ONLY_API that can add a
dummy.cc target (empty) file and not build any libxxhash.a.

I'm happy to push a patch for the first part of this issue.

.alex

While adding xxhash to a cmake project using external project add ``` EXTERNAL_PROJECT_ARGS CONFIGURE_COMMAND ${CMAKE_COMMAND} -DCMAKE_INSTALL_PREFIX=<INSTALL_DIR> -DBUILD_XXHSUM=OFF -DBUILD_SHARED_LIBS=OFF <SOURCE_DIR>/cmake_unofficial INSTALL_COMMAND ${make_command} install && ${CMAKE_COMMAND} -E copy <SOURCE_DIR>/xxhash.c <INSTALL_DIR>/include/xxhash.c URL https://github.com/Cyan4973/xxHash/archive/v0.6.5.tar.gz URL_MD5 6af3a964f3c2accebce66e54b44b6446) ``` I had to override the install command to include the xxhash.c If enabled, the `install()` the install target does not include xxhash.c A secondary solution would be to allow for `-DXXHASH_INLINE_ONLY_API` that can add a `dummy.cc` target (empty) file and *not* build any `libxxhash.a`. I'm happy to push a patch for the first part of this issue. .alex

emaxerrno commented

2019-01-08 18:54:06 -06:00

(Migrated from github.com)

to be clear, when you do make install for example, the contents of include are:

include/xxhash.h and not include/xxhash.h, include/xxhash.c

to be clear, when you do `make install` for example, the contents of `include` are: `include/xxhash.h` and *not* `include/xxhash.h, include/xxhash.c`

Cyan4973 commented

2019-01-08 19:01:01 -06:00

(Migrated from github.com)

Hi @senior7515,
the cmake build script is community maintained.
I'll be happy to review and merge your patch if it can help your use case.

Hi @senior7515, the `cmake` build script is community maintained. I'll be happy to review and merge your patch if it can help your use case.

emaxerrno commented

2019-01-09 14:35:14 -06:00

(Migrated from github.com)

@Cyan4973 - sure thing no problem.

One thing I wanted to ask - related to cmake.

Are you against moving the CMakeLists.txt up one directory? - it makes integrating w/ xxhash simpler.

for example to integrate google test i just do 👍


... snip ...

  EXTERNAL_PROJECT_ARGS
    URL https://github.com/google/googletest/archive/644319b.tar.gz
    URL_MD5 e4cfcbe80bf32cb8caa8a7666ebc206c)

and for xxhash i have to specify <SOURCE_DIR>/cmake_unofficial and a configure command, etc.

It's no big deal - but for future consumers they might find it easier to integrate.

I'll prepare the patch for this change alone in just a minute.

@Cyan4973 - sure thing no problem. One thing I wanted to ask - related to cmake. Are you against moving the CMakeLists.txt up one directory? - it makes integrating w/ xxhash simpler. for example to integrate `google test` i just do :+1: ``` ... snip ... EXTERNAL_PROJECT_ARGS URL https://github.com/google/googletest/archive/644319b.tar.gz URL_MD5 e4cfcbe80bf32cb8caa8a7666ebc206c) ``` and for xxhash i have to specify `<SOURCE_DIR>/cmake_unofficial` and a configure command, etc. It's no big deal - but for future consumers they might find it easier to integrate. I'll prepare the patch for this change alone in just a minute.

emaxerrno commented

2019-01-09 14:46:14 -06:00

(Migrated from github.com)

I posted the patch. let me know about other patches.

A follow up patch would be to add integration test script.

I posted the patch. let me know about other patches. A follow up patch would be to add integration test script.

👍 1

Cyan4973 commented

2019-01-09 15:32:50 -06:00

(Migrated from github.com)

For the time being, I prefer to keep the cmake script into its own directory.
At the very least, the directory name helps users to realize this script is community maintained, and may need to be updated to follow the "official" Makefile from time to time.

For the time being, I prefer to keep the `cmake` script into its own directory. At the very least, the directory name helps users to realize this script is community maintained, and may need to be updated to follow the "official" `Makefile` from time to time.

emaxerrno commented

2019-01-09 15:59:40 -06:00

(Migrated from github.com)

@Cyan4973 no probs. added.

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#163

No description provided.

Rows
Columns