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.

Sign up for GitHub

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

Jump to bottom

Fix zstd typo in cmake #12309

Closed
wants to merge 1 commit into from
Closed

Fix zstd typo in cmake #12309

wants to merge 1 commit into from

Conversation

raffertyyu
Copy link
Contributor

#12247 imported another typo in cmakelists.txt and findzstd.cmake.
cmake report ZSTD_INCLUDE_DIRS not found.
Actually it should be

rocksdb/cmake/modules/Findzstd.cmake

Line 8 in aacf60d

find_path(zstd_INCLUDE_DIRS

@rhubner
Copy link
Contributor

rhubner commented Jan 30, 2024

Hello @raffertyyu,

Thank you for your PR. I think you are right. There is a bug. But if I can suggest, I will prefer to make fix here : https://github.com/facebook/rocksdb/blob/main/cmake/modules/Findzstd.cmake#L28

  INTERFACE_INCLUDE_DIRECTORIES ${zstd_INCLUDE_DIRS}) <-- Current code
  INTERFACE_INCLUDE_DIRECTORIES ${ZSTD_INCLUDE_DIRS}) <-- New fix

Then all the variables for ZSTD will be uppercase. Please correct me if I'm wrong, but uppercase variables are a common standard in CMake.

@raffertyyu raffertyyu force-pushed the fixCmakeTypo branch 2 times, most recently from 57f419e to 3a4a218 Compare January 30, 2024 08:54
@raffertyyu raffertyyu mentioned this pull request Feb 18, 2024
Closed
@raffertyyu raffertyyu force-pushed the fixCmakeTypo branch 2 times, most recently from a6b02fc to c390048 Compare February 18, 2024 03:39
@Mis1eader-dev
Copy link
Contributor

Is this fix not going to be merged?

@adamretter
Copy link
Collaborator

@Mis1eader-dev We are waiting for @raffertyyu to answer @rhubner's questions...

@raffertyyu
Copy link
Contributor Author

Hello @raffertyyu,

Thank you for your PR. I think you are right. There is a bug. But if I can suggest, I will prefer to make fix here : https://github.com/facebook/rocksdb/blob/main/cmake/modules/Findzstd.cmake#L28

  INTERFACE_INCLUDE_DIRECTORIES ${zstd_INCLUDE_DIRS}) <-- Current code
  INTERFACE_INCLUDE_DIRECTORIES ${ZSTD_INCLUDE_DIRS}) <-- New fix

Then all the variables for ZSTD will be uppercase. Please correct me if I'm wrong, but uppercase variables are a common standard in CMake.

Hi @rhubner sorry for not replying in time. I agree that cmake vars should be UPPERCASE. All zstd vars are modified in this commit. Thanks for suggestion.

@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 e09b9d0.

girlbossceo added a commit to girlbossceo/conduwuit that referenced this pull request Mar 22, 2024
@girlbossceo
Revert RocksDB 9.0.0 update
e5d2b1a 
there seems to be a regression, likely from facebook/rocksdb#12361 / facebook/rocksdb#12309

```
[1/0/2 built] building rocksdb-9.0.0 (configurePhase): -- Detecting CXX compile features - donedirenv: ([/Users/strawberry/.nix-profile/bin/direnv export zsh]) is taking a while to execute. Use CTRL-C to give up.
error: builder for '/nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv' failed with exit code 1;
       last 10 log lines:
       > -- Found lz4: /nix/store/cafwv4439qbm2ij04mpc7xz5m3f7mfix-lz4-1.9.4/lib/liblz4.dylib
       > CMake Error at /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
       >   Could NOT find zstd (missing: ZSTD_INCLUDE_DIRS)
       > Call Stack (most recent call first):
       >   /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
       >   cmake/modules/Findzstd.cmake:17 (find_package_handle_standard_args)
       >   CMakeLists.txt:167 (find_package)
       >
       >
       > -- Configuring incomplete, errors occurred!
       For full logs, run 'nix log /nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv'.
error: 1 dependencies of derivation '/nix/store/ir8jf2wic98iymjlk7d2i1kjjsgv15v2-nix-shell-env.drv' failed to build
```

