Skip to content

feat(c/driver_manager): don't ignore invalid manifests #3399

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
Merged
lidavidm merged 2 commits into from
Sep 8, 2025
Merged

feat(c/driver_manager): don't ignore invalid manifests #3399

lidavidm merged 2 commits into from
Sep 8, 2025

Conversation

@lidavidm
Copy link
Member

@lidavidm lidavidm commented Sep 7, 2025

Fixes #3398.

@lidavidm lidavidm marked this pull request as ready for review September 7, 2025 05:32
@lidavidm lidavidm requested review from amoeba and zeroshade September 7, 2025 05:32
@github-actions github-actions bot added this to the ADBC Libraries 20 milestone Sep 7, 2025
@lidavidm lidavidm requested a review from ianmcook September 7, 2025 05:33
@lidavidm lidavidm marked this pull request as draft September 7, 2025 05:33
@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2025

@amoeba how does this work for you?

I think the one case that isn't handled, though, is if it finds a manifest, the manifest is valid, but the manifest doesn't contain a path for the current platform.

@zeroshade
Copy link
Member

zeroshade commented Sep 7, 2025

Don't forget to update the version in the go codebase (precommit checks for it) and the original reason why I returned "NOT_FOUND" in this case was so that it would keep searching if it encountered an invalid manifest rather than error immediately.

@lidavidm
Copy link
Member Author

lidavidm commented Sep 7, 2025 •
edited
Loading

I figured, but I'm asking if silently ignoring invalid manifests is really the behavior we want. And yes, this PR is incomplete

@lidavidm
Copy link
Member Author

lidavidm commented Sep 8, 2025

This is a bit unwieldy but I think it's better to just preserve all info even if it's a bit ugly:

[Driver Manager] dlopen() failed: sqlite: cannot open shared object file: No such file or directory
dlopen() failed: libsqlite.so: cannot open shared object file: No such file or directory
Also searched these paths for manifests:
	not set: ADBC_DRIVER_PATH
	additional search path: /tmp/adbc_driver_manager_test
	not enabled at build time: Conda prefix
	found /tmp/adbc_driver_manager_test/sqlite.toml but:[Driver Manager] Driver path not found in manifest '/tmp/adbc_driver_manager_test/sqlite.toml' for current architecture 'linux_amd64'. Architectures found: non-existent windows-alpha64
	does not exist: /home/lidavidm/.config/adbc/drivers
	does not exist: /etc/adbc/drivers

@lidavidm
Copy link
Member Author

lidavidm commented Sep 8, 2025 

adbc_driver_manager.ProgrammingError: NOT_FOUND: [Driver Manager] dlopen() failed: testdriver: cannot open shared object file: No such file or directory
dlopen() failed: libtestdriver.so: cannot open shared object file: No such file or directory
Also searched these paths for manifests:
	ADBC_DRIVER_PATH: /home/lidavidm/Code/arrow-adbc
	not enabled at build time: Conda prefix
	found /home/lidavidm/Code/arrow-adbc/testdriver.toml but: [Driver Manager] dlopen() failed: sqlite: cannot open shared object file: No such file or directory
dlopen() failed: libsqlite.so: cannot open shared object file: No such file or directory
	does not exist: /home/lidavidm/.config/adbc/drivers
	does not exist: /etc/adbc/drivers

@lidavidm
Copy link
Member Author

lidavidm commented Sep 8, 2025

@amoeba @ianmcook see the new error messages here + new Python integration test

@lidavidm lidavidm requested a review from paleolimbot September 8, 2025 08:41
@lidavidm
Copy link
Member Author

lidavidm commented Sep 8, 2025

Also @zeroshade the go file consistency check has been broken for a while it appears so I fixed that too

@lidavidm lidavidm force-pushed the gh-3398 branch 7 times, most recently from ccef136 to 592298a Compare September 8, 2025 12:08
@lidavidm
Copy link
Member Author

lidavidm commented Sep 8, 2025

welp I managed to guess the problem after all

@lidavidm lidavidm marked this pull request as ready for review September 8, 2025 13:46
@lidavidm lidavidm requested a review from kou as a code owner September 8, 2025 13:46
zeroshade
zeroshade reviewed Sep 8, 2025
View reviewed changes
zeroshade
zeroshade reviewed Sep 8, 2025
View reviewed changes
zeroshade
zeroshade approved these changes Sep 8, 2025
View reviewed changes
Copy link
Member

@zeroshade zeroshade left a comment

Choose a reason for hiding this comment

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

just the one nitpick, but everything looks great, thanks!

amoeba
amoeba approved these changes Sep 8, 2025
View reviewed changes
Copy link
Member

@amoeba amoeba left a comment

Choose a reason for hiding this comment

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

+1

Tested interactively and this is working great. Thanks for figuring this out.

@lidavidm lidavidm merged commit 69ba923 into apache:main Sep 8, 2025
72 checks passed
@lidavidm lidavidm deleted the gh-3398 branch September 8, 2025 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@amoeba amoeba amoeba approved these changes

@zeroshade zeroshade zeroshade approved these changes

@ianmcook ianmcook Awaiting requested review from ianmcook

@paleolimbot paleolimbot Awaiting requested review from paleolimbot

@kou kou Awaiting requested review from kou kou is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

ADBC Libraries 20

Development

Successfully merging this pull request may close these issues.

bug(c/driver_manager): AbdcFindLoadDriver eats inner error when using driver manifest

3 participants

@lidavidm @zeroshade @amoeba