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

[rocksdb] update to v8.3.2, add/rework features #29093

Merged
merged 33 commits into from
Jul 7, 2023
Merged

[rocksdb] update to v8.3.2, add/rework features #29093

merged 33 commits into from
Jul 7, 2023

Conversation

yurybura
Copy link
Contributor

This PR adds more build options.

github-actions[bot]
github-actions bot previously approved these changes Jan 21, 2023
View reviewed changes
@yurybura yurybura marked this pull request as ready for review January 21, 2023 15:39
@yurybura yurybura changed the title [recksdb] add/rework features [rocksdb] add/rework features Jan 21, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 22, 2023
View reviewed changes
@BillyONeal BillyONeal added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jan 25, 2023
BillyONeal
BillyONeal requested changes Jan 25, 2023
View reviewed changes
ports/rocksdb/vcpkg.json Outdated Show resolved Hide resolved
ports/rocksdb/portfile.cmake Show resolved Hide resolved
@BillyONeal BillyONeal added requires:author-response requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function requires:author-response labels Jan 25, 2023
BillyONeal
BillyONeal previously approved these changes Jan 27, 2023
View reviewed changes
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Approving this to get my 'request changes' to go away, but I want a team response to adding more features that turn on CPU settings.

@BillyONeal BillyONeal mentioned this pull request Jan 27, 2023
Merged
@yurybura
Copy link
Contributor Author

yurybura commented Jan 27, 2023 •
edited
Loading

Approving this to get my 'request changes' to go away, but I want a team response to adding more features that turn on CPU settings.

Thank you for review. Custom triplet may be better solution, but the original RocksDB CMake contains CPU-related options.

@yurybura yurybura closed this Jan 27, 2023
@yurybura yurybura reopened this Jan 27, 2023
github-actions[bot]
github-actions bot previously approved these changes Jan 27, 2023
View reviewed changes
@JavierMatosD JavierMatosD added category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed labels Feb 16, 2023
@yurybura yurybura marked this pull request as ready for review June 26, 2023 20:34
@yurybura yurybura changed the title [rocksdb] add/rework features [rocksdb] update 8.3.2, add/rework features Jun 26, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Jun 26, 2023

arm-neon-android is not supported in RocksDB. I want to modify supports in vcpkg.json, but I don't know how to do this for mentioned platform.

I think what you want to exclude is 32-bit arm. This means adding & !(arm & !arm64 & android).

I've already decided to leave an entry in ci.baseline.txt.

But this is wrong. That file is for things which generally should work (i.e. are supported in RocksDB) but fail in vcpkg CI, e.g. due to missing SDKs.

@yurybura yurybura changed the title [rocksdb] update 8.3.2, add/rework features [rocksdb] update to v8.3.2, add/rework features Jun 26, 2023
@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label Jun 27, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Jun 27, 2023

These comments aren't resolved for me. I'm not in the position to request changes. But I would like to leave them visible for consideration by maintainers:
#29093 (comment): Don't patch find_dependency(lz4).
#29093 (comment): Feature liburing should be "supports": "linux".

@yurybura
Copy link
Contributor Author

#29093 (comment): Don't patch find_dependency(lz4).

My patch forces config mode search for LZ4 library. I expect the module provided by VCPKG to be used, not custom search (Findlz4.cmake). The custom finder might have a different CMake export target (not lz4::lz4) or some other mismatch. The same changes have been made for other dependencies like Snyppy, ZStd, TBB in this patch file. All patched libraries do not have the standard finder provided by CMake.

#29093 (comment): Feature liburing should be "supports": "linux".

liburing is ON by default in RocksDB:
image
In VCPKG liburing supports only Linux. So, liburing should be ON by default but only for Linux.
I'm just trying to reproduce default ReockDB features...

@yurybura
Copy link
Contributor Author

arm-neon-android is not supported in RocksDB. I want to modify supports in vcpkg.json, but I don't know how to do this for mentioned platform.

I think what you want to exclude is 32-bit arm. This means adding & !(arm & !arm64 & android).

I've already decided to leave an entry in ci.baseline.txt.

But this is wrong. That file is for things which generally should work (i.e. are supported in RocksDB) but fail in vcpkg CI, e.g. due to missing SDKs.

Done.

@yurybura
Copy link
Contributor Author

@MonicaLiu0311 CI failure is not related to my changes. Could u kindly review this PR? Thanks!

@MonicaLiu0311
Copy link
Contributor

@MonicaLiu0311 CI failure is not related to my changes. Could u kindly review this PR? Thanks!

Depend on #32320. Please wait for a while, the fix is in progress.

@MonicaLiu0311 MonicaLiu0311 added the depends:different-pr This PR or Issue depends on a PR which has been filed label Jun 30, 2023
@MonicaLiu0311 MonicaLiu0311 removed the depends:different-pr This PR or Issue depends on a PR which has been filed label Jul 3, 2023
MonicaLiu0311
MonicaLiu0311 previously approved these changes Jul 3, 2023
View reviewed changes
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Jul 3, 2023
@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 5, 2023
vicroms
vicroms requested changes Jul 6, 2023
View reviewed changes
ports/rocksdb/portfile.cmake Outdated Show resolved Hide resolved
@vicroms vicroms removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jul 7, 2023
@JavierMatosD JavierMatosD merged commit 4c39783 into microsoft:master Jul 7, 2023
@yurybura yurybura deleted the rocksdb-features branch July 7, 2023 16:11
@BillyONeal BillyONeal mentioned this pull request Jan 4, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@BillyONeal BillyONeal BillyONeal left review comments

@vicroms vicroms vicroms approved these changes

@MonicaLiu0311 MonicaLiu0311 MonicaLiu0311 left review comments

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

@MonicaLiu0311 MonicaLiu0311

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@yurybura @BillyONeal @MonicaLiu0311 @dg0yt @vicroms @JavierMatosD