feat: Added optional platform selection for driver install - #466
Open
wbeardall wants to merge 1 commit into
Open
feat: Added optional platform selection for driver install#466wbeardall wants to merge 1 commit into
wbeardall wants to merge 1 commit into
Conversation
…to host platform, so standard install use-case is unaffected. WASM ops do not expose platform selection.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
This is a contribution in response to Issue #50
The idea is to expand platform resolution, enabling users to install drivers which target non-host platforms when using the
dbc installcommand.Goals and Constraints
dbc installbehaviour; non-host always opt-inplatform.go, worth considering in case someone ends up adding weird and wonderful supported platforms to the CDNwindows_arm64forsqlite. In this case, the standardError: no package found for platform 'windows_arm64'path occurs.Potential Concerns and Considerations
The main concern I have currently is that
dbc listreads installed manifests without confirming that the driver is actually installed for the host machine. This has the potential for user confusion, as they might have a non-host driver installed, and assume that the entry indbc listmeans that they can use that driver with ADBC now!I deliberately haven't changed the
dbc listbehaviour, because it has the potential for a bigger direct impact on user experience than the rest of this PR. That said, this is how I'd go about modifyingdbc list:--all-platformsflag to thedbc list commanddbc listshould filter out any drivers without a host lib explicitly linked in the<driver>.toml, and then print exactly as current--all-platformsflag, thedbc listtable would expand with anotherPLATFORMcolumn, with the host platform being marked explicitly (e.g.linux_amd64 (*))I've put together a follow-up PR #467 implementing this.
Tests
config/platform_test.go(new file)TestPlatformUnmarshalText
TestPlatformResolve
config/config_api_test.goTestInstallDriver/records_explicit_platform_in_manifest
cmd/dbc/main_test.goTestInstallInvalidPlatformRejectedAtParse
TestInstallHelpMentionsVersionConstraints (extended)
cmd/dbc/install_test.goTestInstallWithPlatform (SubcommandTestSuite)
client_methods_test.goTestClientInstall/installs_driver_for_explicit_platform
Updated Tests
TestClientInstall, TestClientUninstall, TestInstallDriver/success,
TestInstallDriver/invalid_tarball — updated for new Install/InstallDriver signatures
Notes
gofmt-ed the PR specifically!