Repository navigation
Conversation
352affc to
9fa4004
Compare
9fa4004 to
c136c8b
Compare
9239767 to
99c1202
Compare
…http tiny_http 0.12 unwraps peer_addr() on accept; on Windows it fails with WSAENOTCONN when the client has already closed the connection, killing the worker and failing the next request with EOF.
…LIB_DIR Keep bundled platform crates as the default, with custom-directory and pkg-config overrides selecting static or opt-in dynamic linking. Move the MSVC Go initializer into the sys crate and reject non-x86_64 static MSVC links. pkg-config 0.3.34 parse_libs defaults the non-macOS/non-Windows system root to /usr, and is_static_available requires an archive path outside every system root. Thus statik(true) alone emits an unqualified link for /usr/lib/<triplet>/libdbc.a, allowing the shared library to win. Probe without Cargo link metadata and explicitly emit static=dbc, leaving private system libraries dynamic.
The default build now locates an installed libdbc through the dbc pkg-config module. The prebuilt platform crates move behind the `bundled` feature, and the `pkg-config` feature is removed since it is now the default. DBC_SYS_LIB_DIR and DBC_SYS_USE_PKG_CONFIG keep their precedence over `bundled`.
d200703 to
e74846d
Compare
amoeba
left a comment
There was a problem hiding this comment.
Generally I think this looks fine. I looked closer at some files than others and I expect I may have more review comments once this is all merged together.
| ```rust | ||
| use columnar_dbc::{list_installed, Client, InstallLocation}; | ||
|
|
||
| let client = Client::new()?; |
There was a problem hiding this comment.
| let client = Client::new()?; | |
| // First create a Client | |
| let client = Client::new()?; |
| use columnar_dbc::{list_installed, Client, InstallLocation}; | ||
|
|
||
| let client = Client::new()?; | ||
| for d in client.search("postgres")?.drivers { |
There was a problem hiding this comment.
| for d in client.search("postgres")?.drivers { | |
| // Search for a driver using a pattern | |
| for d in client.search("postgres")?.drivers { |
| println!("{} - {}", d.path, d.description); | ||
| } | ||
|
|
||
| // Into dbc's user-level location, like `dbc install -l user` ... |
There was a problem hiding this comment.
| // Into dbc's user-level location, like `dbc install -l user` ... | |
| // Install a driver to the user level install location. Same behavior as `dbc install -l user`. |
| } | ||
|
|
||
| // Into dbc's user-level location, like `dbc install -l user` ... | ||
| let manifest = client.install("postgresql", &InstallLocation::User)?; |
There was a problem hiding this comment.
Hrm.... So I think the C API and therefore Rust require an explicit install location. I'm now wondering whether we can have bindings for install behave more like the CLI. I think the ideal code here would be,
let driver = client.install("postgresql")?;| let dir = InstallLocation::Directory("/opt/adbc/drivers".into()); | ||
| client.install("postgresql", &dir)?; | ||
| assert_eq!(list_installed(&dir)?.len(), 1); | ||
| # Ok::<(), columnar_dbc::Error>(()) |
There was a problem hiding this comment.
What is this line doing? If this was supposed to be a doctest, I don't think the README is hooked up yet. May as well. I guess you can add this to lib.rs,
#![doc = include_str!("../README.md")]
and fence the codeblock with ````rust,no_run`.
| `install` verifies each driver's signature with Columnar's key and removes a | ||
| driver that fails. For drivers from a trusted private registry, |
There was a problem hiding this comment.
| `install` verifies each driver's signature with Columnar's key and removes a | |
| driver that fails. For drivers from a trusted private registry, | |
| `install` verifies each driver's signature with Columnar's public signing key and cleans up a | |
| driver that fails verification. For drivers from a trusted private registry, |
| User, | ||
| /// dbc's system-level location, as `dbc install -l system`; usually needs | ||
| /// elevated privileges. | ||
| System, |
There was a problem hiding this comment.
Same comment as in earlier PRs. dbc's config.ConfigLevel is System|User|Env. I'm not sure if it matters to align.
|
|
||
| #[derive(Debug, Clone)] | ||
| #[non_exhaustive] | ||
| pub struct Driver { |
There was a problem hiding this comment.
Should this struct be missing the other fields in the registry index like urls, docs_url, pkginfo, and an association to a registry so we can disambiguate drivers with the same path on multiple registries?
| let manifest = client.install("postgresql", &InstallLocation::User)?; | ||
| println!("installed {} {} at {}", manifest.id, manifest.version, manifest.driver_path); | ||
|
|
||
| // ... or into an explicit directory. |
There was a problem hiding this comment.
| // ... or into an explicit directory. | |
| // You can also install the driver to a specific directory. |
I don't really like how these PR stacks are exposing this new way of installing drivers. The old way was to set ADBC_DRIVER_PATH before doing things and now we have two ways.
rust/:columnar-dbc(safe API),columnar-dbc-sys(bindgen 0.73.2 bindings generated fromlibdbc.h, committed) and fivecolumnar-dbc-sys-<platform>crates, each bundling a prebuiltlibdbc.a. Supported targets: x86_64/aarch64 Linux (glibc), x86_64/aarch64 macOS,x86_64-pc-windows-msvcandx86_64-pc-windows-gnu. Other targets fail at build time.MSVC: a Go c-archive registers its runtime entry in
.ctors, which only the MinGW CRT runs. Under MSVC the first dbc call blocked forever. The Windows platform crate registers_rt0_amd64_windows_libin.CRT$XCUfortarget_env = "msvc". A localcargo xwinlink map shows the pointer in the CRT'sXCA..XCZinitializer table. CI in the top PR of the stack runs the tests.Stacked on #547.
Linking libdbc
Modeled on
zstd-sys, with pkg-config as the default. The source is chosen in this order:DBC_SYS_LIB_DIR=<dir>: a custom directory.DBC_SYS_USE_PKG_CONFIGset: pkg-config moduledbc, even withbundledon.bundledfeature: the per-platform prebuilt crates.dbc.By default the platform crates aren't downloaded at all;
features = ["bundled"]opts into them. Env overrides win even whenbundledis on: the platform crate is neverextern crated, so its archive isn't linked. docs.rs builds withbundledsince it has no installed libdbc.Linking is static by default;
DBC_DYNAMIC(pkg-config-rs's own convention) linkslibdbc.so. pkg-config-rs refuses static links for archives under/usr, sobuild.rsemits the link directives itself rather than using.statik(true).The MSVC Go runtime initializer moved from the Windows platform crate into
-sys. It's applied to any static x86_64 MSVC link, and static MSVC links on other architectures fail with a clear error.