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

Fix typo in CMakeList. #12247

Closed
wants to merge 1 commit into from
Closed

Conversation

rhubner
Copy link
Contributor

@rhubner rhubner commented Jan 18, 2024

Fix #12237

Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

I think a small fix is still needed in Findzstd.cmake.
It might also be worth checking the other files in cmake/modules/ as well?

@@ -166,7 +166,7 @@ else()
if(WITH_ZSTD)
find_package(zstd REQUIRED)
add_definitions(-DZSTD)
include_directories(${ZSTD_INCLUDE_DIR})
include_directories(${ZSTD_INCLUDE_DIRS})
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this also needs fixing in cmake/modules/Findzstd.cmake where it seems to be in mixed-case.

@rhubner rhubner marked this pull request as ready for review January 24, 2024 12:04
@adamretter adamretter self-requested a review January 24, 2024 17:20
Copy link
Collaborator

@adamretter adamretter left a comment

Choose a reason for hiding this comment

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

Now LGTM. Thanks @rhubner

@adamretter adamretter added the Build build, makefile, cmake, scripts label Jan 24, 2024
@adamretter
Copy link
Collaborator

@ajkr @pdillinger Can we get this one imported please?

@facebook-github-bot
Copy link
Contributor

@ajkr has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@ajkr merged this pull request in 054c00e.

facebook-github-bot pushed a commit that referenced this pull request Feb 22, 2024
Summary:
#12247 imported another typo in cmakelists.txt and findzstd.cmake.
cmake report ZSTD_INCLUDE_DIRS not found.
Actually it should be
https://github.com/facebook/rocksdb/blob/aacf60dda2a138f9d3826c25818a3bcf250859fd/cmake/modules/Findzstd.cmake#L8

Pull Request resolved: #12309

Reviewed By: hx235

Differential Revision: D54070348

Pulled By: ajkr

fbshipit-source-id: eaf6e260ea3669b8ea38e4c74a375bb885761b51
girlbossceo added a commit to girlbossceo/rocksdb that referenced this pull request Mar 28, 2024
@georgthegreat
Copy link
Contributor

georgthegreat commented Apr 2, 2024

This PR broke nixpkgs build. It looks like their version of zstd does not export ZSTD_INCLUDE_DIRS

@rhubner
Copy link
Contributor Author

rhubner commented Apr 2, 2024

Hello @georgthegreat,

This PR broke nixpkgs build. It looks like their version of zstd does not export ZSTD_INCLUDE_DIRS

Can you please provide more details? Link to CI/CD, reported issue in nixpkgs, ...

Radek

@georgthegreat
Copy link
Contributor

We have an internal system built atop of nixpkgs.
I am receiving the following error from it:

-- Found assembler: /nix/store/90h6k8ylkgn81k10190v5c9ldyjpzgl9-gcc-wrapper-12.3.0/bin/gcc
-- Detecting CXX compiler ABI info
-- Detecting CXX compiler ABI info - done
-- Check for working CXX compiler: /nix/store/90h6k8ylkgn81k10190v5c9ldyjpzgl9-gcc-wrapper-12.3.0/bin/g++ - skipped
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /nix/store/90h6k8ylkgn81k10190v5c9ldyjpzgl9-gcc-wrapper-12.3.0/bin/gcc - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Found ZLIB: /nix/store/8xgb8phqmfn9h971q7dg369h647i1aa0-zlib-1.3/lib/libz.so (found version "1.3")
-- Found BZip2: /nix/store/155qsyx1mv11fsi48nz4dlc0vh1a3drx-bzip2-1.0.8/lib/libbz2.so (found version "1.0.8")
-- Looking for BZ2_bzCompressInit
-- Looking for BZ2_bzCompressInit - found
-- Found lz4: /nix/store/874gxh0sgwkbwb7nfbp72sn7a058b7sa-lz4-1.9.4/lib/liblz4.so
CMake Error at /nix/store/vnhl4zdy7igx9gd3q1d548vwzz15a9ma-cmake-3.27.7/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find zstd (missing: ZSTD_INCLUDE_DIRS)
Call Stack (most recent call first):
  /nix/store/vnhl4zdy7igx9gd3q1d548vwzz15a9ma-cmake-3.27.7/share/cmake-3.27/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  cmake/modules/Findzstd.cmake:17 (find_package_handle_standard_args)
  CMakeLists.txt:167 (find_package)

I have worked it around by adding the following:

  cmakeFlags = attrs.cmakeFlags ++ [
    # Configuration fails otherwise as ZSTD_INCLUDE_DIRS is checked instead of ZSTD_INCLUDE_DIR
    "-DZSTD_INCLUDE_DIRS=${zstd.dev}"
  ];

I am not quite familiar with cmake internals to debug this.

@rhubner
Copy link
Contributor Author

rhubner commented Apr 2, 2024

@georgthegreat Thanks for quick response, I will look into at the end of the week. It's possible there is still bug which we introduce during refactoring.

Radek

@rhubner
Copy link
Contributor Author

rhubner commented Apr 10, 2024

Hello @georgthegreat and sorry for late reply,

Unfortunately I don't know nixos, but I can confirm that we recently changed the CMake build script. The refactor follows the pattern which we used to specify include dir for other libraries. Your fix with "-DZSTD_INCLUDE_DIRS=${zstd.dev}" makes sense and sounds legit for me. What I don't understand, how did you build it before without providing libzstd include path.

About CMake. During the build script generation with CMake, you can specify path where you have your libzstd. This allows you to use your version of zstd and not for example system/distribution version. I assume that when you are building nixos, you also build libzstd and then CMake needs to know where it is, if it's not in the default search path.

Radek

@georgthegreat
Copy link
Contributor

I have tried to understand why previous version worked, but I was unable to get to the root of it.
It looks like CMake's find_package does not provide any include-related variables out of the box, so it is up to the specific package manager to decide whether to provide this variables and how to name them.

I agree that -DZSTD_INCLUDE_DIRS solution solves the problems.
I am fine with keeping it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Build build, makefile, cmake, scripts CLA Signed Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Potential bug in the cmake files.
5 participants