Skip to content

Conversation

@KinomotoMio
Copy link
Contributor

@KinomotoMio KinomotoMio commented Aug 12, 2025

Which issue does this PR close?

Closes #6250.

Rationale for this change

Currently, the C++ binding hardcodes all storage service features in Cargo.toml, which means:

  • Users must compile and link all services regardless of their actual needs
  • Binary size is unnecessarily large
  • Compilation time is longer than necessary

This PR implements user-configurable features similar to the C binding (#4313), allowing users to specify only the storage services they need at build time.

What changes are included in this PR?

  • Modified bindings/cpp/Cargo.toml: Removed hardcoded service dependencies, keeping only the essential blocking feature
  • Enhanced bindings/cpp/CMakeLists.txt:
    • Added FEATURES cache variable for user configuration
    • Implemented intelligent feature merging logic that combines user-specified services with async support
    • Maintains backward compatibility with existing OPENDAL_ENABLE_ASYNC option
  • Removed obsolete feature handling: Cleaned up the previous hardcoded async feature logic

Are there any user-facing changes?

  • New option: FEATURES - allows users to specify which OpenDAL storage services to include
  • Behavior change: By default, no storage services are included (users must explicitly specify needed services)
  • Backward compatibility: Existing usage of OPENDAL_ENABLE_ASYNC continues to work unchanged
  • Migration: Users upgrading will need to add -DFEATURES="service1,service2" to their cmake commands to include storage services

Remove hardcoded service dependencies to enable user-configurable features.
  Only keep 'blocking' feature as it's required for C++ bindings.

Related to apache#6250
@KinomotoMio
Copy link
Contributor Author

@asukaminato0721

@asukaminato0721
Copy link
Contributor

refer to here

run: cargo build --features="opendal/${{ matrix.cases.feature }}"

@asukaminato0721
Copy link
Contributor

cmake \
-DCMAKE_BUILD_TYPE=Release \
-DOPENDAL_ENABLE_TESTING=ON \
-DOPENDAL_DEV=ON \
-DOPENDAL_ENABLE_ASYNC=ON \
-DCMAKE_CXX_STANDARD=20 \
..

Maybe you need to add the feature config here.

@asukaminato0721
Copy link
Contributor

asukaminato0721 commented Aug 12, 2025

/home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/rocksdb-0.21.0/src/ffi_util.rs:75:
(.text._ZN7rocksdb2db21DBCommon$LT$T$C$D$GT$3get17h65f01d16dcc18acaE+0x52): undefined reference to 
`rocksdb_get_pinned'

can't understand...

@asukaminato0721
Copy link
Contributor

monoiofs

==7264==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 608 byte(s) in 38 object(s) allocated from:
    #0 0x55c2f57a72e3 in malloc (/home/runner/work/opendal/opendal/bindings/cpp/build/opendal_cpp_test+0x1a82e3) (BuildId: a9450bbf7a6088b8d60b7ed549bd88dfc165fab6)

Alluxio doesn't support file overwrite without explicit overwrite option.

Related to apache#6250
Alluxio doesn't support file overwrite without explicit overwrite option.

Related to apache#6250
Alluxio doesn't support file overwrite without explicit overwrite option.

Related to apache#6250
Alluxio doesn't support file overwrite without explicit overwrite option.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
Alluxio has different list behavior that includes unexpected paths.

Related to apache#6250
@KinomotoMio KinomotoMio force-pushed the feat/cpp-binding-configurable-features branch from efb120e to 6129ffd Compare August 13, 2025 04:40
@asukaminato0721
Copy link
Contributor

seems only memory leak now.

@Xuanwo
Copy link
Member

Xuanwo commented Aug 14, 2025

seems only memory leak now.

Thank you for the work, I think we can disable cpp test on those two services for now.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Overall LGTM, thank you for your work!

@KinomotoMio KinomotoMio force-pushed the feat/cpp-binding-configurable-features branch from dce280e to ae941db Compare August 14, 2025 21:50
@KinomotoMio KinomotoMio marked this pull request as ready for review August 14, 2025 22:23
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/feat The PR implements a new feature or has a title that begins with "feat" labels Aug 14, 2025
@asukaminato0721 asukaminato0721 self-requested a review August 14, 2025 23:41
@asukaminato0721 asukaminato0721 merged commit 08880ef into apache:main Aug 14, 2025
64 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

new feature: cpp binding enable feature by user

3 participants