From 3c6dc2a979351c6c4ccae46a78aa19a5adf404c6 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 12:05:30 +0530 Subject: [PATCH 01/18] feat(adbc): scaffold ampup adbc subcommand Add an `adbc` subcommand (install/list/uninstall) mirroring the existing `self` subcommand structure. Runners are stubs that fail loudly until the download/placement path lands. Part of edgeandnode/amp#2600 (optional ADBC driver support in ampup). --- ampup/src/commands.rs | 1 + ampup/src/commands/adbc.rs | 21 +++++++++++++++++++++ ampup/src/main.rs | 35 +++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+) create mode 100644 ampup/src/commands/adbc.rs diff --git a/ampup/src/commands.rs b/ampup/src/commands.rs index a361ccf..b64901c 100644 --- a/ampup/src/commands.rs +++ b/ampup/src/commands.rs @@ -1,3 +1,4 @@ +pub mod adbc; pub mod build; pub mod init; pub mod install; diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs new file mode 100644 index 0000000..73ce420 --- /dev/null +++ b/ampup/src/commands/adbc.rs @@ -0,0 +1,21 @@ +//! Optional ADBC driver components (#2600). +//! +//! Installs destination-specific ADBC driver libraries from the pinned set +//! shipped with each Amp release, so `ampd` can load them at runtime. + +use anyhow::{Result, bail}; + +/// Install an ADBC driver for the active amp version. +pub async fn install(driver: &str) -> Result<()> { + bail!("`ampup adbc install {driver}` is not yet implemented"); +} + +/// List installed ADBC drivers for the active version. +pub fn list() -> Result<()> { + bail!("`ampup adbc list` is not yet implemented"); +} + +/// Uninstall an ADBC driver. +pub fn uninstall(driver: &str) -> Result<()> { + bail!("`ampup adbc uninstall {driver}` is not yet implemented"); +} diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 449cc2a..5d7a1e1 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -158,6 +158,12 @@ enum Commands { #[command(subcommand)] command: SelfCommands, }, + + /// Manage optional ADBC driver components + Adbc { + #[command(subcommand)] + command: AdbcCommands, + }, } #[derive(Debug, clap::Subcommand)] @@ -177,6 +183,24 @@ enum SelfCommands { Version, } +#[derive(Debug, clap::Subcommand)] +enum AdbcCommands { + /// Install an ADBC driver for the active amp version + Install { + /// Driver to install (e.g. postgresql) + driver: String, + }, + + /// List installed ADBC drivers for the active version + List, + + /// Uninstall an ADBC driver + Uninstall { + /// Driver to uninstall (e.g. postgresql) + driver: String, + }, +} + #[tokio::main] async fn main() { if let Err(e) = run().await { @@ -266,6 +290,17 @@ async fn run() -> anyhow::Result<()> { println!("ampup {}", env!("VERGEN_GIT_DESCRIBE")); } }, + Some(Commands::Adbc { command }) => match command { + AdbcCommands::Install { driver } => { + commands::adbc::install(&driver).await?; + } + AdbcCommands::List => { + commands::adbc::list()?; + } + AdbcCommands::Uninstall { driver } => { + commands::adbc::uninstall(&driver)?; + } + }, None => { // Default: install latest version (same as 'ampup update') commands::install::run( From 0d8168ce9640cd26c18dab79e2a7aca80f71a1cc Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 12:26:43 +0530 Subject: [PATCH 02/18] feat(adbc): add driver catalog and release-asset naming Add an `adbc` module with a `Driver` catalog (postgresql) and `asset_name`, mapping a driver + platform/arch to the release asset `adbc-driver---.tar.gz`. Wire the command runners to validate the driver name against the catalog. Part of edgeandnode/amp#2600. --- ampup/src/adbc.rs | 73 ++++++++++++++++++++++++++++++++++++++ ampup/src/commands/adbc.rs | 19 +++++++++- ampup/src/lib.rs | 1 + 3 files changed, 92 insertions(+), 1 deletion(-) create mode 100644 ampup/src/adbc.rs diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs new file mode 100644 index 0000000..cf1344a --- /dev/null +++ b/ampup/src/adbc.rs @@ -0,0 +1,73 @@ +//! ADBC driver catalog and release-asset naming. +//! +//! Amp releases ship a pinned set of ADBC driver libraries as archive assets. +//! This module maps a supported driver plus a target platform/arch to the +//! release asset that carries it. + +use crate::platform::{Architecture, Platform}; + +/// A supported ADBC driver, as shipped in Amp releases. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Driver { + Postgresql, +} + +impl Driver { + /// Every supported driver. + pub const ALL: &'static [Driver] = &[Driver::Postgresql]; + + /// The driver's canonical name (the `` segment in asset names). + pub fn as_str(&self) -> &'static str { + match self { + Self::Postgresql => "postgresql", + } + } + + /// Parse a driver from its canonical name, or `None` if unsupported. + pub fn from_name(name: &str) -> Option { + Self::ALL.iter().copied().find(|d| d.as_str() == name) + } +} + +impl std::fmt::Display for Driver { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.as_str()) + } +} + +/// The GitHub release asset name for a driver on a given target. +/// +/// Must match the naming the release pipeline produces (edgeandnode/amp #2599): +/// `adbc-driver---.tar.gz`. +pub fn asset_name(driver: Driver, platform: Platform, arch: Architecture) -> String { + format!( + "adbc-driver-{}-{}-{}.tar.gz", + driver.as_str(), + platform.as_str(), + arch.as_str(), + ) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn asset_name_matches_release_contract() { + assert_eq!( + asset_name(Driver::Postgresql, Platform::Linux, Architecture::X86_64), + "adbc-driver-postgresql-linux-x86_64.tar.gz", + ); + assert_eq!( + asset_name(Driver::Postgresql, Platform::Darwin, Architecture::Aarch64), + "adbc-driver-postgresql-darwin-aarch64.tar.gz", + ); + } + + #[test] + fn from_name_accepts_supported_and_rejects_unknown() { + assert_eq!(Driver::from_name("postgresql"), Some(Driver::Postgresql)); + assert_eq!(Driver::from_name("mysql"), None); + assert_eq!(Driver::from_name(""), None); + } +} diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 73ce420..858b9ba 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,10 +3,13 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use anyhow::{Result, bail}; +use anyhow::{Result, anyhow, bail}; + +use crate::adbc::Driver; /// Install an ADBC driver for the active amp version. pub async fn install(driver: &str) -> Result<()> { + let driver = parse_driver(driver)?; bail!("`ampup adbc install {driver}` is not yet implemented"); } @@ -17,5 +20,19 @@ pub fn list() -> Result<()> { /// Uninstall an ADBC driver. pub fn uninstall(driver: &str) -> Result<()> { + let driver = parse_driver(driver)?; bail!("`ampup adbc uninstall {driver}` is not yet implemented"); } + +/// Resolve a driver name against the catalog, with a helpful error listing the +/// supported drivers. +fn parse_driver(name: &str) -> Result { + Driver::from_name(name).ok_or_else(|| { + let supported = Driver::ALL + .iter() + .map(Driver::as_str) + .collect::>() + .join(", "); + anyhow!("unknown ADBC driver `{name}` (supported: {supported})") + }) +} diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index fa2c81f..262806f 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -1,3 +1,4 @@ +pub mod adbc; pub mod builder; pub mod commands; pub mod config; From b44c5e3fd8d45807a275298cdd949b21d57b6903 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 13:13:15 +0530 Subject: [PATCH 03/18] feat(download): verify artifact SHA-256 against release digest Read the digest GitHub advertises for each release asset and verify the downloaded bytes against it, failing with ChecksumMismatch on a mismatch. Applies to every download (binaries and, later, drivers). Previously only a non-empty check ran. - github: Asset/ResolvedAsset carry an optional digest. - download_manager: verify_artifact checks SHA-256 when a digest is present. - tests: verify_artifact match/mismatch/prefix/no-digest, plus end-to-end matching and mismatched-digest cases through download_all. Part of edgeandnode/amp#2600. --- Cargo.lock | 72 +++++++++++ ampup/Cargo.toml | 1 + ampup/src/download_manager.rs | 224 +++++++++++++++++++++++++++++++--- ampup/src/github.rs | 6 + 4 files changed, 288 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2db55cb..542b263 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -15,6 +15,7 @@ dependencies = [ "reqwest", "semver", "serde", + "sha2", "tempfile", "tokio", "vergen-gitcl", @@ -122,6 +123,15 @@ version = "2.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "812e12b5285cc515a9c72a5c1d3b6d46a19dac5acfef5265968c166106e31dd3" +[[package]] +name = "block-buffer" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3078c7629b62d3f0439517fa394996acacc5cbc91c5a20d8c658e77abd503a71" +dependencies = [ + "generic-array", +] + [[package]] name = "bumpalo" version = "3.19.1" @@ -258,6 +268,25 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "cpufeatures" +version = "0.2.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "59ed5838eebb26a2bb2e58f6d5b5316989ae9d08bab10e0e6d103e656d1b0280" +dependencies = [ + "libc", +] + +[[package]] +name = "crypto-common" +version = "0.1.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "78c8292055d1c1df0cce5d180393dc8cce0abec0a7102adb6c7b1eef6016d60a" +dependencies = [ + "generic-array", + "typenum", +] + [[package]] name = "darling" version = "0.20.11" @@ -345,6 +374,16 @@ dependencies = [ "zeroize", ] +[[package]] +name = "digest" +version = "0.10.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9ed9a281f7bc9b7576e61468ba615a66a5c8cfdff42420a70aa82701a3b1e292" +dependencies = [ + "block-buffer", + "crypto-common", +] + [[package]] name = "displaydoc" version = "0.2.5" @@ -521,6 +560,16 @@ dependencies = [ "slab", ] +[[package]] +name = "generic-array" +version = "0.14.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "85649ca51fd72272d7821adaf274ad91c288277713d9c18820d8499a7ff69e9a" +dependencies = [ + "typenum", + "version_check", +] + [[package]] name = "getrandom" version = "0.2.17" @@ -1438,6 +1487,17 @@ dependencies = [ "serde", ] +[[package]] +name = "sha2" +version = "0.10.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a7507d819769d01a365ab707794a4084392c824f54a7a6a7862f8c3d0892b283" +dependencies = [ + "cfg-if", + "cpufeatures", + "digest", +] + [[package]] name = "shell-words" version = "1.1.1" @@ -1752,6 +1812,12 @@ version = "0.2.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e421abadd41a4225275504ea4d6566923418b7f05506fbc9c0fe86ba7396114b" +[[package]] +name = "typenum" +version = "1.20.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6f5e870be6c3b371b77fe0ee0bafb859fa4964b4404c27de1d380043c4dda20" + [[package]] name = "unicode-ident" version = "1.0.23" @@ -1838,6 +1904,12 @@ dependencies = [ "rustversion", ] +[[package]] +name = "version_check" +version = "0.9.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0b928f33d975fc6ad9f86c8f283853ad26bdd5b10b7f1542aa2fa15e2289105a" + [[package]] name = "walkdir" version = "2.5.0" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index 5d3bf8c..a8d34da 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -25,6 +25,7 @@ reqwest = { version = "0.13", default-features = false, features = [ ] } semver = { version = "1.0.18", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } +sha2 = "0.10" tempfile = "3.13.0" tokio = { version = "1.36.0", features = [ "macros", diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index 0a4c02b..e8e32eb 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -74,6 +74,17 @@ pub enum DownloadError { /// where the semaphore was dropped or closed while tasks were still waiting /// to acquire permits. SemaphoreClosed { artifact_name: String }, + + /// Downloaded artifact's SHA-256 did not match the digest advertised by + /// the release metadata. + /// + /// The bytes were corrupted or truncated in transit, or the release + /// metadata and asset are inconsistent. + ChecksumMismatch { + artifact_name: String, + expected: String, + actual: String, + }, } impl std::fmt::Display for DownloadError { @@ -117,6 +128,21 @@ impl std::fmt::Display for DownloadError { writeln!(f, "Internal error: concurrency semaphore closed")?; write!(f, " Artifact: {}", artifact_name)?; } + Self::ChecksumMismatch { + artifact_name, + expected, + actual, + } => { + writeln!(f, "Downloaded artifact failed integrity check")?; + writeln!(f, " Artifact: {}", artifact_name)?; + writeln!(f, " Expected SHA-256: {}", expected)?; + writeln!(f, " Actual SHA-256: {}", actual)?; + writeln!(f)?; + write!( + f, + " The download was corrupted or truncated. Please try again." + )?; + } } Ok(()) } @@ -127,7 +153,9 @@ impl std::error::Error for DownloadError { match self { Self::TaskFailed { source, .. } => Some(source.as_ref()), Self::StagingWrite { source, .. } => Some(source), - Self::EmptyArtifact { .. } | Self::SemaphoreClosed { .. } => None, + Self::EmptyArtifact { .. } + | Self::SemaphoreClosed { .. } + | Self::ChecksumMismatch { .. } => None, } } } @@ -139,8 +167,9 @@ impl std::error::Error for DownloadError { /// Manages bounded-concurrent downloads of release artifacts. /// /// Downloads proceed in parallel up to `max_concurrent` tasks. Each task -/// downloads an artifact, verifies it (currently: non-empty check), and -/// writes it to a staging directory. Only after all tasks succeed does +/// downloads an artifact, verifies it (non-empty, plus a SHA-256 digest check +/// when the release advertises one), and writes it to a staging directory. +/// Only after all tasks succeed does /// the staging directory get atomically renamed to the final version /// directory. /// @@ -230,7 +259,7 @@ impl DownloadManager { reporter.component_started(&task.artifact_name); let data = download_with_retry(&github, &asset).await?; - verify_artifact(&task.artifact_name, &data)?; + verify_artifact(&task.artifact_name, &data, asset.digest.as_deref())?; write_to_staging(&staging_path, &task.dest_filename, &data)?; Ok(task.artifact_name) @@ -366,17 +395,48 @@ async fn download_with_retry( } } -/// Verify a downloaded artifact. Currently checks non-empty. -/// Per-artifact checksum/attestation verification will be added in a follow-up PR. -fn verify_artifact(artifact_name: &str, data: &[u8]) -> std::result::Result<(), DownloadError> { +/// Verify a downloaded artifact: it must be non-empty, and — when the release +/// advertises a digest — its SHA-256 must match. +/// +/// `expected_digest` is the release-metadata digest (e.g. `"sha256:"`), or +/// `None` when the release does not provide one, in which case only the +/// non-empty check applies. +fn verify_artifact( + artifact_name: &str, + data: &[u8], + expected_digest: Option<&str>, +) -> std::result::Result<(), DownloadError> { if data.is_empty() { return Err(DownloadError::EmptyArtifact { artifact_name: artifact_name.to_string(), }); } + + if let Some(expected) = expected_digest { + // GitHub advertises digests as "sha256:"; tolerate a bare hex too. + let expected_hex = expected.strip_prefix("sha256:").unwrap_or(expected); + let actual_hex = sha256_hex(data); + if !actual_hex.eq_ignore_ascii_case(expected_hex) { + return Err(DownloadError::ChecksumMismatch { + artifact_name: artifact_name.to_string(), + expected: expected_hex.to_ascii_lowercase(), + actual: actual_hex, + }); + } + } + Ok(()) } +/// Lowercase hex SHA-256 of `data`. +fn sha256_hex(data: &[u8]) -> String { + use sha2::{Digest, Sha256}; + Sha256::digest(data) + .iter() + .map(|byte| format!("{byte:02x}")) + .collect() +} + /// Write artifact data to the staging directory. fn write_to_staging( staging_path: &Path, @@ -397,7 +457,8 @@ fn download_error_artifact_name(err: &DownloadError) -> &str { DownloadError::TaskFailed { artifact_name, .. } | DownloadError::EmptyArtifact { artifact_name } | DownloadError::StagingWrite { artifact_name, .. } - | DownloadError::SemaphoreClosed { artifact_name } => artifact_name, + | DownloadError::SemaphoreClosed { artifact_name } + | DownloadError::ChecksumMismatch { artifact_name, .. } => artifact_name, } } @@ -437,7 +498,7 @@ mod tests { let data: Vec = vec![]; //* When - let result = verify_artifact("ampd-linux-x86_64", &data); + let result = verify_artifact("ampd-linux-x86_64", &data, None); //* Then let err = result.expect_err("should return DownloadError for empty data"); @@ -447,6 +508,49 @@ mod tests { err ); } + + /// SHA-256 of "abc": a known vector used to exercise digest matching. + const ABC_SHA256: &str = "ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad"; + + /// A matching digest passes, and the `sha256:` prefix is accepted. + #[test] + fn verify_artifact_accepts_matching_digest() { + let data = b"abc"; + verify_artifact("driver", data, Some(&format!("sha256:{ABC_SHA256}"))) + .expect("matching digest should verify"); + // Bare hex (no prefix) is tolerated too. + verify_artifact("driver", data, Some(ABC_SHA256)) + .expect("bare-hex matching digest should verify"); + // Digest comparison is case-insensitive. + verify_artifact("driver", data, Some(&ABC_SHA256.to_uppercase())) + .expect("uppercase digest should verify"); + } + + /// A wrong digest is rejected with `ChecksumMismatch`. + #[test] + fn verify_artifact_rejects_mismatched_digest() { + let data = b"abc"; + let wrong = "sha256:0000000000000000000000000000000000000000000000000000000000000000"; + + let err = verify_artifact("driver", data, Some(wrong)) + .expect_err("mismatched digest should fail"); + + assert!( + matches!(err, DownloadError::ChecksumMismatch { .. }), + "expected ChecksumMismatch, got: {:?}", + err + ); + } + + /// With no advertised digest, only the non-empty check applies. + #[test] + fn verify_artifact_without_digest_only_checks_non_empty() { + verify_artifact("driver", b"abc", None).expect("non-empty data should verify"); + + let err = verify_artifact("driver", b"", None) + .expect_err("empty data should still fail without a digest"); + assert!(matches!(err, DownloadError::EmptyArtifact { .. })); + } } #[cfg(unix)] @@ -720,18 +824,26 @@ mod tests { }) } - /// Build release JSON for the mock server with the given assets. - fn release_json(addr: std::net::SocketAddr, asset_names: &[&str]) -> Vec { - let assets: Vec = asset_names + /// Build release JSON where each asset may advertise a `digest`. + fn release_json_with_digests( + addr: std::net::SocketAddr, + assets: &[(&str, Option<&str>)], + ) -> Vec { + let assets: Vec = assets .iter() .enumerate() - .map(|(i, name)| { + .map(|(i, (name, digest))| { + let digest_field = match digest { + Some(d) => format!(r#","digest":"{d}""#), + None => String::new(), + }; format!( - r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"}}"#, + r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"{}}}"#, i + 1, name, addr, name, + digest_field, ) }) .collect(); @@ -755,13 +867,25 @@ mod tests { release_assets: &[&str], download_routes: Vec, max_concurrent: usize, + ) -> Self { + let assets: Vec<(&str, Option<&str>)> = + release_assets.iter().map(|name| (*name, None)).collect(); + Self::new_with_digests(&assets, download_routes, max_concurrent).await + } + + /// Like [`new`](Self::new), but each release asset may advertise a + /// `digest` in its metadata. + async fn new_with_digests( + release_assets: &[(&str, Option<&str>)], + download_routes: Vec, + max_concurrent: usize, ) -> Self { let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await .expect("should bind to a random port"); let addr = listener.local_addr().expect("should have a local address"); - let release_body = release_json(addr, release_assets); + let release_body = release_json_with_digests(addr, release_assets); let mut routes = vec![Route::ok("tags/v1.0.0", release_body)]; routes.extend(download_routes); @@ -852,6 +976,76 @@ mod tests { ); } + /// A correct release digest verifies through the full download pipeline. + #[tokio::test] + async fn download_all_verifies_matching_digest() { + //* Given + let ampd_data = b"fake-ampd-binary".to_vec(); + let digest = format!("sha256:{}", super::super::sha256_hex(&d_data)); + + let fixture = TestFixture::new_with_digests( + &[("ampd-linux-x86_64", Some(digest.as_str()))], + vec![Route::ok("download/ampd-linux-x86_64", ampd_data.clone())], + 4, + ) + .await; + + //* When + let result = fixture + .download(vec![DownloadTask { + artifact_name: "ampd-linux-x86_64".to_string(), + dest_filename: "ampd".to_string(), + optional: false, + }]) + .await; + + //* Then + assert!( + result.is_ok(), + "a valid digest should verify: {:?}", + result.err() + ); + assert_eq!( + fs::read(fixture.version_dir.join("ampd")).expect("should read ampd"), + ampd_data, + ); + } + + /// A wrong release digest fails the batch and leaves no partial install. + #[tokio::test] + async fn download_all_rejects_mismatched_digest() { + //* Given + let ampd_data = b"fake-ampd-binary".to_vec(); + let wrong = "sha256:0000000000000000000000000000000000000000000000000000000000000000"; + + let fixture = TestFixture::new_with_digests( + &[("ampd-linux-x86_64", Some(wrong))], + vec![Route::ok("download/ampd-linux-x86_64", ampd_data)], + 4, + ) + .await; + + //* When + let result = fixture + .download(vec![DownloadTask { + artifact_name: "ampd-linux-x86_64".to_string(), + dest_filename: "ampd".to_string(), + optional: false, + }]) + .await; + + //* Then + let err = result.expect_err("a mismatched digest should fail the download"); + assert!( + err.to_string().contains("integrity check"), + "expected an integrity-check error, got: {err}" + ); + assert!( + !fixture.version_dir.exists(), + "no version directory should be created on integrity failure" + ); + } + /// A missing asset fails the whole batch and leaves no partial install. #[tokio::test] async fn download_all_with_missing_asset_fails_without_partial_install() { diff --git a/ampup/src/github.rs b/ampup/src/github.rs index b31859c..5516f46 100644 --- a/ampup/src/github.rs +++ b/ampup/src/github.rs @@ -184,6 +184,9 @@ pub struct ResolvedAsset { pub name: String, /// Direct browser download URL (used for public repos). pub url: String, + /// Expected content digest from release metadata (e.g. "sha256:"), + /// or `None` when the release does not advertise one. + pub digest: Option, } /// The assets of a single fetched release. @@ -211,6 +214,7 @@ impl ReleaseAssets { id: asset.id, name: asset.name.clone(), url: asset.url.clone(), + digest: asset.digest.clone(), })), // Optional artifacts may be missing from a release; skip them. None if optional => Ok(None), @@ -238,6 +242,8 @@ struct Asset { name: String, #[serde(rename = "browser_download_url")] url: String, + #[serde(default)] + digest: Option, } /// Cloneable so `DownloadManager` can move a handle into each spawned task. From cd7de499c54eeab15443a8ffee94ce6be71fb4fa Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 13:37:39 +0530 Subject: [PATCH 04/18] feat(adbc): fetch, verify, and extract driver archive Wire `ampup adbc install ` to resolve the active amp version, download the pinned driver archive, verify its SHA-256 digest, and extract the validated members to a staging dir. Adds a flat-tar.gz extractor (traversal-guarded) and the driver runtime-lib naming. Placement into the version dir is not yet wired. Part of edgeandnode/amp#2600. --- Cargo.lock | 74 +++++++++++++++++ ampup/Cargo.toml | 2 + ampup/src/adbc.rs | 22 ++++++ ampup/src/archive.rs | 144 ++++++++++++++++++++++++++++++++++ ampup/src/commands/adbc.rs | 96 ++++++++++++++++++++++- ampup/src/download_manager.rs | 2 +- ampup/src/lib.rs | 1 + ampup/src/main.rs | 45 ++++++++++- 8 files changed, 379 insertions(+), 7 deletions(-) create mode 100644 ampup/src/archive.rs diff --git a/Cargo.lock b/Cargo.lock index 542b263..5760403 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2,6 +2,12 @@ # It is not intended for manual editing. version = 4 +[[package]] +name = "adler2" +version = "2.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "320119579fcad9c21884f5c4861d16174d0e06250625266f50fe6898340abefa" + [[package]] name = "ampup" version = "0.1.0" @@ -10,12 +16,14 @@ dependencies = [ "clap", "console", "dialoguer", + "flate2", "fs-err", "futures", "reqwest", "semver", "serde", "sha2", + "tar", "tempfile", "tokio", "vergen-gitcl", @@ -277,6 +285,15 @@ dependencies = [ "libc", ] +[[package]] +name = "crc32fast" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9481c1c90cbf2ac953f07c8d4a58aa3945c425b7185c9154d67a65e4230da511" +dependencies = [ + "cfg-if", +] + [[package]] name = "crypto-common" version = "0.1.7" @@ -429,12 +446,32 @@ version = "2.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "37909eebbb50d72f9059c3b6d82c0463f2ff062c9e95845c43a6c9c0355411be" +[[package]] +name = "filetime" +version = "0.2.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5c287a33c7f0a620c38e641e7f60827713987b3c0f26e8ddc9462cc69cf75759" +dependencies = [ + "cfg-if", + "libc", +] + [[package]] name = "find-msvc-tools" version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "flate2" +version = "1.1.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "843fba2746e448b37e26a819579957415c8cef339bf08564fe8b7ddbd959573c" +dependencies = [ + "crc32fast", + "miniz_oxide", +] + [[package]] name = "fnv" version = "1.0.7" @@ -977,6 +1014,16 @@ version = "2.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f8ca58f447f06ed17d5fc4043ce1b10dd205e060fb3ce5b979b8ed8e59ff3f79" +[[package]] +name = "miniz_oxide" +version = "0.8.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "1fa76a2c86f704bdb222d66965fb3d63269ce38518b83cb0575fca855ebb6316" +dependencies = [ + "adler2", + "simd-adler32", +] + [[package]] name = "mio" version = "1.1.1" @@ -1510,6 +1557,12 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" +[[package]] +name = "simd-adler32" +version = "0.3.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3a219298ac11a56ea9a6d2120044824d6f01aeb034955e7af7bc16858527deea" + [[package]] name = "slab" version = "0.4.12" @@ -1581,6 +1634,17 @@ dependencies = [ "syn", ] +[[package]] +name = "tar" +version = "0.4.46" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f6221d9a6003c78398e3b239969f352578258df48c8eb051caadae0015bc840" +dependencies = [ + "filetime", + "libc", + "xattr", +] + [[package]] name = "tempfile" version = "3.25.0" @@ -2419,6 +2483,16 @@ version = "0.6.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "9edde0db4769d2dc68579893f2306b26c6ecfbe0ef499b013d731b7b9247e0b9" +[[package]] +name = "xattr" +version = "1.6.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "32e45ad4206f6d2479085147f02bc2ef834ac85886624a23575ae137c8aa8156" +dependencies = [ + "libc", + "rustix", +] + [[package]] name = "yoke" version = "0.8.1" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index a8d34da..cbef488 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -15,6 +15,7 @@ anyhow = "1.0.80" clap = { version = "4.5.2", features = ["derive", "env"] } console = "0.16" dialoguer = "0.12" +flate2 = "1.0" fs-err = "3.0.0" futures = "0.3" reqwest = { version = "0.13", default-features = false, features = [ @@ -26,6 +27,7 @@ reqwest = { version = "0.13", default-features = false, features = [ semver = { version = "1.0.18", features = ["serde"] } serde = { version = "1.0", features = ["derive"] } sha2 = "0.10" +tar = "0.4" tempfile = "3.13.0" tokio = { version = "1.36.0", features = [ "macros", diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index cf1344a..ee98b91 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -27,6 +27,16 @@ impl Driver { pub fn from_name(name: &str) -> Option { Self::ALL.iter().copied().find(|d| d.as_str() == name) } + + /// The runtime library filename this driver's archive carries on + /// `platform` (e.g. `libadbc_driver_postgresql.so` on Linux). + pub fn runtime_lib_filename(&self, platform: Platform) -> String { + let ext = match platform { + Platform::Linux => "so", + Platform::Darwin => "dylib", + }; + format!("libadbc_driver_{}.{ext}", self.as_str()) + } } impl std::fmt::Display for Driver { @@ -70,4 +80,16 @@ mod tests { assert_eq!(Driver::from_name("mysql"), None); assert_eq!(Driver::from_name(""), None); } + + #[test] + fn runtime_lib_filename_is_platform_specific() { + assert_eq!( + Driver::Postgresql.runtime_lib_filename(Platform::Linux), + "libadbc_driver_postgresql.so", + ); + assert_eq!( + Driver::Postgresql.runtime_lib_filename(Platform::Darwin), + "libadbc_driver_postgresql.dylib", + ); + } } diff --git a/ampup/src/archive.rs b/ampup/src/archive.rs new file mode 100644 index 0000000..97afe3b --- /dev/null +++ b/ampup/src/archive.rs @@ -0,0 +1,144 @@ +//! Extraction of ADBC driver release archives (flat gzip-compressed tar). +//! +//! Driver archives contain exactly a driver library plus `LICENSE.txt` and +//! `NOTICE.txt` at the root. Extraction enforces that flat shape and an exact +//! member set, refusing anything that could escape the destination directory. + +use std::{ + collections::BTreeSet, + path::{Component, Path}, +}; + +use anyhow::{Context, Result, bail}; +use flate2::read::GzDecoder; +use tar::Archive; + +/// Extract a gzip-compressed tar archive into `dest`, requiring a flat layout +/// whose entries exactly equal `expected_members`. +/// +/// Every entry must be a single normal path component (no directories, no +/// `..`), so an entry can never be written outside `dest`. Any unexpected or +/// missing member is an error. +pub fn extract_and_validate(data: &[u8], dest: &Path, expected_members: &[&str]) -> Result<()> { + let mut archive = Archive::new(GzDecoder::new(data)); + let mut seen = BTreeSet::new(); + + for entry in archive + .entries() + .context("failed to read archive entries")? + { + let mut entry = entry.context("failed to read an archive entry")?; + let path = entry.path().context("archive entry has an invalid path")?; + + // A flat file name is exactly one normal component. This rejects + // nested paths, absolute paths, and `..`, so `dest.join(name)` stays + // inside `dest`. + let mut components = path.components(); + let name = match (components.next(), components.next()) { + (Some(Component::Normal(name)), None) => name + .to_str() + .context("archive entry name is not valid UTF-8")? + .to_owned(), + _ => bail!("archive entry {:?} is not a flat file name", path), + }; + + if !expected_members.contains(&name.as_str()) { + bail!("unexpected member in driver archive: {name}"); + } + + entry + .unpack(dest.join(&name)) + .with_context(|| format!("failed to extract {name}"))?; + seen.insert(name); + } + + let expected: BTreeSet = expected_members.iter().map(|m| m.to_string()).collect(); + if seen != expected { + let missing: Vec = expected.difference(&seen).cloned().collect(); + bail!( + "driver archive is missing expected members: {}", + missing.join(", ") + ); + } + + Ok(()) +} + +#[cfg(test)] +mod tests { + use flate2::{Compression, write::GzEncoder}; + use tar::{Builder, Header}; + + use super::*; + + /// Build a gzip-compressed tar archive from `(name, contents)` pairs. + fn make_targz(files: &[(&str, &[u8])]) -> Vec { + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + for (name, data) in files { + let mut header = Header::new_gnu(); + header.set_size(data.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, name, *data) + .expect("should append tar entry"); + } + builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip") + } + + const MEMBERS: &[&str] = &["libadbc_driver_postgresql.so", "LICENSE.txt", "NOTICE.txt"]; + + #[test] + fn extracts_exact_members() { + let archive = make_targz(&[ + ("libadbc_driver_postgresql.so", b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let dir = tempfile::tempdir().expect("tempdir"); + + extract_and_validate(&archive, dir.path(), MEMBERS).expect("should extract"); + + assert_eq!( + std::fs::read(dir.path().join("libadbc_driver_postgresql.so")).expect("lib"), + b"ELF", + ); + } + + #[test] + fn rejects_unexpected_member() { + let archive = make_targz(&[("evil.sh", b"rm -rf /")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), MEMBERS) + .expect_err("unexpected member should fail"); + assert!(err.to_string().contains("unexpected member"), "got: {err}"); + } + + #[test] + fn rejects_missing_member() { + let archive = make_targz(&[("LICENSE.txt", b"license")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), &["LICENSE.txt", "NOTICE.txt"]) + .expect_err("missing member should fail"); + assert!( + err.to_string().contains("missing expected members"), + "got: {err}" + ); + } + + #[test] + fn rejects_non_flat_entry() { + let archive = make_targz(&[("nested/lib.so", b"x")]); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), &["lib.so"]) + .expect_err("nested entry should fail"); + assert!(err.to_string().contains("flat file name"), "got: {err}"); + } +} diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 858b9ba..0201f03 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,14 +3,78 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use anyhow::{Result, anyhow, bail}; +use std::path::PathBuf; -use crate::adbc::Driver; +use anyhow::{Context, Result, anyhow, bail}; + +use crate::{ + adbc::Driver, + archive, + config::Config, + download_manager::verify_artifact, + github::GitHubClient, + platform::{Architecture, Platform, PlatformError}, + token, ui, + version_manager::VersionManager, +}; /// Install an ADBC driver for the active amp version. -pub async fn install(driver: &str) -> Result<()> { +pub async fn install( + driver: &str, + install_dir: Option, + repo: String, + github_token: Option, + arch: Option, + platform: Option, + _jobs: usize, +) -> Result<()> { let driver = parse_driver(driver)?; - bail!("`ampup adbc install {driver}` is not yet implemented"); + + let config = Config::new(install_dir)?; + let token = token::resolve_github_token(github_token); + let github = GitHubClient::new(repo, token)?; + let version_manager = VersionManager::new(config); + + // Drivers are pinned to an installed amp version, so one must be active. + let version = version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + + let platform = resolve_platform(platform)?; + let arch = resolve_arch(arch)?; + + let asset_name = crate::adbc::asset_name(driver, platform, arch); + ui::info!( + "Installing {} driver for amp {}", + driver, + ui::version(&version) + ); + + let assets = github.fetch_release_assets(&version).await?; + let asset = assets + .resolve(&asset_name, false)? + .expect("a required asset resolves to Some or errors"); + + let data = github.download_resolved_asset(&asset).await?; + verify_artifact(&asset.name, &data, asset.digest.as_deref()) + .context("driver archive failed verification")?; + + // Extract into a staging dir; placement into the version's drivers + // directory is handled separately. + let staging = tempfile::tempdir().context("failed to create staging directory")?; + let lib = driver.runtime_lib_filename(platform); + archive::extract_and_validate( + &data, + staging.path(), + &[lib.as_str(), "LICENSE.txt", "NOTICE.txt"], + )?; + + ui::detail!( + "Fetched, verified, and extracted the {} driver ({} bytes)", + driver, + data.len() + ); + bail!("installing the driver into the amp version is not yet implemented"); } /// List installed ADBC drivers for the active version. @@ -36,3 +100,27 @@ fn parse_driver(name: &str) -> Result { anyhow!("unknown ADBC driver `{name}` (supported: {supported})") }) } + +/// Resolve the target platform from an optional `--platform` override. +fn resolve_platform(over: Option) -> Result { + match over { + Some(p) => match p.as_str() { + "linux" => Ok(Platform::Linux), + "darwin" => Ok(Platform::Darwin), + _ => Err(PlatformError::UnsupportedPlatform { detected: p }.into()), + }, + None => Ok(Platform::detect()?), + } +} + +/// Resolve the target architecture from an optional `--arch` override. +fn resolve_arch(over: Option) -> Result { + match over { + Some(a) => match a.as_str() { + "x86_64" | "amd64" => Ok(Architecture::X86_64), + "aarch64" | "arm64" => Ok(Architecture::Aarch64), + _ => Err(PlatformError::UnsupportedArchitecture { detected: a }.into()), + }, + None => Ok(Architecture::detect()?), + } +} diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index e8e32eb..9940f06 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -401,7 +401,7 @@ async fn download_with_retry( /// `expected_digest` is the release-metadata digest (e.g. `"sha256:"`), or /// `None` when the release does not provide one, in which case only the /// non-empty check applies. -fn verify_artifact( +pub(crate) fn verify_artifact( artifact_name: &str, data: &[u8], expected_digest: Option<&str>, diff --git a/ampup/src/lib.rs b/ampup/src/lib.rs index 262806f..6610a61 100644 --- a/ampup/src/lib.rs +++ b/ampup/src/lib.rs @@ -1,4 +1,5 @@ pub mod adbc; +pub mod archive; pub mod builder; pub mod commands; pub mod config; diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 5d7a1e1..8fc6a1c 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -187,8 +187,32 @@ enum SelfCommands { enum AdbcCommands { /// Install an ADBC driver for the active amp version Install { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + /// Driver to install (e.g. postgresql) driver: String, + + /// GitHub repository in format "owner/repo" + #[arg(long, default_value_t = DEFAULT_REPO.to_string())] + repo: String, + + /// GitHub token for private repository access (defaults to $GITHUB_TOKEN) + #[arg(long, env = "GITHUB_TOKEN", hide_env = true)] + github_token: Option, + + /// Override architecture detection (x86_64, aarch64) + #[arg(long)] + arch: Option, + + /// Override platform detection (linux, darwin) + #[arg(long)] + platform: Option, + + /// Number of concurrent downloads + #[arg(short = 'j', long = "jobs", default_value_t = DEFAULT_DOWNLOAD_JOBS)] + jobs: usize, }, /// List installed ADBC drivers for the active version @@ -291,8 +315,25 @@ async fn run() -> anyhow::Result<()> { } }, Some(Commands::Adbc { command }) => match command { - AdbcCommands::Install { driver } => { - commands::adbc::install(&driver).await?; + AdbcCommands::Install { + install_dir, + driver, + repo, + github_token, + arch, + platform, + jobs, + } => { + commands::adbc::install( + &driver, + install_dir, + repo, + github_token, + arch, + platform, + jobs, + ) + .await?; } AdbcCommands::List => { commands::adbc::list()?; From 6ef6e8d7abd8885bbd79f1a5a45ba94b58467d58 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:43:24 +0000 Subject: [PATCH 05/18] feat(adbc): place driver and write manifest into version dir Install now moves the extracted driver into a self-contained ~/.amp/versions//drivers// and writes an ADBC manifest.toml (typed via the toml crate) whose Driver.shared is the absolute library path, so ampd can load the driver by manifest. Staging happens inside the drivers dir so the final placement is a same-filesystem atomic rename; reinstalls replace the existing dir. Part of edgeandnode/amp#2600. --- Cargo.lock | 55 +++++++++++++++++++ ampup/Cargo.toml | 1 + ampup/src/adbc.rs | 49 +++++++++++++++++ ampup/src/commands/adbc.rs | 107 ++++++++++++++++++++++++++++++++++--- ampup/src/config.rs | 12 +++++ 5 files changed, 216 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5760403..5c45e74 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,6 +26,7 @@ dependencies = [ "tar", "tempfile", "tokio", + "toml", "vergen-gitcl", ] @@ -1522,6 +1523,15 @@ dependencies = [ "zmij", ] +[[package]] +name = "serde_spanned" +version = "1.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" +dependencies = [ + "serde_core", +] + [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -1806,6 +1816,45 @@ dependencies = [ "tokio", ] +[[package]] +name = "toml" +version = "1.1.3+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "53c96ecdfa941c8fc4fcaed14f99ada8ebed502eef533015095a07e3301d4c3c" +dependencies = [ + "indexmap", + "serde_core", + "serde_spanned", + "toml_datetime", + "toml_parser", + "toml_writer", + "winnow", +] + +[[package]] +name = "toml_datetime" +version = "1.1.1+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" +dependencies = [ + "serde_core", +] + +[[package]] +name = "toml_parser" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a2abe9b86193656635d2411dc43050282ca48aa31c2451210f4202550afb7526" +dependencies = [ + "winnow", +] + +[[package]] +name = "toml_writer" +version = "1.1.2+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" + [[package]] name = "tower" version = "0.5.3" @@ -2389,6 +2438,12 @@ version = "0.53.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" +[[package]] +name = "winnow" +version = "1.0.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" + [[package]] name = "wit-bindgen" version = "0.51.0" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index cbef488..7300fd4 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -36,6 +36,7 @@ tokio = { version = "1.36.0", features = [ "sync", "test-util", ] } +toml = "1.1.3" [build-dependencies] vergen-gitcl = { version = "9.0.0", features = ["build"] } diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index ee98b91..b93ed2f 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -4,6 +4,10 @@ //! This module maps a supported driver plus a target platform/arch to the //! release asset that carries it. +use std::path::Path; + +use serde::Serialize; + use crate::platform::{Architecture, Platform}; /// A supported ADBC driver, as shipped in Amp releases. @@ -58,6 +62,36 @@ pub fn asset_name(driver: Driver, platform: Platform, arch: Architecture) -> Str ) } +/// An ADBC driver manifest (`manifest.toml`), the on-disk contract `ampd` +/// reads to load a driver by absolute path. +/// +/// Only `Driver.shared` is required; the postgres driver's entrypoint is +/// derived from its name, so no `entrypoint` key is emitted. +#[derive(Serialize)] +struct Manifest { + manifest_version: u32, + #[serde(rename = "Driver")] + driver: DriverSection, +} + +#[derive(Serialize)] +struct DriverSection { + shared: String, +} + +/// Render the `manifest.toml` contents pointing `ampd` at the driver library +/// at `lib_path` (an absolute path). +pub fn driver_manifest(lib_path: &Path) -> String { + let manifest = Manifest { + manifest_version: 1, + driver: DriverSection { + shared: lib_path.to_string_lossy().into_owned(), + }, + }; + // Serializing a fixed two-field struct cannot fail. + toml::to_string(&manifest).expect("driver manifest serializes") +} + #[cfg(test)] mod tests { use super::*; @@ -92,4 +126,19 @@ mod tests { "libadbc_driver_postgresql.dylib", ); } + + #[test] + fn driver_manifest_round_trips_the_library_path() { + // A path with a quote and a backslash must survive escaping so the + // manifest stays valid TOML that parses back to the same path. + let path = Path::new("/home/a\"b\\c/drivers/postgresql/libadbc_driver_postgresql.so"); + let rendered = driver_manifest(path); + + let parsed: toml::Value = toml::from_str(&rendered).expect("manifest is valid TOML"); + assert_eq!(parsed["manifest_version"].as_integer(), Some(1)); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(path.to_string_lossy().as_ref()), + ); + } } diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 0201f03..ec089f1 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -3,9 +3,10 @@ //! Installs destination-specific ADBC driver libraries from the pinned set //! shipped with each Amp release, so `ampd` can load them at runtime. -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use anyhow::{Context, Result, anyhow, bail}; +use fs_err as fs; use crate::{ adbc::Driver, @@ -59,9 +60,13 @@ pub async fn install( verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; - // Extract into a staging dir; placement into the version's drivers - // directory is handled separately. - let staging = tempfile::tempdir().context("failed to create staging directory")?; + // Stage inside the drivers directory so the final rename stays on one + // filesystem (atomic, no cross-device copy). + let drivers_dir = version_manager.config().drivers_dir(&version); + fs::create_dir_all(&drivers_dir).context("failed to create drivers directory")?; + let staging = + tempfile::tempdir_in(&drivers_dir).context("failed to create staging directory")?; + let lib = driver.runtime_lib_filename(platform); archive::extract_and_validate( &data, @@ -69,12 +74,41 @@ pub async fn install( &[lib.as_str(), "LICENSE.txt", "NOTICE.txt"], )?; - ui::detail!( - "Fetched, verified, and extracted the {} driver ({} bytes)", + // Absolute so the manifest's `Driver.shared` resolves regardless of + // ampd's working directory. + let driver_dir = std::path::absolute( + version_manager + .config() + .driver_dir(&version, driver.as_str()), + ) + .context("failed to resolve driver directory")?; + place_driver(staging.path(), &driver_dir, &lib)?; + // The staged files now live at `driver_dir`; disarm TempDir cleanup. + let _ = staging.keep(); + + ui::info!( + "Installed {} driver for amp {} at {}", driver, - data.len() + ui::version(&version), + driver_dir.display(), ); - bail!("installing the driver into the amp version is not yet implemented"); + Ok(()) +} + +/// Assemble the self-contained driver directory: write the manifest into the +/// staged files (pointing `Driver.shared` at the final library path), then +/// atomically move the staged directory into `driver_dir`, replacing any +/// existing install. +fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> { + let manifest = crate::adbc::driver_manifest(&driver_dir.join(lib_name)); + fs::write(staging.join("manifest.toml"), manifest) + .context("failed to write ADBC driver manifest")?; + + if driver_dir.exists() { + fs::remove_dir_all(driver_dir).context("failed to remove the existing driver directory")?; + } + fs::rename(staging, driver_dir).context("failed to move the driver into place")?; + Ok(()) } /// List installed ADBC drivers for the active version. @@ -124,3 +158,60 @@ fn resolve_arch(over: Option) -> Result { None => Ok(Architecture::detect()?), } } + +#[cfg(test)] +mod tests { + use super::*; + + const LIB: &str = "libadbc_driver_postgresql.so"; + + /// Build a staging directory holding the three extracted driver files, + /// with `lib_contents` as the library body. + fn staged_driver(parent: &Path, lib_contents: &[u8]) -> PathBuf { + let staging = tempfile::tempdir_in(parent).expect("staging dir"); + fs::write(staging.path().join(LIB), lib_contents).expect("write lib"); + fs::write(staging.path().join("LICENSE.txt"), b"license").expect("write license"); + fs::write(staging.path().join("NOTICE.txt"), b"notice").expect("write notice"); + staging.keep() + } + + #[test] + fn place_driver_writes_files_and_manifest() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + fs::create_dir_all(&drivers_dir).expect("drivers dir"); + let staging = staged_driver(&drivers_dir, b"ELF"); + let driver_dir = drivers_dir.join("postgresql"); + + place_driver(&staging, &driver_dir, LIB).expect("place"); + + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); + assert!(driver_dir.join("LICENSE.txt").exists()); + assert!(driver_dir.join("NOTICE.txt").exists()); + + let manifest = + fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); + let parsed: toml::Value = toml::from_str(&manifest).expect("valid TOML"); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(driver_dir.join(LIB).to_string_lossy().as_ref()), + ); + } + + #[test] + fn place_driver_replaces_an_existing_install() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + fs::create_dir_all(&drivers_dir).expect("drivers dir"); + let driver_dir = drivers_dir.join("postgresql"); + + place_driver(&staged_driver(&drivers_dir, b"old"), &driver_dir, LIB).expect("first"); + // A stray file from the old install must not survive the reinstall. + fs::write(driver_dir.join("stale.txt"), b"x").expect("stray file"); + + place_driver(&staged_driver(&drivers_dir, b"new"), &driver_dir, LIB).expect("second"); + + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"new"); + assert!(!driver_dir.join("stale.txt").exists()); + } +} diff --git a/ampup/src/config.rs b/ampup/src/config.rs index 3b0be7d..8421f29 100644 --- a/ampup/src/config.rs +++ b/ampup/src/config.rs @@ -98,6 +98,18 @@ impl Config { self.versions_dir.join(version).join("ampsql") } + /// Get the ADBC drivers directory for a specific version + /// (~/.amp/versions//drivers) + pub fn drivers_dir(&self, version: &str) -> PathBuf { + self.versions_dir.join(version).join("drivers") + } + + /// Get the self-contained directory for a single ADBC driver under a + /// version (~/.amp/versions//drivers/) + pub fn driver_dir(&self, version: &str, driver: &str) -> PathBuf { + self.drivers_dir(version).join(driver) + } + /// Get the active ampsql binary symlink path pub fn active_ampsql_path(&self) -> PathBuf { self.bin_dir.join("ampsql") From 1f3fd430c1a4a4436df6a7820b7508d2cf3efa5c Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:58:07 +0000 Subject: [PATCH 06/18] fix(adbc): reject non-regular archive entries extract_and_validate checked only the entry name, so a symlink or hardlink named as an allowed member would be materialized by unpack as a link instead of the archived bytes. Reject any entry that is not a regular file before unpacking. Part of edgeandnode/amp#2600. --- ampup/src/archive.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/ampup/src/archive.rs b/ampup/src/archive.rs index 97afe3b..f098108 100644 --- a/ampup/src/archive.rs +++ b/ampup/src/archive.rs @@ -46,6 +46,13 @@ pub fn extract_and_validate(data: &[u8], dest: &Path, expected_members: &[&str]) bail!("unexpected member in driver archive: {name}"); } + // Only regular files are expected. A symlink/hardlink entry (even under + // an allowed name) would be materialized by `unpack` as a link rather + // than the archived bytes, so reject anything that isn't a plain file. + if !entry.header().entry_type().is_file() { + bail!("archive member {name} is not a regular file"); + } + entry .unpack(dest.join(&name)) .with_context(|| format!("failed to extract {name}"))?; @@ -141,4 +148,27 @@ mod tests { .expect_err("nested entry should fail"); assert!(err.to_string().contains("flat file name"), "got: {err}"); } + + #[test] + fn rejects_symlink_entry() { + // A symlink named as an allowed member, pointing outside the archive. + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + let mut header = Header::new_gnu(); + header.set_entry_type(tar::EntryType::Symlink); + header.set_size(0); + header.set_mode(0o644); + builder + .append_link(&mut header, "libadbc_driver_postgresql.so", "/etc/passwd") + .expect("should append symlink entry"); + let archive = builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip"); + let dir = tempfile::tempdir().expect("tempdir"); + + let err = extract_and_validate(&archive, dir.path(), MEMBERS) + .expect_err("symlink member should fail"); + assert!(err.to_string().contains("not a regular file"), "got: {err}"); + } } From e9948adc78bc1d990351ef866dedc181edb9c06e Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 10:58:07 +0000 Subject: [PATCH 07/18] refactor(adbc): drop unused --jobs flag from adbc install adbc install downloads a single asset directly, so the -j/--jobs flag was accepted and silently ignored. Remove it from the subcommand and the install signature. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 1 - ampup/src/main.rs | 17 ++--------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index ec089f1..bfca157 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -27,7 +27,6 @@ pub async fn install( github_token: Option, arch: Option, platform: Option, - _jobs: usize, ) -> Result<()> { let driver = parse_driver(driver)?; diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 8fc6a1c..5baad91 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -209,10 +209,6 @@ enum AdbcCommands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, - - /// Number of concurrent downloads - #[arg(short = 'j', long = "jobs", default_value_t = DEFAULT_DOWNLOAD_JOBS)] - jobs: usize, }, /// List installed ADBC drivers for the active version @@ -322,18 +318,9 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, - jobs, } => { - commands::adbc::install( - &driver, - install_dir, - repo, - github_token, - arch, - platform, - jobs, - ) - .await?; + commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) + .await?; } AdbcCommands::List => { commands::adbc::list()?; From c7a6da050f7e3bccd50a1c1c0f179f9204964bdc Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 11:11:02 +0000 Subject: [PATCH 08/18] feat(adbc): add adbc list and uninstall commands list shows the complete drivers installed for the active amp version (directories that match the catalog and hold a manifest.toml, so leftover staging dirs and partial installs are skipped). uninstall removes a driver's directory and prunes an emptied drivers dir. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 134 +++++++++++++++++++++++++++++++++++-- ampup/src/main.rs | 21 ++++-- 2 files changed, 144 insertions(+), 11 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index bfca157..389b879 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -110,15 +110,96 @@ fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> Ok(()) } -/// List installed ADBC drivers for the active version. -pub fn list() -> Result<()> { - bail!("`ampup adbc list` is not yet implemented"); +/// List installed ADBC drivers for the active amp version. +pub fn list(install_dir: Option) -> Result<()> { + let config = Config::new(install_dir)?; + let version_manager = VersionManager::new(config); + + let Some(version) = version_manager.get_current()? else { + ui::info!("No active amp version"); + return Ok(()); + }; + + let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; + if drivers.is_empty() { + ui::info!( + "No ADBC drivers installed for amp {}", + ui::version(&version) + ); + return Ok(()); + } + + ui::info!("Installed ADBC drivers for amp {}:", ui::version(&version)); + for driver in drivers { + println!(" {driver}"); + } + Ok(()) } -/// Uninstall an ADBC driver. -pub fn uninstall(driver: &str) -> Result<()> { +/// Uninstall an ADBC driver from the active amp version. +pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { let driver = parse_driver(driver)?; - bail!("`ampup adbc uninstall {driver}` is not yet implemented"); + + let config = Config::new(install_dir)?; + let version_manager = VersionManager::new(config); + + let version = version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + + let driver_dir = version_manager + .config() + .driver_dir(&version, driver.as_str()); + if !driver_dir.exists() { + bail!( + "the {driver} driver is not installed for amp {}", + ui::version(&version) + ); + } + + fs::remove_dir_all(&driver_dir).context("failed to remove the driver directory")?; + // Tidy up an otherwise-empty drivers directory; ignore the error when it + // still holds other drivers (or a leftover staging dir). + let _ = fs::remove_dir(version_manager.config().drivers_dir(&version)); + + ui::info!( + "Uninstalled {} driver from amp {}", + driver, + ui::version(&version) + ); + Ok(()) +} + +/// The catalog drivers currently installed under `drivers_dir`. +/// +/// Only complete installs count: an entry must be a directory named after a +/// known driver and hold a `manifest.toml`, which skips leftover staging +/// directories and partial installs. Returns empty when `drivers_dir` is +/// absent (a version with no drivers installed). +fn installed_drivers(drivers_dir: &Path) -> Result> { + if !drivers_dir.exists() { + return Ok(Vec::new()); + } + + let mut drivers = Vec::new(); + for entry in fs::read_dir(drivers_dir).context("failed to read the drivers directory")? { + let entry = entry.context("failed to read a drivers directory entry")?; + if !entry + .file_type() + .context("failed to determine a drivers entry type")? + .is_dir() + { + continue; + } + let Some(driver) = entry.file_name().to_str().and_then(Driver::from_name) else { + continue; + }; + if entry.path().join("manifest.toml").is_file() { + drivers.push(driver); + } + } + drivers.sort_by_key(|d| d.as_str()); + Ok(drivers) } /// Resolve a driver name against the catalog, with a helpful error listing the @@ -213,4 +294,45 @@ mod tests { assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"new"); assert!(!driver_dir.join("stale.txt").exists()); } + + /// Create `drivers_dir//`, optionally with a `manifest.toml`. + fn make_driver_dir(drivers_dir: &Path, name: &str, with_manifest: bool) { + let dir = drivers_dir.join(name); + fs::create_dir_all(&dir).expect("driver dir"); + if with_manifest { + fs::write(dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + } + } + + #[test] + fn installed_drivers_lists_only_complete_catalog_dirs() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + make_driver_dir(&drivers_dir, "postgresql", true); // complete -> included + make_driver_dir(&drivers_dir, "mysql", true); // not in catalog -> excluded + make_driver_dir(&drivers_dir, ".tmpABC123", true); // leftover staging -> excluded + fs::write(drivers_dir.join("stray.txt"), b"x").expect("stray file"); // non-dir -> excluded + + assert_eq!( + installed_drivers(&drivers_dir).expect("list"), + vec![Driver::Postgresql], + ); + } + + #[test] + fn installed_drivers_skips_partial_installs() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); + make_driver_dir(&drivers_dir, "postgresql", false); // no manifest -> excluded + + assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + } + + #[test] + fn installed_drivers_on_missing_dir_is_empty() { + let root = tempfile::tempdir().expect("root"); + let drivers_dir = root.path().join("drivers"); // never created + + assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + } } diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 5baad91..6fbd8c5 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -212,10 +212,18 @@ enum AdbcCommands { }, /// List installed ADBC drivers for the active version - List, + List { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + }, /// Uninstall an ADBC driver Uninstall { + /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) + #[arg(long, env = "AMP_DIR")] + install_dir: Option, + /// Driver to uninstall (e.g. postgresql) driver: String, }, @@ -322,11 +330,14 @@ async fn run() -> anyhow::Result<()> { commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) .await?; } - AdbcCommands::List => { - commands::adbc::list()?; + AdbcCommands::List { install_dir } => { + commands::adbc::list(install_dir)?; } - AdbcCommands::Uninstall { driver } => { - commands::adbc::uninstall(&driver)?; + AdbcCommands::Uninstall { + install_dir, + driver, + } => { + commands::adbc::uninstall(install_dir, &driver)?; } }, None => { From 8fea67a399c1b7291cdf2914d68611790274bbb6 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 11:30:50 +0000 Subject: [PATCH 09/18] fix(adbc): retry driver download on transient failure The driver download called download_resolved_asset directly, so it lacked the single application-level retry that binary downloads get via download_with_retry. Reuse that wrapper (now pub(crate)) so a driver fetch survives a transient failure the same way binaries do. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 4 ++-- ampup/src/download_manager.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 389b879..c70c047 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -12,7 +12,7 @@ use crate::{ adbc::Driver, archive, config::Config, - download_manager::verify_artifact, + download_manager::{download_with_retry, verify_artifact}, github::GitHubClient, platform::{Architecture, Platform, PlatformError}, token, ui, @@ -55,7 +55,7 @@ pub async fn install( .resolve(&asset_name, false)? .expect("a required asset resolves to Some or errors"); - let data = github.download_resolved_asset(&asset).await?; + let data = download_with_retry(&github, &asset).await?; verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; diff --git a/ampup/src/download_manager.rs b/ampup/src/download_manager.rs index 9940f06..2382d44 100644 --- a/ampup/src/download_manager.rs +++ b/ampup/src/download_manager.rs @@ -374,7 +374,7 @@ fn append_extension(path: &Path, ext: &str) -> PathBuf { /// already handled at the HTTP layer by `GitHubClient::send_with_rate_limit`, /// so a rate-limited request will have been retried there before surfacing /// as an error here. -async fn download_with_retry( +pub(crate) async fn download_with_retry( github: &GitHubClient, asset: &ResolvedAsset, ) -> std::result::Result, DownloadError> { From 318300d58c143885ed4652853d5c78fcc2b85509 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Wed, 22 Jul 2026 14:51:59 +0000 Subject: [PATCH 10/18] test(adbc): cover install and uninstall end-to-end Split install into a thin wrapper plus install_driver so a test can inject a GitHubClient pointed at a mock server. Add an in-process mock GitHub server and integration tests: install drives the full fetch -> verify -> extract -> place -> manifest path against the mock, and uninstall removes a placed driver and prunes the emptied drivers dir. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 19 ++++-- ampup/src/tests/it_adbc.rs | 120 +++++++++++++++++++++++++++++++++ ampup/src/tests/mock_github.rs | 107 +++++++++++++++++++++++++++++ ampup/src/tests/mod.rs | 2 + 4 files changed, 244 insertions(+), 4 deletions(-) create mode 100644 ampup/src/tests/it_adbc.rs create mode 100644 ampup/src/tests/mock_github.rs diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index c70c047..93f57e6 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -29,10 +29,21 @@ pub async fn install( platform: Option, ) -> Result<()> { let driver = parse_driver(driver)?; - let config = Config::new(install_dir)?; - let token = token::resolve_github_token(github_token); - let github = GitHubClient::new(repo, token)?; + let github = GitHubClient::new(repo, token::resolve_github_token(github_token))?; + install_driver(&github, config, driver, arch, platform).await +} + +/// Fetch, verify, extract, and place `driver` for the active amp version using +/// an already-constructed GitHub client. Split from [`install`] so tests can +/// inject a client pointed at a mock server. +pub(crate) async fn install_driver( + github: &GitHubClient, + config: Config, + driver: Driver, + arch: Option, + platform: Option, +) -> Result<()> { let version_manager = VersionManager::new(config); // Drivers are pinned to an installed amp version, so one must be active. @@ -55,7 +66,7 @@ pub async fn install( .resolve(&asset_name, false)? .expect("a required asset resolves to Some or errors"); - let data = download_with_retry(&github, &asset).await?; + let data = download_with_retry(github, &asset).await?; verify_artifact(&asset.name, &data, asset.digest.as_deref()) .context("driver archive failed verification")?; diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs new file mode 100644 index 0000000..378f9a3 --- /dev/null +++ b/ampup/src/tests/it_adbc.rs @@ -0,0 +1,120 @@ +//! Integration tests for the `ampup adbc` commands (#2600). +//! +//! `adbc_install_places_driver_and_manifest` drives the whole install path +//! (fetch release metadata -> download -> verify digest -> extract -> place + +//! manifest) against an in-process mock GitHub server, so no network is +//! required. The uninstall test exercises the command against a placed driver. + +use flate2::{Compression, write::GzEncoder}; +use fs_err as fs; +use sha2::{Digest, Sha256}; +use tar::{Builder, Header}; + +use crate::{ + adbc::Driver, + commands::adbc, + config::Config, + github::GitHubClient, + tests::{fixtures::TempInstallDir, mock_github}, +}; + +const LIB: &str = "libadbc_driver_postgresql.so"; +const ASSET: &str = "adbc-driver-postgresql-linux-x86_64.tar.gz"; + +/// Build a gzip-compressed tar of `(name, contents)` entries. +fn make_targz(files: &[(&str, &[u8])]) -> Vec { + let mut builder = Builder::new(GzEncoder::new(Vec::new(), Compression::default())); + for (name, data) in files { + let mut header = Header::new_gnu(); + header.set_size(data.len() as u64); + header.set_mode(0o644); + header.set_cksum(); + builder + .append_data(&mut header, name, *data) + .expect("should append tar entry"); + } + builder + .into_inner() + .expect("should finish tar") + .finish() + .expect("should finish gzip") +} + +#[tokio::test] +async fn adbc_install_places_driver_and_manifest() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + // The release asset the installer will fetch, plus its advertised digest. + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); + + // Mock GitHub: release metadata for the tag, plus the asset download. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, version, &[(ASSET, Some(&digest))]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{version}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + ) + .await + .expect("install should succeed"); + + let driver_dir = temp + .versions_dir() + .join(version) + .join("drivers") + .join("postgresql"); + assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); + assert!(driver_dir.join("LICENSE.txt").exists(), "LICENSE placed"); + assert!(driver_dir.join("NOTICE.txt").exists(), "NOTICE placed"); + + let manifest = fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); + let parsed: toml::Value = toml::from_str(&manifest).expect("manifest is valid TOML"); + assert_eq!( + parsed["Driver"]["shared"].as_str(), + Some(driver_dir.join(LIB).to_string_lossy().as_ref()), + "Driver.shared points at the placed library", + ); +} + +#[test] +fn adbc_uninstall_removes_installed_driver() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + let drivers_dir = temp.versions_dir().join(version).join("drivers"); + let driver_dir = drivers_dir.join("postgresql"); + fs::create_dir_all(&driver_dir).expect("driver dir"); + fs::write(driver_dir.join(LIB), b"ELF").expect("lib"); + fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql") + .expect("uninstall should succeed"); + + assert!(!driver_dir.exists(), "driver directory removed"); + assert!( + !drivers_dir.exists(), + "emptied drivers directory pruned after removing the last driver", + ); +} diff --git a/ampup/src/tests/mock_github.rs b/ampup/src/tests/mock_github.rs new file mode 100644 index 0000000..1bd8993 --- /dev/null +++ b/ampup/src/tests/mock_github.rs @@ -0,0 +1,107 @@ +//! A minimal in-process HTTP mock for the GitHub release API, used by +//! integration tests that drive the download path without a network. +//! +//! Bind a listener, describe the routes, and point a +//! [`GitHubClient`](crate::github::GitHubClient) at the returned base URL via +//! `with_api_base`. Requests are matched by a path substring. + +use tokio::io::{AsyncReadExt, AsyncWriteExt}; + +/// A single mock route: any request whose path contains `prefix` receives +/// `body` with a 200 response. +#[derive(Clone)] +pub(crate) struct Route { + pub prefix: String, + pub body: Vec, +} + +impl Route { + pub fn ok(prefix: impl Into, body: Vec) -> Self { + Self { + prefix: prefix.into(), + body, + } + } +} + +/// Build the release-metadata JSON GitHub returns for a tag, advertising each +/// asset's download URL (served by the same mock) and optional digest. +pub(crate) fn release_json( + addr: std::net::SocketAddr, + tag: &str, + assets: &[(&str, Option<&str>)], +) -> Vec { + let assets: Vec = assets + .iter() + .enumerate() + .map(|(i, (name, digest))| { + let digest_field = match digest { + Some(d) => format!(r#","digest":"{d}""#), + None => String::new(), + }; + format!( + r#"{{"id":{},"name":"{}","browser_download_url":"http://{}/download/{}"{}}}"#, + i + 1, + name, + addr, + name, + digest_field, + ) + }) + .collect(); + format!( + r#"{{"tag_name":"{}","assets":[{}]}}"#, + tag, + assets.join(",") + ) + .into_bytes() +} + +/// Spawn the mock server on a pre-bound listener. It reads each request's path, +/// returns the first matching route's body with 200, or 404 if none match. +pub(crate) fn start( + listener: tokio::net::TcpListener, + routes: Vec, +) -> tokio::task::JoinHandle<()> { + tokio::spawn(async move { + loop { + let Ok((mut stream, _)) = listener.accept().await else { + break; + }; + let routes = routes.clone(); + + tokio::spawn(async move { + let mut buf = [0u8; 4096]; + let n = stream.read(&mut buf).await.expect("should read request"); + let request = String::from_utf8_lossy(&buf[..n]); + let path = request + .lines() + .next() + .and_then(|line| line.split_whitespace().nth(1)) + .unwrap_or("/"); + + let response = routes + .iter() + .find(|r| path.contains(r.prefix.as_str())) + .map(|route| { + format!( + "HTTP/1.1 200 OK\r\nContent-Length: {}\r\n\r\n", + route.body.len() + ) + .into_bytes() + .into_iter() + .chain(route.body.iter().copied()) + .collect::>() + }) + .unwrap_or_else(|| { + b"HTTP/1.1 404 Not Found\r\nContent-Length: 0\r\n\r\n".to_vec() + }); + + stream + .write_all(&response) + .await + .expect("should write response"); + }); + } + }) +} diff --git a/ampup/src/tests/mod.rs b/ampup/src/tests/mod.rs index f43f76c..81cbaa5 100644 --- a/ampup/src/tests/mod.rs +++ b/ampup/src/tests/mod.rs @@ -1,2 +1,4 @@ mod fixtures; +mod it_adbc; mod it_ampup; +mod mock_github; From 656928256cb9c055f6896fd0ce6793c00ee6fbd2 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 05:30:59 +0000 Subject: [PATCH 11/18] fix(adbc): require a digest for driver assets verify_artifact only checks that the download is non-empty when no digest is advertised. Driver assets always carry one, so treat a missing digest as a malformed release and refuse rather than install a library that ampd would load unverified. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 11 +++++++++- ampup/src/tests/it_adbc.rs | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 1 deletion(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 93f57e6..4d5e713 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -67,7 +67,16 @@ pub(crate) async fn install_driver( .expect("a required asset resolves to Some or errors"); let data = download_with_retry(github, &asset).await?; - verify_artifact(&asset.name, &data, asset.digest.as_deref()) + // Driver assets always advertise a digest, so a missing one means the + // release is malformed. Refuse rather than install a library that would be + // loaded into ampd without its integrity checked. + let digest = asset.digest.as_deref().ok_or_else(|| { + anyhow!( + "release asset {} has no digest; refusing to install an unverified driver", + asset.name + ) + })?; + verify_artifact(&asset.name, &data, Some(digest)) .context("driver archive failed verification")?; // Stage inside the drivers directory so the final rename stays on one diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index 378f9a3..ee48f00 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -97,6 +97,50 @@ async fn adbc_install_places_driver_and_manifest() { ); } +#[tokio::test] +async fn adbc_install_rejects_asset_without_digest() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + + // Same release, except the asset advertises no digest. + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, version, &[(ASSET, None)]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{version}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + let err = adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + ) + .await + .expect_err("install should refuse an asset without a digest"); + assert!(err.to_string().contains("no digest"), "got: {err}"); + + assert!( + !temp.versions_dir().join(version).join("drivers").exists(), + "nothing should be installed when the digest is missing", + ); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); From eb9911c2bf275e374a83b08e28dc50ff82d2954b Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 06:16:30 +0000 Subject: [PATCH 12/18] feat(adbc): add --version and require the version to be installed The adbc commands were pinned to the active version, unlike `ampup install` which takes one. Add --version to install, list, and uninstall. Resolving the version now also requires it to be installed. Installing amp replaces the whole version directory, so drivers placed under a version whose binaries are not there yet would be destroyed by the next `ampup install`. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 62 ++++++++++++++++++++++++++------------ ampup/src/main.rs | 41 ++++++++++++++++++++----- ampup/src/tests/it_adbc.rs | 43 ++++++++++++++++++++++++-- 3 files changed, 117 insertions(+), 29 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 4d5e713..5ecc266 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -19,7 +19,7 @@ use crate::{ version_manager::VersionManager, }; -/// Install an ADBC driver for the active amp version. +/// Install an ADBC driver for an amp version (the active one by default). pub async fn install( driver: &str, install_dir: Option, @@ -27,15 +27,16 @@ pub async fn install( github_token: Option, arch: Option, platform: Option, + version: Option, ) -> Result<()> { let driver = parse_driver(driver)?; let config = Config::new(install_dir)?; let github = GitHubClient::new(repo, token::resolve_github_token(github_token))?; - install_driver(&github, config, driver, arch, platform).await + install_driver(&github, config, driver, arch, platform, version).await } -/// Fetch, verify, extract, and place `driver` for the active amp version using -/// an already-constructed GitHub client. Split from [`install`] so tests can +/// Fetch, verify, extract, and place `driver` for an amp version using an +/// already-constructed GitHub client. Split from [`install`] so tests can /// inject a client pointed at a mock server. pub(crate) async fn install_driver( github: &GitHubClient, @@ -43,13 +44,10 @@ pub(crate) async fn install_driver( driver: Driver, arch: Option, platform: Option, + version: Option, ) -> Result<()> { let version_manager = VersionManager::new(config); - - // Drivers are pinned to an installed amp version, so one must be active. - let version = version_manager - .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + let version = resolve_version(&version_manager, version)?; let platform = resolve_platform(platform)?; let arch = resolve_arch(arch)?; @@ -130,15 +128,18 @@ fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> Ok(()) } -/// List installed ADBC drivers for the active amp version. -pub fn list(install_dir: Option) -> Result<()> { +/// List installed ADBC drivers for an amp version (the active one by default). +pub fn list(install_dir: Option, version: Option) -> Result<()> { let config = Config::new(install_dir)?; let version_manager = VersionManager::new(config); - let Some(version) = version_manager.get_current()? else { + // Without an explicit version, having none active is an empty state rather + // than an error. + if version.is_none() && version_manager.get_current()?.is_none() { ui::info!("No active amp version"); return Ok(()); - }; + } + let version = resolve_version(&version_manager, version)?; let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; if drivers.is_empty() { @@ -156,16 +157,17 @@ pub fn list(install_dir: Option) -> Result<()> { Ok(()) } -/// Uninstall an ADBC driver from the active amp version. -pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { +/// Uninstall an ADBC driver from an amp version (the active one by default). +pub fn uninstall( + install_dir: Option, + driver: &str, + version: Option, +) -> Result<()> { let driver = parse_driver(driver)?; let config = Config::new(install_dir)?; let version_manager = VersionManager::new(config); - - let version = version_manager - .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?; + let version = resolve_version(&version_manager, version)?; let driver_dir = version_manager .config() @@ -190,6 +192,28 @@ pub fn uninstall(install_dir: Option, driver: &str) -> Result<()> { Ok(()) } +/// Resolve the amp version to operate on: an explicit one, or the active one. +/// +/// The version must already be installed. Installing amp replaces the whole +/// version directory, so drivers placed under a version whose binaries are not +/// there yet would be destroyed by the next `ampup install`. +fn resolve_version(version_manager: &VersionManager, version: Option) -> Result { + let version = match version { + Some(version) => version, + None => version_manager + .get_current()? + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?, + }; + + if !version_manager.is_installed(&version) { + bail!( + "amp {} is not installed; run `ampup install {version}` first", + ui::version(&version), + ); + } + Ok(version) +} + /// The catalog drivers currently installed under `drivers_dir`. /// /// Only complete installs count: an entry must be a directory named after a diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 6fbd8c5..15cd10e 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -185,7 +185,7 @@ enum SelfCommands { #[derive(Debug, clap::Subcommand)] enum AdbcCommands { - /// Install an ADBC driver for the active amp version + /// Install an ADBC driver for an amp version Install { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] @@ -209,16 +209,24 @@ enum AdbcCommands { /// Override platform detection (linux, darwin) #[arg(long)] platform: Option, + + /// Amp version to install the driver for (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, - /// List installed ADBC drivers for the active version + /// List installed ADBC drivers for an amp version List { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] install_dir: Option, + + /// Amp version to list drivers for (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, - /// Uninstall an ADBC driver + /// Uninstall an ADBC driver from an amp version Uninstall { /// Installation directory (defaults to $AMP_DIR or $XDG_CONFIG_HOME/.amp or $HOME/.amp) #[arg(long, env = "AMP_DIR")] @@ -226,6 +234,10 @@ enum AdbcCommands { /// Driver to uninstall (e.g. postgresql) driver: String, + + /// Amp version to uninstall the driver from (defaults to the active one) + #[arg(long = "version")] + amp_version: Option, }, } @@ -326,18 +338,31 @@ async fn run() -> anyhow::Result<()> { github_token, arch, platform, + amp_version, } => { - commands::adbc::install(&driver, install_dir, repo, github_token, arch, platform) - .await?; + commands::adbc::install( + &driver, + install_dir, + repo, + github_token, + arch, + platform, + amp_version, + ) + .await?; } - AdbcCommands::List { install_dir } => { - commands::adbc::list(install_dir)?; + AdbcCommands::List { + install_dir, + amp_version, + } => { + commands::adbc::list(install_dir, amp_version)?; } AdbcCommands::Uninstall { install_dir, driver, + amp_version, } => { - commands::adbc::uninstall(install_dir, &driver)?; + commands::adbc::uninstall(install_dir, &driver, amp_version)?; } }, None => { diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index ee48f00..b581281 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -15,7 +15,10 @@ use crate::{ commands::adbc, config::Config, github::GitHubClient, - tests::{fixtures::TempInstallDir, mock_github}, + tests::{ + fixtures::{MockBinary, TempInstallDir}, + mock_github, + }, }; const LIB: &str = "libadbc_driver_postgresql.so"; @@ -45,6 +48,7 @@ async fn adbc_install_places_driver_and_manifest() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); // The release asset the installer will fetch, plus its advertised digest. let tarball = make_targz(&[ @@ -75,6 +79,7 @@ async fn adbc_install_places_driver_and_manifest() { Driver::Postgresql, Some("x86_64".to_string()), Some("linux".to_string()), + None, ) .await .expect("install should succeed"); @@ -102,6 +107,7 @@ async fn adbc_install_rejects_asset_without_digest() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); let tarball = make_targz(&[ (LIB, b"ELF"), @@ -130,6 +136,7 @@ async fn adbc_install_rejects_asset_without_digest() { Driver::Postgresql, Some("x86_64".to_string()), Some("linux".to_string()), + None, ) .await .expect_err("install should refuse an asset without a digest"); @@ -141,11 +148,43 @@ async fn adbc_install_rejects_asset_without_digest() { ); } +#[tokio::test] +async fn adbc_install_refuses_a_version_that_is_not_installed() { + let temp = TempInstallDir::new().expect("temp install dir"); + // v1.0.0 is active and installed; v2.0.0 has no binaries. + fs::write(temp.current_version_file(), "v1.0.0").expect("write active version"); + MockBinary::create(&temp, "v1.0.0").expect("install version binaries"); + + // No mock server: resolving the version fails before anything is fetched. + let github = GitHubClient::with_api_base("http://127.0.0.1:1".to_string()).expect("client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + let err = adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + Some("v2.0.0".to_string()), + ) + .await + .expect_err("install should refuse a version that is not installed"); + assert!(err.to_string().contains("is not installed"), "got: {err}"); + + // Installing amp replaces the whole version directory, so drivers must not + // be placed under a version whose binaries are not there yet. + assert!( + !temp.versions_dir().join("v2.0.0").exists(), + "no version directory should be created for an uninstalled version", + ); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); let drivers_dir = temp.versions_dir().join(version).join("drivers"); let driver_dir = drivers_dir.join("postgresql"); @@ -153,7 +192,7 @@ fn adbc_uninstall_removes_installed_driver() { fs::write(driver_dir.join(LIB), b"ELF").expect("lib"); fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); - adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql") + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) .expect("uninstall should succeed"); assert!(!driver_dir.exists(), "driver directory removed"); From 2c237fc59df11961f697e97068dcf0acae6870ec Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 07:20:05 +0000 Subject: [PATCH 13/18] fix(adbc): only gate installation on the version being installed Requiring installed binaries for list and uninstall left an orphaned driver directory impossible to inspect or remove, and made a read-only query fail on a stale .version. Split the check out of version resolution so it guards installation only, and reuse VersionError so the message matches the rest of the CLI. Also name the flag's value VERSION in help, and cover installing for an explicit non-active version plus uninstalling from a version whose binaries are gone. Part of edgeandnode/amp#2600. --- ampup/src/commands/adbc.rs | 54 +++++++++++++++---------- ampup/src/main.rs | 6 +-- ampup/src/tests/it_adbc.rs | 81 +++++++++++++++++++++++++++++++++++++- 3 files changed, 117 insertions(+), 24 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index 5ecc266..db794ba 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -16,7 +16,7 @@ use crate::{ github::GitHubClient, platform::{Architecture, Platform, PlatformError}, token, ui, - version_manager::VersionManager, + version_manager::{VersionError, VersionManager}, }; /// Install an ADBC driver for an amp version (the active one by default). @@ -48,6 +48,7 @@ pub(crate) async fn install_driver( ) -> Result<()> { let version_manager = VersionManager::new(config); let version = resolve_version(&version_manager, version)?; + ensure_installed(&version_manager, &version)?; let platform = resolve_platform(platform)?; let arch = resolve_arch(arch)?; @@ -135,11 +136,16 @@ pub fn list(install_dir: Option, version: Option) -> Result<()> // Without an explicit version, having none active is an empty state rather // than an error. - if version.is_none() && version_manager.get_current()?.is_none() { - ui::info!("No active amp version"); - return Ok(()); - } - let version = resolve_version(&version_manager, version)?; + let version = match version { + Some(version) => version, + None => match version_manager.get_current()? { + Some(version) => version, + None => { + ui::info!("No active amp version"); + return Ok(()); + } + }, + }; let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; if drivers.is_empty() { @@ -193,25 +199,33 @@ pub fn uninstall( } /// Resolve the amp version to operate on: an explicit one, or the active one. -/// -/// The version must already be installed. Installing amp replaces the whole -/// version directory, so drivers placed under a version whose binaries are not -/// there yet would be destroyed by the next `ampup install`. fn resolve_version(version_manager: &VersionManager, version: Option) -> Result { - let version = match version { - Some(version) => version, + match version { + Some(version) => Ok(version), None => version_manager .get_current()? - .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first"))?, - }; + .ok_or_else(|| anyhow!("no active amp version; run `ampup install` first")), + } +} - if !version_manager.is_installed(&version) { - bail!( - "amp {} is not installed; run `ampup install {version}` first", - ui::version(&version), - ); +/// Require `version`'s binaries before placing drivers under it. +/// +/// `ampup install ` skips all work when ``'s binaries are already there; +/// otherwise it replaces the whole version directory, taking any `drivers/` +/// subdirectory with it. Installing only into versions that are past that +/// short-circuit keeps drivers from being destroyed by a later install. +/// +/// Only installation is gated: listing and uninstalling must still work on a +/// version whose binaries have gone missing, or an orphaned driver directory +/// could never be inspected or removed. +fn ensure_installed(version_manager: &VersionManager, version: &str) -> Result<()> { + if !version_manager.is_installed(version) { + return Err(VersionError::NotInstalled { + version: version.to_string(), + } + .into()); } - Ok(version) + Ok(()) } /// The catalog drivers currently installed under `drivers_dir`. diff --git a/ampup/src/main.rs b/ampup/src/main.rs index 15cd10e..bf6de88 100644 --- a/ampup/src/main.rs +++ b/ampup/src/main.rs @@ -211,7 +211,7 @@ enum AdbcCommands { platform: Option, /// Amp version to install the driver for (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, @@ -222,7 +222,7 @@ enum AdbcCommands { install_dir: Option, /// Amp version to list drivers for (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, @@ -236,7 +236,7 @@ enum AdbcCommands { driver: String, /// Amp version to uninstall the driver from (defaults to the active one) - #[arg(long = "version")] + #[arg(long = "version", value_name = "VERSION")] amp_version: Option, }, } diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index b581281..f19230f 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -169,7 +169,7 @@ async fn adbc_install_refuses_a_version_that_is_not_installed() { ) .await .expect_err("install should refuse a version that is not installed"); - assert!(err.to_string().contains("is not installed"), "got: {err}"); + assert!(err.to_string().contains("not installed"), "got: {err}"); // Installing amp replaces the whole version directory, so drivers must not // be placed under a version whose binaries are not there yet. @@ -179,6 +179,85 @@ async fn adbc_install_refuses_a_version_that_is_not_installed() { ); } +#[tokio::test] +async fn adbc_install_targets_an_explicit_version() { + let temp = TempInstallDir::new().expect("temp install dir"); + // Active version differs from the one being targeted. + fs::write(temp.current_version_file(), "v1.0.0").expect("write active version"); + MockBinary::create(&temp, "v1.0.0").expect("install active version binaries"); + let target = "v2.0.0"; + MockBinary::create(&temp, target).expect("install target version binaries"); + + let tarball = make_targz(&[ + (LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]); + let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); + + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind mock server"); + let addr = listener.local_addr().expect("mock server address"); + let release = mock_github::release_json(addr, target, &[(ASSET, Some(&digest))]); + let routes = vec![ + mock_github::Route::ok(format!("tags/{target}"), release), + mock_github::Route::ok("download/", tarball), + ]; + let _server = mock_github::start(listener, routes); + + let github = GitHubClient::with_api_base(format!("http://{addr}")).expect("mock client"); + let config = Config::new(Some(temp.path().to_path_buf())).expect("config"); + + adbc::install_driver( + &github, + config, + Driver::Postgresql, + Some("x86_64".to_string()), + Some("linux".to_string()), + Some(target.to_string()), + ) + .await + .expect("install should succeed for an explicit version"); + + // The driver lands under the requested version, not the active one. + assert!( + temp.versions_dir() + .join(target) + .join("drivers") + .join("postgresql") + .join("manifest.toml") + .exists(), + "driver installed under the requested version", + ); + assert!( + !temp.versions_dir().join("v1.0.0").join("drivers").exists(), + "the active version should be untouched", + ); +} + +#[test] +fn adbc_uninstall_works_for_a_version_without_binaries() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + + // An orphaned driver directory: binaries are gone, drivers remain. Cleanup + // must still work, otherwise it could never be removed with ampup. + let driver_dir = temp + .versions_dir() + .join(version) + .join("drivers") + .join("postgresql"); + fs::create_dir_all(&driver_dir).expect("driver dir"); + fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) + .expect("uninstall should work without the version's binaries"); + + assert!(!driver_dir.exists(), "orphaned driver directory removed"); +} + #[test] fn adbc_uninstall_removes_installed_driver() { let temp = TempInstallDir::new().expect("temp install dir"); From 5403be6c719f296f772bd659f64747bb6197891a Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 07:20:05 +0000 Subject: [PATCH 14/18] test: serialize the tests that swap PATH build_from_local_path_with_custom_name and rebuild_removes_stale_optional_binaries both replace the process-global PATH to install a mock cargo. Run concurrently, one restores the original value while the other is mid-build, so the mock disappears and the real cargo fails against the fake repo. Share a mutex so they cannot overlap. --- ampup/src/tests/it_ampup.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ampup/src/tests/it_ampup.rs b/ampup/src/tests/it_ampup.rs index 304e519..fa13b29 100644 --- a/ampup/src/tests/it_ampup.rs +++ b/ampup/src/tests/it_ampup.rs @@ -7,6 +7,11 @@ use tempfile::TempDir; use super::fixtures::{MockBinary, TempInstallDir}; use crate::{DEFAULT_DOWNLOAD_JOBS, DEFAULT_REPO}; +/// Serializes the tests that swap `PATH` to install a mock `cargo`. `PATH` is +/// process-global, so two of them running at once restore each other's value +/// mid-build and the mock binary disappears. +static PATH_LOCK: tokio::sync::Mutex<()> = tokio::sync::Mutex::const_new(()); + #[tokio::test] async fn init_creates_directory_structure() -> Result<()> { let temp = TempInstallDir::new()?; @@ -317,6 +322,7 @@ async fn build_from_local_path_with_custom_name() -> Result<()> { } // Temporarily modify PATH to use mock cargo + let _path_guard = PATH_LOCK.lock().await; let original_path = env::var("PATH").unwrap_or_default(); let new_path = format!("{}:{}", mock_cargo_dir.path().display(), original_path); unsafe { @@ -393,6 +399,7 @@ async fn rebuild_removes_stale_optional_binaries() -> Result<()> { write_executable(&mock_cargo, "#!/bin/sh\nexit 0")?; // Temporarily modify PATH to use mock cargo. + let _path_guard = PATH_LOCK.lock().await; let original_path = env::var("PATH").unwrap_or_default(); let new_path = format!("{}:{}", mock_cargo_dir.path().display(), original_path); unsafe { From 4ae8a7d848687ca5d3e58dc366b623c76b20b372 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Thu, 23 Jul 2026 13:26:12 +0000 Subject: [PATCH 15/18] refactor(adbc): install drivers flat into the version directory Drivers were installed into a self-contained drivers// directory holding an ADBC manifest.toml beside the library. Install them directly into the version directory instead, alongside the amp binaries, and drop the manifest: with the library at a known path, a manifest naming its own neighbour is indirection amp does not need. That also removes the toml dependency. The library is renamed to amp-adbc-driver-.{so,dylib} on install, so its filename is what distinguishes it from ampd/ampctl/ampsql now that they share a directory. License files are namespaced the same way. list and uninstall check the catalog's expected filenames rather than walking a directory, and cover every supported platform rather than the host's: `adbc install --platform` can place a library this host would not look for, which would otherwise strand a file no ampup command could remove. Placement is now per-file renames rather than one directory rename, so a failed move rolls back the files that already landed; without that a partial install would leave the library behind and list would report a driver whose installation had errored. Part of edgeandnode/amp#2600. --- Cargo.lock | 55 ------ ampup/Cargo.toml | 1 - ampup/src/adbc.rs | 123 +++++++------ ampup/src/commands/adbc.rs | 365 ++++++++++++++++++++++++------------- ampup/src/config.rs | 21 +-- ampup/src/platform.rs | 6 + ampup/src/tests/it_adbc.rs | 203 +++++++++++++-------- 7 files changed, 451 insertions(+), 323 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c45e74..5760403 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -26,7 +26,6 @@ dependencies = [ "tar", "tempfile", "tokio", - "toml", "vergen-gitcl", ] @@ -1523,15 +1522,6 @@ dependencies = [ "zmij", ] -[[package]] -name = "serde_spanned" -version = "1.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6662b5879511e06e8999a8a235d848113e942c9124f211511b16466ee2995f26" -dependencies = [ - "serde_core", -] - [[package]] name = "serde_urlencoded" version = "0.7.1" @@ -1816,45 +1806,6 @@ dependencies = [ "tokio", ] -[[package]] -name = "toml" -version = "1.1.3+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "53c96ecdfa941c8fc4fcaed14f99ada8ebed502eef533015095a07e3301d4c3c" -dependencies = [ - "indexmap", - "serde_core", - "serde_spanned", - "toml_datetime", - "toml_parser", - "toml_writer", - "winnow", -] - -[[package]] -name = "toml_datetime" -version = "1.1.1+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "3165f65f62e28e0115a00b2ebdd37eb6f3b641855f9d636d3cd4103767159ad7" -dependencies = [ - "serde_core", -] - -[[package]] -name = "toml_parser" -version = "1.1.2+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2abe9b86193656635d2411dc43050282ca48aa31c2451210f4202550afb7526" -dependencies = [ - "winnow", -] - -[[package]] -name = "toml_writer" -version = "1.1.2+spec-1.1.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "7d56353a2a665ad0f41a421187180aab746c8c325620617ad883a99a1cbe66d2" - [[package]] name = "tower" version = "0.5.3" @@ -2438,12 +2389,6 @@ version = "0.53.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d6bbff5f0aada427a1e5a6da5f1f98158182f26556f345ac9e04d36d0ebed650" -[[package]] -name = "winnow" -version = "1.0.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "23b97319f7b8343df12cc98938e5c3eb436064524c8d2b4e30a1d3a36eecdf81" - [[package]] name = "wit-bindgen" version = "0.51.0" diff --git a/ampup/Cargo.toml b/ampup/Cargo.toml index 7300fd4..cbef488 100644 --- a/ampup/Cargo.toml +++ b/ampup/Cargo.toml @@ -36,7 +36,6 @@ tokio = { version = "1.36.0", features = [ "sync", "test-util", ] } -toml = "1.1.3" [build-dependencies] vergen-gitcl = { version = "9.0.0", features = ["build"] } diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index b93ed2f..96e0020 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -4,12 +4,12 @@ //! This module maps a supported driver plus a target platform/arch to the //! release asset that carries it. -use std::path::Path; - -use serde::Serialize; - use crate::platform::{Architecture, Platform}; +/// Prefix every installed driver library carries, namespacing it against the +/// amp binaries it sits beside in a version directory. +pub const DRIVER_FILE_PREFIX: &str = "amp-adbc-driver-"; + /// A supported ADBC driver, as shipped in Amp releases. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum Driver { @@ -32,14 +32,39 @@ impl Driver { Self::ALL.iter().copied().find(|d| d.as_str() == name) } - /// The runtime library filename this driver's archive carries on - /// `platform` (e.g. `libadbc_driver_postgresql.so` on Linux). - pub fn runtime_lib_filename(&self, platform: Platform) -> String { - let ext = match platform { - Platform::Linux => "so", - Platform::Darwin => "dylib", - }; - format!("libadbc_driver_{}.{ext}", self.as_str()) + /// The library filename this driver's release archive carries on + /// `platform` (e.g. `libadbc_driver_postgresql.so` on Linux) — upstream's + /// own name, which the archive is validated against. + pub fn archive_lib_filename(&self, platform: Platform) -> String { + format!( + "libadbc_driver_{}.{}", + self.as_str(), + lib_extension(platform) + ) + } + + /// The library filename this driver is installed under, prefixed so it is + /// distinguishable from the amp binaries sharing its directory. + pub fn installed_lib_filename(&self, platform: Platform) -> String { + format!( + "{DRIVER_FILE_PREFIX}{}.{}", + self.as_str(), + lib_extension(platform) + ) + } + + /// The stem shared by this driver's installed files, used for the license + /// sidecars. Platform-independent, so cleanup needs no platform. + pub fn installed_stem(&self) -> String { + format!("{DRIVER_FILE_PREFIX}{}", self.as_str()) + } +} + +/// The dynamic library extension for `platform`. +fn lib_extension(platform: Platform) -> &'static str { + match platform { + Platform::Linux => "so", + Platform::Darwin => "dylib", } } @@ -62,36 +87,6 @@ pub fn asset_name(driver: Driver, platform: Platform, arch: Architecture) -> Str ) } -/// An ADBC driver manifest (`manifest.toml`), the on-disk contract `ampd` -/// reads to load a driver by absolute path. -/// -/// Only `Driver.shared` is required; the postgres driver's entrypoint is -/// derived from its name, so no `entrypoint` key is emitted. -#[derive(Serialize)] -struct Manifest { - manifest_version: u32, - #[serde(rename = "Driver")] - driver: DriverSection, -} - -#[derive(Serialize)] -struct DriverSection { - shared: String, -} - -/// Render the `manifest.toml` contents pointing `ampd` at the driver library -/// at `lib_path` (an absolute path). -pub fn driver_manifest(lib_path: &Path) -> String { - let manifest = Manifest { - manifest_version: 1, - driver: DriverSection { - shared: lib_path.to_string_lossy().into_owned(), - }, - }; - // Serializing a fixed two-field struct cannot fail. - toml::to_string(&manifest).expect("driver manifest serializes") -} - #[cfg(test)] mod tests { use super::*; @@ -116,29 +111,47 @@ mod tests { } #[test] - fn runtime_lib_filename_is_platform_specific() { + fn archive_lib_filename_is_the_upstream_platform_specific_name() { assert_eq!( - Driver::Postgresql.runtime_lib_filename(Platform::Linux), + Driver::Postgresql.archive_lib_filename(Platform::Linux), "libadbc_driver_postgresql.so", ); assert_eq!( - Driver::Postgresql.runtime_lib_filename(Platform::Darwin), + Driver::Postgresql.archive_lib_filename(Platform::Darwin), "libadbc_driver_postgresql.dylib", ); } #[test] - fn driver_manifest_round_trips_the_library_path() { - // A path with a quote and a backslash must survive escaping so the - // manifest stays valid TOML that parses back to the same path. - let path = Path::new("/home/a\"b\\c/drivers/postgresql/libadbc_driver_postgresql.so"); - let rendered = driver_manifest(path); - - let parsed: toml::Value = toml::from_str(&rendered).expect("manifest is valid TOML"); - assert_eq!(parsed["manifest_version"].as_integer(), Some(1)); + fn installed_lib_filename_is_prefixed_and_distinct_from_the_archive_name() { + for platform in Platform::ALL.iter().copied() { + let installed = Driver::Postgresql.installed_lib_filename(platform); + assert!( + installed.starts_with(DRIVER_FILE_PREFIX), + "installed name namespaces the driver: {installed}", + ); + assert_ne!( + installed, + Driver::Postgresql.archive_lib_filename(platform), + "the driver is renamed on install", + ); + } assert_eq!( - parsed["Driver"]["shared"].as_str(), - Some(path.to_string_lossy().as_ref()), + Driver::Postgresql.installed_lib_filename(Platform::Darwin), + "amp-adbc-driver-postgresql.dylib", + ); + } + + #[test] + fn installed_stem_prefixes_every_installed_file() { + let stem = Driver::Postgresql.installed_stem(); + assert_eq!(stem, "amp-adbc-driver-postgresql"); + // The sidecars and the library share the stem, so cleanup by stem + // catches all of them without knowing the platform. + assert!( + Driver::Postgresql + .installed_lib_filename(Platform::Linux) + .starts_with(&stem) ); } } diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index db794ba..ba80dc6 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -78,55 +78,71 @@ pub(crate) async fn install_driver( verify_artifact(&asset.name, &data, Some(digest)) .context("driver archive failed verification")?; - // Stage inside the drivers directory so the final rename stays on one - // filesystem (atomic, no cross-device copy). - let drivers_dir = version_manager.config().drivers_dir(&version); - fs::create_dir_all(&drivers_dir).context("failed to create drivers directory")?; + // Stage inside the version directory so the final renames stay on one + // filesystem. TempDir cleans this up if anything below fails. + let version_dir = version_manager.config().version_dir(&version); let staging = - tempfile::tempdir_in(&drivers_dir).context("failed to create staging directory")?; + tempfile::tempdir_in(&version_dir).context("failed to create staging directory")?; - let lib = driver.runtime_lib_filename(platform); + let archive_lib = driver.archive_lib_filename(platform); archive::extract_and_validate( &data, staging.path(), - &[lib.as_str(), "LICENSE.txt", "NOTICE.txt"], + &[archive_lib.as_str(), "LICENSE.txt", "NOTICE.txt"], )?; - // Absolute so the manifest's `Driver.shared` resolves regardless of - // ampd's working directory. - let driver_dir = std::path::absolute( - version_manager - .config() - .driver_dir(&version, driver.as_str()), - ) - .context("failed to resolve driver directory")?; - place_driver(staging.path(), &driver_dir, &lib)?; - // The staged files now live at `driver_dir`; disarm TempDir cleanup. - let _ = staging.keep(); + let installed_lib = place_driver(staging.path(), &version_dir, driver, platform)?; ui::info!( "Installed {} driver for amp {} at {}", driver, ui::version(&version), - driver_dir.display(), + installed_lib.display(), ); Ok(()) } -/// Assemble the self-contained driver directory: write the manifest into the -/// staged files (pointing `Driver.shared` at the final library path), then -/// atomically move the staged directory into `driver_dir`, replacing any -/// existing install. -fn place_driver(staging: &Path, driver_dir: &Path, lib_name: &str) -> Result<()> { - let manifest = crate::adbc::driver_manifest(&driver_dir.join(lib_name)); - fs::write(staging.join("manifest.toml"), manifest) - .context("failed to write ADBC driver manifest")?; - - if driver_dir.exists() { - fs::remove_dir_all(driver_dir).context("failed to remove the existing driver directory")?; +/// Move the extracted files from `staging` into `version_dir`, renaming the +/// library to its installed (prefixed) name and namespacing the license +/// sidecars alongside it. Returns the installed library path. +/// +/// Each move is a rename within one filesystem, which replaces any existing +/// file of that name — so a reinstall overwrites in place. Renaming over a +/// library `ampd` currently has loaded is safe: the open inode survives. +/// +/// A failed move rolls back the ones that already landed. Without that, a +/// partial install would leave the library behind and `list` would report a +/// driver whose installation reported an error. +fn place_driver( + staging: &Path, + version_dir: &Path, + driver: Driver, + platform: Platform, +) -> Result { + let stem = driver.installed_stem(); + let moves = [ + ( + driver.archive_lib_filename(platform), + driver.installed_lib_filename(platform), + ), + ("LICENSE.txt".to_string(), format!("{stem}.LICENSE.txt")), + ("NOTICE.txt".to_string(), format!("{stem}.NOTICE.txt")), + ]; + + let mut placed: Vec = Vec::with_capacity(moves.len()); + for (from, to) in &moves { + let destination = version_dir.join(to); + if let Err(error) = fs::rename(staging.join(from), &destination) { + for path in &placed { + let _ = fs::remove_file(path); + } + return Err(anyhow::Error::new(error)) + .with_context(|| format!("failed to move {from} into place")); + } + placed.push(destination); } - fs::rename(staging, driver_dir).context("failed to move the driver into place")?; - Ok(()) + + Ok(version_dir.join(driver.installed_lib_filename(platform))) } /// List installed ADBC drivers for an amp version (the active one by default). @@ -147,7 +163,7 @@ pub fn list(install_dir: Option, version: Option) -> Result<()> }, }; - let drivers = installed_drivers(&version_manager.config().drivers_dir(&version))?; + let drivers = installed_drivers(&version_manager.config().version_dir(&version)); if drivers.is_empty() { ui::info!( "No ADBC drivers installed for amp {}", @@ -175,20 +191,26 @@ pub fn uninstall( let version_manager = VersionManager::new(config); let version = resolve_version(&version_manager, version)?; - let driver_dir = version_manager - .config() - .driver_dir(&version, driver.as_str()); - if !driver_dir.exists() { + let version_dir = version_manager.config().version_dir(&version); + let installed = installed_lib_paths(&version_dir, driver); + if installed.is_empty() { bail!( "the {driver} driver is not installed for amp {}", ui::version(&version) ); } - fs::remove_dir_all(&driver_dir).context("failed to remove the driver directory")?; - // Tidy up an otherwise-empty drivers directory; ignore the error when it - // still holds other drivers (or a leftover staging dir). - let _ = fs::remove_dir(version_manager.config().drivers_dir(&version)); + // The library plus its license sidecars all share the driver's stem. + let stem = driver.installed_stem(); + for name in [format!("{stem}.LICENSE.txt"), format!("{stem}.NOTICE.txt")] { + let sidecar = version_dir.join(name); + if sidecar.exists() { + fs::remove_file(&sidecar).context("failed to remove a driver license file")?; + } + } + for lib in installed { + fs::remove_file(&lib).context("failed to remove the driver library")?; + } ui::info!( "Uninstalled {} driver from amp {}", @@ -211,13 +233,13 @@ fn resolve_version(version_manager: &VersionManager, version: Option) -> /// Require `version`'s binaries before placing drivers under it. /// /// `ampup install ` skips all work when ``'s binaries are already there; -/// otherwise it replaces the whole version directory, taking any `drivers/` -/// subdirectory with it. Installing only into versions that are past that +/// otherwise it replaces the whole version directory, taking any drivers +/// installed into it. Installing only into versions that are past that /// short-circuit keeps drivers from being destroyed by a later install. /// /// Only installation is gated: listing and uninstalling must still work on a -/// version whose binaries have gone missing, or an orphaned driver directory -/// could never be inspected or removed. +/// version whose binaries have gone missing, or an orphaned driver could never +/// be inspected or removed. fn ensure_installed(version_manager: &VersionManager, version: &str) -> Result<()> { if !version_manager.is_installed(version) { return Err(VersionError::NotInstalled { @@ -228,36 +250,34 @@ fn ensure_installed(version_manager: &VersionManager, version: &str) -> Result<( Ok(()) } -/// The catalog drivers currently installed under `drivers_dir`. +/// The installed library paths for `driver` in `version_dir`, across every +/// supported platform. /// -/// Only complete installs count: an entry must be a directory named after a -/// known driver and hold a `manifest.toml`, which skips leftover staging -/// directories and partial installs. Returns empty when `drivers_dir` is -/// absent (a version with no drivers installed). -fn installed_drivers(drivers_dir: &Path) -> Result> { - if !drivers_dir.exists() { - return Ok(Vec::new()); - } +/// Platform-agnostic on purpose: `adbc install --platform` can place a library +/// for a platform other than this host's, and list/uninstall must still see it +/// rather than stranding a file no ampup command can remove. +fn installed_lib_paths(version_dir: &Path, driver: Driver) -> Vec { + Platform::ALL + .iter() + .map(|platform| version_dir.join(driver.installed_lib_filename(*platform))) + .filter(|path| path.is_file()) + .collect() +} - let mut drivers = Vec::new(); - for entry in fs::read_dir(drivers_dir).context("failed to read the drivers directory")? { - let entry = entry.context("failed to read a drivers directory entry")?; - if !entry - .file_type() - .context("failed to determine a drivers entry type")? - .is_dir() - { - continue; - } - let Some(driver) = entry.file_name().to_str().and_then(Driver::from_name) else { - continue; - }; - if entry.path().join("manifest.toml").is_file() { - drivers.push(driver); - } - } +/// The catalog drivers currently installed in `version_dir`. +/// +/// The directory also holds the amp binaries and each driver's license +/// sidecars, so membership is decided by looking up the catalog's expected +/// filenames rather than by matching names found there. A missing directory +/// yields no drivers, since the per-file checks simply do not match. +fn installed_drivers(version_dir: &Path) -> Vec { + let mut drivers: Vec = Driver::ALL + .iter() + .copied() + .filter(|driver| !installed_lib_paths(version_dir, *driver).is_empty()) + .collect(); drivers.sort_by_key(|d| d.as_str()); - Ok(drivers) + drivers } /// Resolve a driver name against the catalog, with a helpful error listing the @@ -301,96 +321,187 @@ fn resolve_arch(over: Option) -> Result { mod tests { use super::*; - const LIB: &str = "libadbc_driver_postgresql.so"; + const ARCHIVE_LIB: &str = "libadbc_driver_postgresql.so"; + const INSTALLED_LIB: &str = "amp-adbc-driver-postgresql.so"; - /// Build a staging directory holding the three extracted driver files, - /// with `lib_contents` as the library body. + /// A staging directory holding the three files a driver archive carries. fn staged_driver(parent: &Path, lib_contents: &[u8]) -> PathBuf { let staging = tempfile::tempdir_in(parent).expect("staging dir"); - fs::write(staging.path().join(LIB), lib_contents).expect("write lib"); + fs::write(staging.path().join(ARCHIVE_LIB), lib_contents).expect("write lib"); fs::write(staging.path().join("LICENSE.txt"), b"license").expect("write license"); fs::write(staging.path().join("NOTICE.txt"), b"notice").expect("write notice"); staging.keep() } - #[test] - fn place_driver_writes_files_and_manifest() { - let root = tempfile::tempdir().expect("root"); - let drivers_dir = root.path().join("drivers"); - fs::create_dir_all(&drivers_dir).expect("drivers dir"); - let staging = staged_driver(&drivers_dir, b"ELF"); - let driver_dir = drivers_dir.join("postgresql"); + /// A version directory containing the amp binaries, as a real install has. + fn version_dir_with_binaries() -> tempfile::TempDir { + let dir = tempfile::tempdir().expect("version dir"); + for binary in ["ampd", "ampctl", "ampsql"] { + fs::write(dir.path().join(binary), b"#!/bin/sh\n").expect("write binary"); + } + dir + } - place_driver(&staging, &driver_dir, LIB).expect("place"); + #[test] + fn place_driver_renames_the_library_and_namespaces_the_sidecars() { + let version_dir = version_dir_with_binaries(); + let staging = staged_driver(version_dir.path(), b"ELF"); + + let placed = place_driver( + &staging, + version_dir.path(), + Driver::Postgresql, + Platform::Linux, + ) + .expect("place"); + + assert_eq!(placed, version_dir.path().join(INSTALLED_LIB)); + assert_eq!(fs::read(&placed).expect("lib"), b"ELF"); + assert!( + version_dir + .path() + .join("amp-adbc-driver-postgresql.LICENSE.txt") + .exists(), + "license is namespaced so a second driver cannot clobber it", + ); + assert!( + version_dir + .path() + .join("amp-adbc-driver-postgresql.NOTICE.txt") + .exists() + ); + // The upstream name must not survive alongside the renamed library. + assert!(!version_dir.path().join(ARCHIVE_LIB).exists()); + // The amp binaries are untouched. + for binary in ["ampd", "ampctl", "ampsql"] { + assert!(version_dir.path().join(binary).exists(), "{binary} intact"); + } + } - assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); - assert!(driver_dir.join("LICENSE.txt").exists()); - assert!(driver_dir.join("NOTICE.txt").exists()); + #[test] + fn place_driver_overwrites_a_previous_install() { + let version_dir = version_dir_with_binaries(); + + let first = staged_driver(version_dir.path(), b"old"); + place_driver( + &first, + version_dir.path(), + Driver::Postgresql, + Platform::Linux, + ) + .expect("first"); + + let second = staged_driver(version_dir.path(), b"new"); + place_driver( + &second, + version_dir.path(), + Driver::Postgresql, + Platform::Linux, + ) + .expect("second"); - let manifest = - fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); - let parsed: toml::Value = toml::from_str(&manifest).expect("valid TOML"); assert_eq!( - parsed["Driver"]["shared"].as_str(), - Some(driver_dir.join(LIB).to_string_lossy().as_ref()), + fs::read(version_dir.path().join(INSTALLED_LIB)).expect("lib"), + b"new", ); } #[test] - fn place_driver_replaces_an_existing_install() { - let root = tempfile::tempdir().expect("root"); - let drivers_dir = root.path().join("drivers"); - fs::create_dir_all(&drivers_dir).expect("drivers dir"); - let driver_dir = drivers_dir.join("postgresql"); - - place_driver(&staged_driver(&drivers_dir, b"old"), &driver_dir, LIB).expect("first"); - // A stray file from the old install must not survive the reinstall. - fs::write(driver_dir.join("stale.txt"), b"x").expect("stray file"); + fn installed_drivers_ignores_the_amp_binaries_and_sidecars() { + let version_dir = version_dir_with_binaries(); + assert!( + installed_drivers(version_dir.path()).is_empty(), + "the amp binaries are not drivers", + ); - place_driver(&staged_driver(&drivers_dir, b"new"), &driver_dir, LIB).expect("second"); + // A license sidecar alone must not register as an installed driver. + fs::write( + version_dir + .path() + .join("amp-adbc-driver-postgresql.LICENSE.txt"), + b"license", + ) + .expect("sidecar"); + assert!( + installed_drivers(version_dir.path()).is_empty(), + "a sidecar without the library is not an install", + ); - assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"new"); - assert!(!driver_dir.join("stale.txt").exists()); + fs::write(version_dir.path().join(INSTALLED_LIB), b"ELF").expect("lib"); + assert_eq!( + installed_drivers(version_dir.path()), + vec![Driver::Postgresql], + ); } - /// Create `drivers_dir//`, optionally with a `manifest.toml`. - fn make_driver_dir(drivers_dir: &Path, name: &str, with_manifest: bool) { - let dir = drivers_dir.join(name); - fs::create_dir_all(&dir).expect("driver dir"); - if with_manifest { - fs::write(dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); - } + /// The library name for a platform that is not this host's, so the test + /// exercises the cross-platform path wherever it runs. + fn foreign_platform_lib(driver: Driver) -> String { + let host = Platform::detect().expect("supported host platform"); + let foreign = Platform::ALL + .iter() + .copied() + .find(|platform| *platform != host) + .expect("another supported platform exists"); + driver.installed_lib_filename(foreign) } #[test] - fn installed_drivers_lists_only_complete_catalog_dirs() { - let root = tempfile::tempdir().expect("root"); - let drivers_dir = root.path().join("drivers"); - make_driver_dir(&drivers_dir, "postgresql", true); // complete -> included - make_driver_dir(&drivers_dir, "mysql", true); // not in catalog -> excluded - make_driver_dir(&drivers_dir, ".tmpABC123", true); // leftover staging -> excluded - fs::write(drivers_dir.join("stray.txt"), b"x").expect("stray file"); // non-dir -> excluded + fn installed_drivers_finds_a_library_built_for_another_platform() { + // `adbc install --platform ` leaves a library this host would + // not look for. list and uninstall must still see it, or the file + // could never be removed. + let version_dir = version_dir_with_binaries(); + fs::write( + version_dir + .path() + .join(foreign_platform_lib(Driver::Postgresql)), + b"FOREIGN", + ) + .expect("lib"); assert_eq!( - installed_drivers(&drivers_dir).expect("list"), + installed_drivers(version_dir.path()), vec![Driver::Postgresql], ); } #[test] - fn installed_drivers_skips_partial_installs() { + fn installed_drivers_on_missing_dir_is_empty() { let root = tempfile::tempdir().expect("root"); - let drivers_dir = root.path().join("drivers"); - make_driver_dir(&drivers_dir, "postgresql", false); // no manifest -> excluded + let missing = root.path().join("nope"); - assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + assert!(installed_drivers(&missing).is_empty()); } #[test] - fn installed_drivers_on_missing_dir_is_empty() { - let root = tempfile::tempdir().expect("root"); - let drivers_dir = root.path().join("drivers"); // never created + fn place_driver_rolls_back_when_a_later_move_fails() { + let version_dir = version_dir_with_binaries(); + let staging = staged_driver(version_dir.path(), b"ELF"); + // A directory where a sidecar must land makes that rename fail, after + // the library has already been moved. + fs::create_dir( + version_dir + .path() + .join("amp-adbc-driver-postgresql.LICENSE.txt"), + ) + .expect("blocking directory"); + + let result = place_driver( + &staging, + version_dir.path(), + Driver::Postgresql, + Platform::Linux, + ); - assert!(installed_drivers(&drivers_dir).expect("list").is_empty()); + assert!(result.is_err(), "a blocked sidecar move fails the install"); + assert!( + !version_dir.path().join(INSTALLED_LIB).exists(), + "the library is rolled back, so list cannot report a failed install", + ); + assert!( + installed_drivers(version_dir.path()).is_empty(), + "no driver is reported after a failed install", + ); } } diff --git a/ampup/src/config.rs b/ampup/src/config.rs index 8421f29..03e3087 100644 --- a/ampup/src/config.rs +++ b/ampup/src/config.rs @@ -75,7 +75,7 @@ impl Config { /// Get the binary path for a specific version pub fn version_binary_path(&self, version: &str) -> PathBuf { - self.versions_dir.join(version).join("ampd") + self.version_dir(version).join("ampd") } /// Get the active ampd binary symlink path @@ -85,7 +85,7 @@ impl Config { /// Get the ampctl binary path for a specific version pub fn version_ampctl_path(&self, version: &str) -> PathBuf { - self.versions_dir.join(version).join("ampctl") + self.version_dir(version).join("ampctl") } /// Get the active ampctl binary symlink path @@ -95,19 +95,14 @@ impl Config { /// Get the ampsql binary path for a specific version pub fn version_ampsql_path(&self, version: &str) -> PathBuf { - self.versions_dir.join(version).join("ampsql") + self.version_dir(version).join("ampsql") } - /// Get the ADBC drivers directory for a specific version - /// (~/.amp/versions//drivers) - pub fn drivers_dir(&self, version: &str) -> PathBuf { - self.versions_dir.join(version).join("drivers") - } - - /// Get the self-contained directory for a single ADBC driver under a - /// version (~/.amp/versions//drivers/) - pub fn driver_dir(&self, version: &str, driver: &str) -> PathBuf { - self.drivers_dir(version).join(driver) + /// Get the directory holding a version's installed files + /// (~/.amp/versions/): the amp binaries and any ADBC drivers + /// installed alongside them. + pub fn version_dir(&self, version: &str) -> PathBuf { + self.versions_dir.join(version) } /// Get the active ampsql binary symlink path diff --git a/ampup/src/platform.rs b/ampup/src/platform.rs index f058a78..d56d0d0 100644 --- a/ampup/src/platform.rs +++ b/ampup/src/platform.rs @@ -46,6 +46,12 @@ pub enum Platform { } impl Platform { + /// Every supported platform. + /// + /// Lets callers cover a version directory that may have been populated for + /// a platform other than this one (`ampup adbc install --platform`). + pub const ALL: &'static [Platform] = &[Platform::Linux, Platform::Darwin]; + /// Detect the current platform pub fn detect() -> Result { match std::env::consts::OS { diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index f19230f..a2cd1d0 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -1,9 +1,10 @@ //! Integration tests for the `ampup adbc` commands (#2600). //! -//! `adbc_install_places_driver_and_manifest` drives the whole install path -//! (fetch release metadata -> download -> verify digest -> extract -> place + -//! manifest) against an in-process mock GitHub server, so no network is -//! required. The uninstall test exercises the command against a placed driver. +//! `adbc_install_places_driver_in_the_version_dir` drives the whole install +//! path (fetch release metadata -> download -> verify digest -> extract -> +//! rename into the version directory) against an in-process mock GitHub +//! server, so no network is required. The uninstall tests exercise the command +//! against a placed driver. use flate2::{Compression, write::GzEncoder}; use fs_err as fs; @@ -11,7 +12,7 @@ use sha2::{Digest, Sha256}; use tar::{Builder, Header}; use crate::{ - adbc::Driver, + adbc::{DRIVER_FILE_PREFIX, Driver}, commands::adbc, config::Config, github::GitHubClient, @@ -21,7 +22,10 @@ use crate::{ }, }; -const LIB: &str = "libadbc_driver_postgresql.so"; +/// The library name inside the release archive (upstream's). +const ARCHIVE_LIB: &str = "libadbc_driver_postgresql.so"; +/// The name it is installed under. +const INSTALLED_LIB: &str = "amp-adbc-driver-postgresql.so"; const ASSET: &str = "adbc-driver-postgresql-linux-x86_64.tar.gz"; /// Build a gzip-compressed tar of `(name, contents)` entries. @@ -43,19 +47,40 @@ fn make_targz(files: &[(&str, &[u8])]) -> Vec { .expect("should finish gzip") } +/// The driver archive as the release ships it. +fn driver_tarball() -> Vec { + make_targz(&[ + (ARCHIVE_LIB, b"ELF"), + ("LICENSE.txt", b"license"), + ("NOTICE.txt", b"notice"), + ]) +} + +/// Names of any driver-owned or leftover staging entries in `version_dir`, +/// ignoring the amp binaries that share it. +/// +/// What "no driver installed" means now that drivers are files among others. +fn driver_artifacts(version_dir: &std::path::Path) -> Vec { + let Ok(entries) = fs::read_dir(version_dir) else { + return Vec::new(); + }; + let mut found: Vec = entries + .filter_map(|entry| entry.ok()) + .map(|entry| entry.file_name().to_string_lossy().into_owned()) + .filter(|name| name.starts_with(DRIVER_FILE_PREFIX) || name.starts_with(".tmp")) + .collect(); + found.sort(); + found +} + #[tokio::test] -async fn adbc_install_places_driver_and_manifest() { +async fn adbc_install_places_driver_in_the_version_dir() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); MockBinary::create(&temp, version).expect("install version binaries"); - // The release asset the installer will fetch, plus its advertised digest. - let tarball = make_targz(&[ - (LIB, b"ELF"), - ("LICENSE.txt", b"license"), - ("NOTICE.txt", b"notice"), - ]); + let tarball = driver_tarball(); let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); // Mock GitHub: release metadata for the tag, plus the asset download. @@ -84,21 +109,40 @@ async fn adbc_install_places_driver_and_manifest() { .await .expect("install should succeed"); - let driver_dir = temp - .versions_dir() - .join(version) - .join("drivers") - .join("postgresql"); - assert_eq!(fs::read(driver_dir.join(LIB)).expect("lib"), b"ELF"); - assert!(driver_dir.join("LICENSE.txt").exists(), "LICENSE placed"); - assert!(driver_dir.join("NOTICE.txt").exists(), "NOTICE placed"); - - let manifest = fs::read_to_string(driver_dir.join("manifest.toml")).expect("manifest exists"); - let parsed: toml::Value = toml::from_str(&manifest).expect("manifest is valid TOML"); + // The library lands beside the amp binaries, under its installed name. + let version_dir = temp.version_dir(version); assert_eq!( - parsed["Driver"]["shared"].as_str(), - Some(driver_dir.join(LIB).to_string_lossy().as_ref()), - "Driver.shared points at the placed library", + fs::read(version_dir.join(INSTALLED_LIB)).expect("lib"), + b"ELF", + ); + assert!( + !version_dir.join(ARCHIVE_LIB).exists(), + "the upstream name must not survive alongside the renamed library", + ); + assert!( + version_dir + .join("amp-adbc-driver-postgresql.LICENSE.txt") + .exists(), + "license sidecar is namespaced", + ); + assert!( + version_dir + .join("amp-adbc-driver-postgresql.NOTICE.txt") + .exists(), + ); + + // The amp binaries it now shares a directory with are untouched. + for binary in ["ampd", "ampctl", "ampsql"] { + assert!(version_dir.join(binary).exists(), "{binary} intact"); + } + + // No staging directory is left behind. + assert!( + !driver_artifacts(&version_dir) + .iter() + .any(|name| name.starts_with(".tmp")), + "staging directory cleaned up: {:?}", + driver_artifacts(&version_dir), ); } @@ -109,12 +153,6 @@ async fn adbc_install_rejects_asset_without_digest() { fs::write(temp.current_version_file(), version).expect("write active version"); MockBinary::create(&temp, version).expect("install version binaries"); - let tarball = make_targz(&[ - (LIB, b"ELF"), - ("LICENSE.txt", b"license"), - ("NOTICE.txt", b"notice"), - ]); - // Same release, except the asset advertises no digest. let listener = tokio::net::TcpListener::bind("127.0.0.1:0") .await @@ -123,7 +161,7 @@ async fn adbc_install_rejects_asset_without_digest() { let release = mock_github::release_json(addr, version, &[(ASSET, None)]); let routes = vec![ mock_github::Route::ok(format!("tags/{version}"), release), - mock_github::Route::ok("download/", tarball), + mock_github::Route::ok("download/", driver_tarball()), ]; let _server = mock_github::start(listener, routes); @@ -143,8 +181,8 @@ async fn adbc_install_rejects_asset_without_digest() { assert!(err.to_string().contains("no digest"), "got: {err}"); assert!( - !temp.versions_dir().join(version).join("drivers").exists(), - "nothing should be installed when the digest is missing", + driver_artifacts(&temp.version_dir(version)).is_empty(), + "nothing installed and no staging left behind when the digest is missing", ); } @@ -188,11 +226,7 @@ async fn adbc_install_targets_an_explicit_version() { let target = "v2.0.0"; MockBinary::create(&temp, target).expect("install target version binaries"); - let tarball = make_targz(&[ - (LIB, b"ELF"), - ("LICENSE.txt", b"license"), - ("NOTICE.txt", b"notice"), - ]); + let tarball = driver_tarball(); let digest = format!("sha256:{:x}", Sha256::digest(&tarball)); let listener = tokio::net::TcpListener::bind("127.0.0.1:0") @@ -220,20 +254,47 @@ async fn adbc_install_targets_an_explicit_version() { .await .expect("install should succeed for an explicit version"); - // The driver lands under the requested version, not the active one. assert!( - temp.versions_dir() - .join(target) - .join("drivers") - .join("postgresql") - .join("manifest.toml") - .exists(), + temp.version_dir(target).join(INSTALLED_LIB).exists(), "driver installed under the requested version", ); assert!( - !temp.versions_dir().join("v1.0.0").join("drivers").exists(), - "the active version should be untouched", + driver_artifacts(&temp.version_dir("v1.0.0")).is_empty(), + "the active version holds no driver files", + ); +} + +#[test] +fn adbc_uninstall_removes_the_library_and_its_sidecars() { + let temp = TempInstallDir::new().expect("temp install dir"); + let version = "v1.0.0"; + fs::write(temp.current_version_file(), version).expect("write active version"); + MockBinary::create(&temp, version).expect("install version binaries"); + + let version_dir = temp.version_dir(version); + fs::write(version_dir.join(INSTALLED_LIB), b"ELF").expect("lib"); + fs::write( + version_dir.join("amp-adbc-driver-postgresql.LICENSE.txt"), + b"license", + ) + .expect("license"); + fs::write( + version_dir.join("amp-adbc-driver-postgresql.NOTICE.txt"), + b"notice", + ) + .expect("notice"); + + adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) + .expect("uninstall should succeed"); + + assert!( + driver_artifacts(&version_dir).is_empty(), + "library and sidecars all removed", ); + // Uninstalling a driver must not disturb the amp binaries it sat beside. + for binary in ["ampd", "ampctl", "ampsql"] { + assert!(version_dir.join(binary).exists(), "{binary} intact"); + } } #[test] @@ -242,41 +303,39 @@ fn adbc_uninstall_works_for_a_version_without_binaries() { let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); - // An orphaned driver directory: binaries are gone, drivers remain. Cleanup - // must still work, otherwise it could never be removed with ampup. - let driver_dir = temp - .versions_dir() - .join(version) - .join("drivers") - .join("postgresql"); - fs::create_dir_all(&driver_dir).expect("driver dir"); - fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + // An orphaned driver: binaries are gone, the library remains. Cleanup must + // still work, otherwise it could never be removed with ampup. + let version_dir = temp.version_dir(version); + fs::create_dir_all(&version_dir).expect("version dir"); + fs::write(version_dir.join(INSTALLED_LIB), b"ELF").expect("lib"); adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) .expect("uninstall should work without the version's binaries"); - assert!(!driver_dir.exists(), "orphaned driver directory removed"); + assert!(!version_dir.join(INSTALLED_LIB).exists(), "library removed"); } #[test] -fn adbc_uninstall_removes_installed_driver() { +fn adbc_uninstall_removes_a_library_built_for_another_platform() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); MockBinary::create(&temp, version).expect("install version binaries"); - let drivers_dir = temp.versions_dir().join(version).join("drivers"); - let driver_dir = drivers_dir.join("postgresql"); - fs::create_dir_all(&driver_dir).expect("driver dir"); - fs::write(driver_dir.join(LIB), b"ELF").expect("lib"); - fs::write(driver_dir.join("manifest.toml"), b"manifest_version = 1").expect("manifest"); + // `--platform ` leaves a library this host would not look for. + // Uninstall must still find it, or it could never be removed. + let host = crate::platform::Platform::detect().expect("supported host platform"); + let other = crate::platform::Platform::ALL + .iter() + .copied() + .find(|platform| *platform != host) + .expect("another supported platform exists"); + let version_dir = temp.version_dir(version); + let foreign = version_dir.join(Driver::Postgresql.installed_lib_filename(other)); + fs::write(&foreign, b"FOREIGN").expect("lib"); adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) - .expect("uninstall should succeed"); + .expect("uninstall should remove a foreign-platform library"); - assert!(!driver_dir.exists(), "driver directory removed"); - assert!( - !drivers_dir.exists(), - "emptied drivers directory pruned after removing the last driver", - ); + assert!(!foreign.exists(), "foreign-platform library removed"); } From 3d508b8f3f90a9b1d331b2281a67b1e231c42c96 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Mon, 27 Jul 2026 07:29:20 +0000 Subject: [PATCH 16/18] refactor(adbc): adopt pre-named archive library, skip license files The release archive now ships the driver library under its final name (amp-adbc-driver-.{so,dylib}), so ampup places it as-is instead of renaming it. LICENSE/NOTICE are validated in the archive but no longer extracted onto disk; redistribution is satisfied by the release tarball. --- ampup/src/adbc.rs | 62 +++----------- ampup/src/commands/adbc.rs | 168 +++++++------------------------------ ampup/src/tests/it_adbc.rs | 41 +++------ 3 files changed, 54 insertions(+), 217 deletions(-) diff --git a/ampup/src/adbc.rs b/ampup/src/adbc.rs index 96e0020..d34aafe 100644 --- a/ampup/src/adbc.rs +++ b/ampup/src/adbc.rs @@ -32,19 +32,11 @@ impl Driver { Self::ALL.iter().copied().find(|d| d.as_str() == name) } - /// The library filename this driver's release archive carries on - /// `platform` (e.g. `libadbc_driver_postgresql.so` on Linux) — upstream's - /// own name, which the archive is validated against. - pub fn archive_lib_filename(&self, platform: Platform) -> String { - format!( - "libadbc_driver_{}.{}", - self.as_str(), - lib_extension(platform) - ) - } - - /// The library filename this driver is installed under, prefixed so it is - /// distinguishable from the amp binaries sharing its directory. + /// The driver library's filename on `platform` (e.g. + /// `amp-adbc-driver-postgresql.so` on Linux). The release pipeline names + /// the library inside the archive this way (edgeandnode/amp #2599), so it + /// is both the archive member and the on-disk name; ampup places it as-is + /// beside the amp binaries it shares a directory with. pub fn installed_lib_filename(&self, platform: Platform) -> String { format!( "{DRIVER_FILE_PREFIX}{}.{}", @@ -52,12 +44,6 @@ impl Driver { lib_extension(platform) ) } - - /// The stem shared by this driver's installed files, used for the license - /// sidecars. Platform-independent, so cleanup needs no platform. - pub fn installed_stem(&self) -> String { - format!("{DRIVER_FILE_PREFIX}{}", self.as_str()) - } } /// The dynamic library extension for `platform`. @@ -111,47 +97,21 @@ mod tests { } #[test] - fn archive_lib_filename_is_the_upstream_platform_specific_name() { - assert_eq!( - Driver::Postgresql.archive_lib_filename(Platform::Linux), - "libadbc_driver_postgresql.so", - ); - assert_eq!( - Driver::Postgresql.archive_lib_filename(Platform::Darwin), - "libadbc_driver_postgresql.dylib", - ); - } - - #[test] - fn installed_lib_filename_is_prefixed_and_distinct_from_the_archive_name() { + fn installed_lib_filename_is_prefixed_and_platform_specific() { for platform in Platform::ALL.iter().copied() { let installed = Driver::Postgresql.installed_lib_filename(platform); assert!( installed.starts_with(DRIVER_FILE_PREFIX), - "installed name namespaces the driver: {installed}", - ); - assert_ne!( - installed, - Driver::Postgresql.archive_lib_filename(platform), - "the driver is renamed on install", + "installed name namespaces the driver against the amp binaries: {installed}", ); } + assert_eq!( + Driver::Postgresql.installed_lib_filename(Platform::Linux), + "amp-adbc-driver-postgresql.so", + ); assert_eq!( Driver::Postgresql.installed_lib_filename(Platform::Darwin), "amp-adbc-driver-postgresql.dylib", ); } - - #[test] - fn installed_stem_prefixes_every_installed_file() { - let stem = Driver::Postgresql.installed_stem(); - assert_eq!(stem, "amp-adbc-driver-postgresql"); - // The sidecars and the library share the stem, so cleanup by stem - // catches all of them without knowing the platform. - assert!( - Driver::Postgresql - .installed_lib_filename(Platform::Linux) - .starts_with(&stem) - ); - } } diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index ba80dc6..b01dc21 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -84,14 +84,18 @@ pub(crate) async fn install_driver( let staging = tempfile::tempdir_in(&version_dir).context("failed to create staging directory")?; - let archive_lib = driver.archive_lib_filename(platform); + // The archive ships the library under its final name plus the Apache-2.0 + // LICENSE/NOTICE files. All three are required for the archive to be + // well-formed, but only the library is placed on disk; the license files + // stay in staging and are discarded with it. + let installed_lib_name = driver.installed_lib_filename(platform); archive::extract_and_validate( &data, staging.path(), - &[archive_lib.as_str(), "LICENSE.txt", "NOTICE.txt"], + &[installed_lib_name.as_str(), "LICENSE.txt", "NOTICE.txt"], )?; - let installed_lib = place_driver(staging.path(), &version_dir, driver, platform)?; + let installed_lib = place_driver(staging.path(), &version_dir, &installed_lib_name)?; ui::info!( "Installed {} driver for amp {} at {}", @@ -102,47 +106,17 @@ pub(crate) async fn install_driver( Ok(()) } -/// Move the extracted files from `staging` into `version_dir`, renaming the -/// library to its installed (prefixed) name and namespacing the license -/// sidecars alongside it. Returns the installed library path. +/// Move the extracted driver library from `staging` into `version_dir` and +/// return its installed path. /// -/// Each move is a rename within one filesystem, which replaces any existing +/// The move is a rename within one filesystem, which replaces any existing /// file of that name — so a reinstall overwrites in place. Renaming over a /// library `ampd` currently has loaded is safe: the open inode survives. -/// -/// A failed move rolls back the ones that already landed. Without that, a -/// partial install would leave the library behind and `list` would report a -/// driver whose installation reported an error. -fn place_driver( - staging: &Path, - version_dir: &Path, - driver: Driver, - platform: Platform, -) -> Result { - let stem = driver.installed_stem(); - let moves = [ - ( - driver.archive_lib_filename(platform), - driver.installed_lib_filename(platform), - ), - ("LICENSE.txt".to_string(), format!("{stem}.LICENSE.txt")), - ("NOTICE.txt".to_string(), format!("{stem}.NOTICE.txt")), - ]; - - let mut placed: Vec = Vec::with_capacity(moves.len()); - for (from, to) in &moves { - let destination = version_dir.join(to); - if let Err(error) = fs::rename(staging.join(from), &destination) { - for path in &placed { - let _ = fs::remove_file(path); - } - return Err(anyhow::Error::new(error)) - .with_context(|| format!("failed to move {from} into place")); - } - placed.push(destination); - } - - Ok(version_dir.join(driver.installed_lib_filename(platform))) +fn place_driver(staging: &Path, version_dir: &Path, lib_name: &str) -> Result { + let destination = version_dir.join(lib_name); + fs::rename(staging.join(lib_name), &destination) + .with_context(|| format!("failed to move {lib_name} into place"))?; + Ok(destination) } /// List installed ADBC drivers for an amp version (the active one by default). @@ -200,14 +174,6 @@ pub fn uninstall( ); } - // The library plus its license sidecars all share the driver's stem. - let stem = driver.installed_stem(); - for name in [format!("{stem}.LICENSE.txt"), format!("{stem}.NOTICE.txt")] { - let sidecar = version_dir.join(name); - if sidecar.exists() { - fs::remove_file(&sidecar).context("failed to remove a driver license file")?; - } - } for lib in installed { fs::remove_file(&lib).context("failed to remove the driver library")?; } @@ -266,10 +232,10 @@ fn installed_lib_paths(version_dir: &Path, driver: Driver) -> Vec { /// The catalog drivers currently installed in `version_dir`. /// -/// The directory also holds the amp binaries and each driver's license -/// sidecars, so membership is decided by looking up the catalog's expected -/// filenames rather than by matching names found there. A missing directory -/// yields no drivers, since the per-file checks simply do not match. +/// The directory also holds the amp binaries, so membership is decided by +/// looking up the catalog's expected library filenames rather than by matching +/// names found there. A missing directory yields no drivers, since the +/// per-file checks simply do not match. fn installed_drivers(version_dir: &Path) -> Vec { let mut drivers: Vec = Driver::ALL .iter() @@ -321,13 +287,14 @@ fn resolve_arch(over: Option) -> Result { mod tests { use super::*; - const ARCHIVE_LIB: &str = "libadbc_driver_postgresql.so"; const INSTALLED_LIB: &str = "amp-adbc-driver-postgresql.so"; - /// A staging directory holding the three files a driver archive carries. + /// A staging directory as it looks after extraction: the library under its + /// final name, plus the license files that stay behind (only the library + /// is placed). fn staged_driver(parent: &Path, lib_contents: &[u8]) -> PathBuf { let staging = tempfile::tempdir_in(parent).expect("staging dir"); - fs::write(staging.path().join(ARCHIVE_LIB), lib_contents).expect("write lib"); + fs::write(staging.path().join(INSTALLED_LIB), lib_contents).expect("write lib"); fs::write(staging.path().join("LICENSE.txt"), b"license").expect("write license"); fs::write(staging.path().join("NOTICE.txt"), b"notice").expect("write notice"); staging.keep() @@ -343,35 +310,20 @@ mod tests { } #[test] - fn place_driver_renames_the_library_and_namespaces_the_sidecars() { + fn place_driver_moves_only_the_library() { let version_dir = version_dir_with_binaries(); let staging = staged_driver(version_dir.path(), b"ELF"); - let placed = place_driver( - &staging, - version_dir.path(), - Driver::Postgresql, - Platform::Linux, - ) - .expect("place"); + let placed = place_driver(&staging, version_dir.path(), INSTALLED_LIB).expect("place"); assert_eq!(placed, version_dir.path().join(INSTALLED_LIB)); assert_eq!(fs::read(&placed).expect("lib"), b"ELF"); + // The license files are not placed on disk; they stay in staging. assert!( - version_dir - .path() - .join("amp-adbc-driver-postgresql.LICENSE.txt") - .exists(), - "license is namespaced so a second driver cannot clobber it", + !version_dir.path().join("LICENSE.txt").exists(), + "the license file is not placed beside the driver", ); - assert!( - version_dir - .path() - .join("amp-adbc-driver-postgresql.NOTICE.txt") - .exists() - ); - // The upstream name must not survive alongside the renamed library. - assert!(!version_dir.path().join(ARCHIVE_LIB).exists()); + assert!(!version_dir.path().join("NOTICE.txt").exists()); // The amp binaries are untouched. for binary in ["ampd", "ampctl", "ampsql"] { assert!(version_dir.path().join(binary).exists(), "{binary} intact"); @@ -383,22 +335,10 @@ mod tests { let version_dir = version_dir_with_binaries(); let first = staged_driver(version_dir.path(), b"old"); - place_driver( - &first, - version_dir.path(), - Driver::Postgresql, - Platform::Linux, - ) - .expect("first"); + place_driver(&first, version_dir.path(), INSTALLED_LIB).expect("first"); let second = staged_driver(version_dir.path(), b"new"); - place_driver( - &second, - version_dir.path(), - Driver::Postgresql, - Platform::Linux, - ) - .expect("second"); + place_driver(&second, version_dir.path(), INSTALLED_LIB).expect("second"); assert_eq!( fs::read(version_dir.path().join(INSTALLED_LIB)).expect("lib"), @@ -407,26 +347,13 @@ mod tests { } #[test] - fn installed_drivers_ignores_the_amp_binaries_and_sidecars() { + fn installed_drivers_ignores_the_amp_binaries() { let version_dir = version_dir_with_binaries(); assert!( installed_drivers(version_dir.path()).is_empty(), "the amp binaries are not drivers", ); - // A license sidecar alone must not register as an installed driver. - fs::write( - version_dir - .path() - .join("amp-adbc-driver-postgresql.LICENSE.txt"), - b"license", - ) - .expect("sidecar"); - assert!( - installed_drivers(version_dir.path()).is_empty(), - "a sidecar without the library is not an install", - ); - fs::write(version_dir.path().join(INSTALLED_LIB), b"ELF").expect("lib"); assert_eq!( installed_drivers(version_dir.path()), @@ -473,35 +400,4 @@ mod tests { assert!(installed_drivers(&missing).is_empty()); } - - #[test] - fn place_driver_rolls_back_when_a_later_move_fails() { - let version_dir = version_dir_with_binaries(); - let staging = staged_driver(version_dir.path(), b"ELF"); - // A directory where a sidecar must land makes that rename fail, after - // the library has already been moved. - fs::create_dir( - version_dir - .path() - .join("amp-adbc-driver-postgresql.LICENSE.txt"), - ) - .expect("blocking directory"); - - let result = place_driver( - &staging, - version_dir.path(), - Driver::Postgresql, - Platform::Linux, - ); - - assert!(result.is_err(), "a blocked sidecar move fails the install"); - assert!( - !version_dir.path().join(INSTALLED_LIB).exists(), - "the library is rolled back, so list cannot report a failed install", - ); - assert!( - installed_drivers(version_dir.path()).is_empty(), - "no driver is reported after a failed install", - ); - } } diff --git a/ampup/src/tests/it_adbc.rs b/ampup/src/tests/it_adbc.rs index a2cd1d0..92f6192 100644 --- a/ampup/src/tests/it_adbc.rs +++ b/ampup/src/tests/it_adbc.rs @@ -22,9 +22,8 @@ use crate::{ }, }; -/// The library name inside the release archive (upstream's). -const ARCHIVE_LIB: &str = "libadbc_driver_postgresql.so"; -/// The name it is installed under. +/// The library name inside the release archive, already its final on-disk name +/// (the release pipeline names it, edgeandnode/amp #2599). const INSTALLED_LIB: &str = "amp-adbc-driver-postgresql.so"; const ASSET: &str = "adbc-driver-postgresql-linux-x86_64.tar.gz"; @@ -47,10 +46,11 @@ fn make_targz(files: &[(&str, &[u8])]) -> Vec { .expect("should finish gzip") } -/// The driver archive as the release ships it. +/// The driver archive as the release ships it: the library under its final +/// name, plus the Apache-2.0 license files at the root. fn driver_tarball() -> Vec { make_targz(&[ - (ARCHIVE_LIB, b"ELF"), + (INSTALLED_LIB, b"ELF"), ("LICENSE.txt", b"license"), ("NOTICE.txt", b"notice"), ]) @@ -115,21 +115,12 @@ async fn adbc_install_places_driver_in_the_version_dir() { fs::read(version_dir.join(INSTALLED_LIB)).expect("lib"), b"ELF", ); + // The license files ship in the archive but are not placed on disk. assert!( - !version_dir.join(ARCHIVE_LIB).exists(), - "the upstream name must not survive alongside the renamed library", - ); - assert!( - version_dir - .join("amp-adbc-driver-postgresql.LICENSE.txt") - .exists(), - "license sidecar is namespaced", - ); - assert!( - version_dir - .join("amp-adbc-driver-postgresql.NOTICE.txt") - .exists(), + !version_dir.join("LICENSE.txt").exists(), + "the license file is not extracted onto disk", ); + assert!(!version_dir.join("NOTICE.txt").exists()); // The amp binaries it now shares a directory with are untouched. for binary in ["ampd", "ampctl", "ampsql"] { @@ -265,7 +256,7 @@ async fn adbc_install_targets_an_explicit_version() { } #[test] -fn adbc_uninstall_removes_the_library_and_its_sidecars() { +fn adbc_uninstall_removes_the_library() { let temp = TempInstallDir::new().expect("temp install dir"); let version = "v1.0.0"; fs::write(temp.current_version_file(), version).expect("write active version"); @@ -273,23 +264,13 @@ fn adbc_uninstall_removes_the_library_and_its_sidecars() { let version_dir = temp.version_dir(version); fs::write(version_dir.join(INSTALLED_LIB), b"ELF").expect("lib"); - fs::write( - version_dir.join("amp-adbc-driver-postgresql.LICENSE.txt"), - b"license", - ) - .expect("license"); - fs::write( - version_dir.join("amp-adbc-driver-postgresql.NOTICE.txt"), - b"notice", - ) - .expect("notice"); adbc::uninstall(Some(temp.path().to_path_buf()), "postgresql", None) .expect("uninstall should succeed"); assert!( driver_artifacts(&version_dir).is_empty(), - "library and sidecars all removed", + "the driver library is removed", ); // Uninstalling a driver must not disturb the amp binaries it sat beside. for binary in ["ampd", "ampctl", "ampsql"] { From be148c9f5a90fab2aa5e8b59d167fcb03c8f6189 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Mon, 27 Jul 2026 07:52:55 +0000 Subject: [PATCH 17/18] docs(adbc): document ampup adbc install/list/uninstall Add ADBC driver usage to the README and the ampup feature doc: the subcommands, per-version behavior, the version directory layout, and the install flow. Uses the generic amp-adbc-driver- filename form. --- README.md | 25 +++++++++++++++++++++++-- docs/features/app-ampup.md | 37 ++++++++++++++++++++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index f08d382..b7b9058 100644 --- a/README.md +++ b/README.md @@ -134,6 +134,26 @@ ampup build --branch main --jobs 8 ampup update ``` +### ADBC Drivers + +Some amp features need an optional ADBC driver library. `ampup adbc` installs these alongside the amp binaries for a version. + +```sh +# Install a driver for the active version +ampup adbc install postgresql + +# Install for a specific version +ampup adbc install postgresql --version v0.1.0 + +# List installed drivers +ampup adbc list + +# Uninstall a driver +ampup adbc uninstall postgresql +``` + +The driver is downloaded from the matching amp release, verified against its published checksum, and placed in the version directory. `--platform` and `--arch` override detection, the same as `install`. + ## How It Works `ampup` is a Rust-based version manager with a minimal bootstrap script for installation. @@ -152,9 +172,10 @@ ampup update │ └── ampd # Symlink to active version ├── versions/ │ ├── v0.1.0/ -│ │ └── ampd # Binary for v0.1.0 +│ │ ├── ampd # Binary for v0.1.0 +│ │ └── amp-adbc-driver-.so # Optional ADBC driver (when installed) │ └── v0.2.0/ -│ └── ampd # Binary for v0.2.0 +│ └── ampd # Binary for v0.2.0 └── .version # Tracks active version ``` diff --git a/docs/features/app-ampup.md b/docs/features/app-ampup.md index 472b440..b188df2 100644 --- a/docs/features/app-ampup.md +++ b/docs/features/app-ampup.md @@ -26,6 +26,7 @@ ampup is the official version manager and installer for Amp binaries (`ampd`, pl - **Builder**: Compiles ampd/ampctl/ampsql from source using cargo, supporting branch, commit, PR, or local path builds - **Self-updater**: Atomic in-place binary replacement for updating ampup itself to the latest version - **Active Version**: The currently selected version, tracked via symlinks in `~/.amp/bin/` and `.version` file +- **ADBC Drivers**: Optional destination-specific driver libraries, installed from the release into a version directory beside the amp binaries and managed per version ## Usage @@ -138,6 +139,29 @@ ampup self version The self-update performs atomic in-place replacement of the running executable. +### ADBC Drivers + +Optional ADBC driver libraries are installed per amp version, alongside the binaries in that version's directory. + +```bash +# Install a driver for the active version +ampup adbc install postgresql + +# Install for a specific amp version +ampup adbc install postgresql --version v0.1.0 + +# Override platform/arch detection +ampup adbc install postgresql --platform linux --arch aarch64 + +# List installed drivers for the active version +ampup adbc list + +# Uninstall a driver +ampup adbc uninstall postgresql +``` + +Each command defaults to the active amp version and accepts `--version` to target another. `postgresql` is the currently supported driver. Installing requires the target version's binaries to be present, so a driver cannot be stranded under a version that a later `ampup install` would replace. + ## Architecture ### Directory Structure @@ -153,7 +177,8 @@ The self-update performs atomic in-place replacement of the running executable. │ ├── v0.1.0/ │ │ ├── ampd │ │ ├── ampctl -│ │ └── ampsql +│ │ ├── ampsql +│ │ └── amp-adbc-driver-.so # Optional ADBC driver (.dylib on macOS) │ ├── v0.2.0/ │ │ ├── ampd │ │ ├── ampctl @@ -196,6 +221,16 @@ The self-update performs atomic in-place replacement of the running executable. 5. Copy `target/release/ampd` (required) plus `ampctl`/`ampsql` when produced to `~/.amp/versions//`; when an optional binary is not produced, any stale copy from a previous build of the same version is removed so activation cannot symlink an outdated binary 6. Activate version (create symlinks) +### ADBC Driver Install Flow + +1. User runs `ampup adbc install [--version ]` +2. Resolve the amp version (explicit `--version` or the active one) and require its binaries to be installed +3. Detect platform (Linux/Darwin) and architecture (x86_64/aarch64), or take `--platform`/`--arch` +4. Query the GitHub API for the version's release and resolve the `adbc-driver-{driver}-{platform}-{arch}.tar.gz` asset +5. Download the archive and verify its SHA-256 against the asset's published digest (an asset without a digest is rejected) +6. Extract into a staging directory inside the version directory (same filesystem, so the move is atomic) +7. Move the driver library into the version directory beside the amp binaries; the archive's license files are validated in staging but not placed beside the binaries + ### Communication ``` From dd2101ec4e58e561cb4bbc0188040e15f8157502 Mon Sep 17 00:00:00 2001 From: Krishnanand V P Date: Mon, 27 Jul 2026 08:00:40 +0000 Subject: [PATCH 18/18] refactor(adbc): check the asset digest before downloading The digest is already in the release metadata, so a missing one can be caught before spending the download instead of after. --- ampup/src/commands/adbc.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/ampup/src/commands/adbc.rs b/ampup/src/commands/adbc.rs index b01dc21..4618815 100644 --- a/ampup/src/commands/adbc.rs +++ b/ampup/src/commands/adbc.rs @@ -65,16 +65,18 @@ pub(crate) async fn install_driver( .resolve(&asset_name, false)? .expect("a required asset resolves to Some or errors"); - let data = download_with_retry(github, &asset).await?; // Driver assets always advertise a digest, so a missing one means the - // release is malformed. Refuse rather than install a library that would be - // loaded into ampd without its integrity checked. + // release is malformed. The digest is already in the release metadata, so + // check it before spending the download rather than after. Refuse rather + // than install a library that would be loaded into ampd unverified. let digest = asset.digest.as_deref().ok_or_else(|| { anyhow!( "release asset {} has no digest; refusing to install an unverified driver", asset.name ) })?; + + let data = download_with_retry(github, &asset).await?; verify_artifact(&asset.name, &data, Some(digest)) .context("driver archive failed verification")?;