fix(rust/driver_manager): remove optional property for windows deps #3693
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Pavel Agafonov <[email protected]>
Member
|
Hi @if0ne, thanks for the PR. I think you're right. Marking the deps as optional without an enabling feature doesn't seem right. Marking this as optional goes back to 92c26ec. The intention may have been to make the ADBC Driver Manifest support optional on Windows but it would make more sense to do that as a feature flag. @eitsupi do you want to take a look here? |
Contributor
|
Thanks for pointing that out, it's definitely my mistake in #3197. |
eitsupi
approved these changes
Nov 8, 2025
Contributor
eitsupi
left a comment
eitsupi
left a comment
There was a problem hiding this comment.
In the current version, Windows-specific dependencies have been mistakenly made into optional features.
❯ cargo add adbc_driver_manager
Updating crates.io index
Adding adbc_driver_manager v0.21.0 to dependencies
Features:
- driver_manager_test_lib
- driver_manager_test_manifest_user
- windows-registry
- windows-sys
Updating crates.io index
Locking 69 packages to latest Rust 1.91.0 compatible versionsThis issue will be resolved by this.
❯ cargo add adbc_driver_manager --git https://github.com/if0ne/arrow-adbc
Updating git repository `https://github.com/if0ne/arrow-adbc`
Updating git submodule `https://github.com/apache/arrow-testing.git`
Adding adbc_driver_manager (git) to dependencies
Features:
- driver_manager_test_lib
- driver_manager_test_manifest_user
Updating git repository `https://github.com/if0ne/arrow-adbc`
Updating crates.io index
Locking 71 packages to latest Rust 1.91.0 compatible versionsThank you!
Member
|
A good follow up here would be to add tests of adbc-driver-manager in the CI so we can catch this in the future. |
amoeba
added a commit
that referenced
this pull request
Nov 12, 2025
Adds regression tests for the Windows-only issue found in #3693. Changes: - Adds to basic regression tests to the `driver_manager` crate tests which ensure the Windows-only deps are pulled in at build/test time - Changes the Rust workflow (`rust.yml`) to now also run tests with default features, in addition to all features. We were previously only running the tests with all features enabled which was hiding the mistake originally caught in #3693. Now we run tests both ways. - Minor: Adds the Debug trait to the `DriverInfo` struct so the above tests pass
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Hi there! When using the
adbc_driver_managercrate on Windows, it's impossible to build the project because theadbc_driver_managerdependency fails to compile. Windows crates are marked as optional and are not included in any of the crate's features. To make it work, theoptionalflag needs to be removed.