happens in both rust-rocksdb and our fork of rust-rocksdb

Signed-off-by: strawberry <strawberry@puppygock.gay>
girlbossceo added a commit to girlbossceo/conduwuit that referenced this pull request Mar 22, 2024
@girlbossceo
Revert RocksDB 9.0.0 update
634d2db 
there seems to be a regression, likely from facebook/rocksdb#12361 / facebook/rocksdb#12309

```
[1/0/2 built] building rocksdb-9.0.0 (configurePhase): -- Detecting CXX compile features - donedirenv: ([/Users/strawberry/.nix-profile/bin/direnv export zsh]) is taking a while to execute. Use CTRL-C to give up.
error: builder for '/nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv' failed with exit code 1;
       last 10 log lines:
       > -- Found lz4: /nix/store/cafwv4439qbm2ij04mpc7xz5m3f7mfix-lz4-1.9.4/lib/liblz4.dylib
       > CMake Error at /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
       >   Could NOT find zstd (missing: ZSTD_INCLUDE_DIRS)
       > Call Stack (most recent call first):
       >   /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
       >   cmake/modules/Findzstd.cmake:17 (find_package_handle_standard_args)
       >   CMakeLists.txt:167 (find_package)
       >
       >
       > -- Configuring incomplete, errors occurred!
       For full logs, run 'nix log /nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv'.
error: 1 dependencies of derivation '/nix/store/ir8jf2wic98iymjlk7d2i1kjjsgv15v2-nix-shell-env.drv' failed to build
```

happens in both rust-rocksdb and our fork of rust-rocksdb

Signed-off-by: strawberry <strawberry@puppygock.gay>
girlbossceo added a commit to girlbossceo/conduwuit that referenced this pull request Mar 23, 2024
@girlbossceo
Revert RocksDB 9.0.0 update
d7fc687 
there seems to be a regression, likely from facebook/rocksdb#12361 / facebook/rocksdb#12309

```
[1/0/2 built] building rocksdb-9.0.0 (configurePhase): -- Detecting CXX compile features - donedirenv: ([/Users/strawberry/.nix-profile/bin/direnv export zsh]) is taking a while to execute. Use CTRL-C to give up.
error: builder for '/nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv' failed with exit code 1;
       last 10 log lines:
       > -- Found lz4: /nix/store/cafwv4439qbm2ij04mpc7xz5m3f7mfix-lz4-1.9.4/lib/liblz4.dylib
       > CMake Error at /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
       >   Could NOT find zstd (missing: ZSTD_INCLUDE_DIRS)
       > Call Stack (most recent call first):
       >   /nix/store/bin32lqag7lx38994xpf9jvhk1xbd64c-cmake-3.28.2/share/cmake-3.28/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
       >   cmake/modules/Findzstd.cmake:17 (find_package_handle_standard_args)
       >   CMakeLists.txt:167 (find_package)
       >
       >
       > -- Configuring incomplete, errors occurred!
       For full logs, run 'nix log /nix/store/9slwgpnardhn2vqzqhn361ic668n38wq-rocksdb-9.0.0.drv'.
error: 1 dependencies of derivation '/nix/store/ir8jf2wic98iymjlk7d2i1kjjsgv15v2-nix-shell-env.drv' failed to build
```

happens in both rust-rocksdb and our fork of rust-rocksdb

Signed-off-by: strawberry <strawberry@puppygock.gay>
@yihuang yihuang mentioned this pull request Apr 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajkr ajkr ajkr approved these changes

Assignees
No one assigned
Labels
CLA Signed Merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@raffertyyu @rhubner @Mis1eader-dev @adamretter @facebook-github-bot @ajkr