diff --git a/docs/source/kernel-requirements.md b/docs/source/kernel-requirements.md index eb1667ec..15fe43bf 100644 --- a/docs/source/kernel-requirements.md +++ b/docs/source/kernel-requirements.md @@ -84,13 +84,16 @@ metadata. Currently the following top-level keys are supported: - `provenance` (`dict`, optional): provenance of the build, used to flag non-reproducible (dirty) builds. It contains two optional sub-objects: - `kernel-builder`: the `kernel-builder` that produced the build, with its - `version` (`str`), the `sha` (`str`) of the `kernel-builder` source it was - built from (when known), and a `dirty` (`bool`) flag that is `true` when + `version` (`str`), the `commit` (`str`) of the `kernel-builder` source it + was built from (when known), and a `dirty` (`bool`) flag that is `true` when `kernel-builder` was built from a source tree with uncommitted changes. - - `kernel`: the kernel source that was built, with its commit `sha` (`str`) + - `kernel`: the kernel source that was built, with its `commit` (`str`) and a `dirty` (`bool`) flag that is `true` when the kernel source had uncommitted changes. + A `commit` is the object identifier of the Git commit: the hexadecimal + encoding of its SHA-1 or SHA-256 hash. + When either `dirty` flag is set, the kernel was built from uncommitted sources and cannot be reliably reproduced. @@ -100,10 +103,10 @@ metadata. Currently the following top-level keys are supported: > builds are not flagged as dirty. Local `create-pyproject` runs only > consider changes to tracked files. - > **Note:** The kernel `sha`/`dirty` are captured at the moment - > `create-pyproject` runs, so they describe the source tree as it was *then*. + > **Note:** The kernel `commit`/`dirty` are captured at the moment + > `create-pyproject` runs, so they describe the source tree as it was _then_. > Running `create-pyproject` and committing afterwards is bad practice: the - > recorded provenance keeps pointing at the pre-commit state (a stale `sha`, + > recorded provenance keeps pointing at the pre-commit state (a stale `commit`, > and `dirty: true` if the tree was dirty) even though the committed source > differs. Generate the metadata from the final, committed source instead. diff --git a/kernel-builder/src/main.rs b/kernel-builder/src/main.rs index 52547334..73a8380a 100644 --- a/kernel-builder/src/main.rs +++ b/kernel-builder/src/main.rs @@ -2,13 +2,14 @@ use std::fs::File; use std::io::{BufWriter, Write}; use std::path::PathBuf; -mod card; -mod check_abi; -use check_abi::{run_check_abi, CheckAbiArgs}; - use clap::{Args, CommandFactory, Parser, Subcommand}; use clap_complete::Shell; use eyre::{Context, Result}; +use kernels_data::git::Oid; + +mod card; +mod check_abi; +use check_abi::{run_check_abi, CheckAbiArgs}; mod completions; use completions::print_completions; @@ -195,7 +196,7 @@ enum Commands { /// metadata. When absent, it is detected from the kernel's git /// repository (used by Nix builds where the source has no `.git`). #[arg(long)] - kernel_sha: Option, + kernel_sha: Option, /// Mark the kernel source as having uncommitted changes in the build /// metadata. Only meaningful together with `--kernel-sha`. diff --git a/kernel-builder/src/pyproject/common.rs b/kernel-builder/src/pyproject/common.rs index 5fbdf3e5..c6ea5f7d 100644 --- a/kernel-builder/src/pyproject/common.rs +++ b/kernel-builder/src/pyproject/common.rs @@ -1,10 +1,12 @@ use std::path::PathBuf; +use std::str::FromStr; use eyre::Result; use itertools::Itertools; use kernels_data::config::{Backend, Build}; -use kernels_data::metadata::{GitHash, KernelBuilderVersion, Metadata, Provenance}; +use kernels_data::git::{GitStatus, Oid}; +use kernels_data::metadata::{KernelBuilderVersion, Metadata, Provenance}; use crate::pyproject::ops_identifier::KernelIdentifier; use crate::pyproject::FileSet; @@ -17,10 +19,12 @@ static ADD_BUILD_METADATA_PY: &str = include_str!("templates/torch/add_build_met /// build sandboxes without a `.git` (e.g. Nix), the derivation supplies it via /// `built`'s `BUILT_OVERRIDE_hf_kernel_builder_GIT_*` environment variables. pub(crate) fn kernel_builder_version() -> KernelBuilderVersion { - let git = crate::built_info::GIT_COMMIT_HASH.map(|sha| GitHash { - sha: sha.to_owned(), - dirty: crate::built_info::GIT_DIRTY.unwrap_or(false), - }); + let git = crate::built_info::GIT_COMMIT_HASH + .and_then(|sha| Oid::from_str(sha).ok()) + .map(|commit| GitStatus { + commit, + dirty: crate::built_info::GIT_DIRTY.unwrap_or(false), + }); KernelBuilderVersion { version: env!("CARGO_PKG_VERSION").to_owned(), git, diff --git a/kernel-builder/src/pyproject/mod.rs b/kernel-builder/src/pyproject/mod.rs index 1c7d5376..fb911377 100644 --- a/kernel-builder/src/pyproject/mod.rs +++ b/kernel-builder/src/pyproject/mod.rs @@ -6,7 +6,8 @@ use std::{ use eyre::{bail, Result}; use kernels_data::config::{Build, Framework}; -use kernels_data::metadata::{GitHash, Provenance}; +use kernels_data::git::{GitStatus, Oid}; +use kernels_data::metadata::Provenance; use minijinja::Environment; use crate::{ @@ -49,7 +50,7 @@ pub fn create_pyproject( target_dir: Option, force: bool, unique_id: Option, - kernel_sha: Option, + kernel_sha: Option, kernel_dirty: bool, ) -> Result<()> { let kernel_dir = check_or_infer_kernel_dir(kernel_dir)?; @@ -62,11 +63,11 @@ pub fn create_pyproject( // `kernel-builder` provenance is always the one baked into this binary at // compile time. let kernel = kernel_sha - .map(|sha| GitHash { - sha, + .map(|commit| GitStatus { + commit, dirty: kernel_dirty, }) - .or_else(|| ops_identifier::git_hash(&kernel_dir)); + .or_else(|| ops_identifier::git_status(&kernel_dir)); let provenance = Provenance { kernel_builder: common::kernel_builder_version(), kernel, diff --git a/kernel-builder/src/pyproject/ops_identifier.rs b/kernel-builder/src/pyproject/ops_identifier.rs index dae82b68..80418e2b 100644 --- a/kernel-builder/src/pyproject/ops_identifier.rs +++ b/kernel-builder/src/pyproject/ops_identifier.rs @@ -1,9 +1,10 @@ use std::path::Path; +use std::str::FromStr; use eyre::{Result, WrapErr}; use git2::Repository; use kernels_data::config::Backend; -use kernels_data::metadata::GitHash; +use kernels_data::git::{GitStatus, Oid}; use rand::Rng; pub fn random_identifier() -> String { @@ -29,18 +30,21 @@ pub fn git_identifier(target_dir: impl AsRef) -> Result { Ok(if dirty { format!("{rev}_dirty") } else { rev }) } -pub fn git_hash(target_dir: impl AsRef) -> Option { +pub fn git_status(target_dir: impl AsRef) -> Option { let repo = Repository::discover(target_dir.as_ref()).ok()?; let head = repo.head().ok()?; let commit = head.peel_to_commit().ok()?; - let sha = commit.id().to_string(); + let commit_oid = Oid::from_str(&commit.id().to_string()).ok()?; let mut status_options = git2::StatusOptions::new(); status_options.include_untracked(false); // Ignore untracked files (like generated CMake files) status_options.exclude_submodules(true); let dirty = !repo.statuses(Some(&mut status_options)).ok()?.is_empty(); - Some(GitHash { sha, dirty }) + Some(GitStatus { + commit: commit_oid, + dirty, + }) } /// Uniquely identifies a kernel for the purpose of ops-name generation. @@ -98,8 +102,8 @@ mod tests { } #[test] - fn git_hash_is_none_in_non_git_dir() { + fn git_status_is_none_in_non_git_dir() { let tmp = tempfile::tempdir().unwrap(); - assert!(git_hash(tmp.path()).is_none()); + assert!(git_status(tmp.path()).is_none()); } } diff --git a/kernel-builder/src/upload.rs b/kernel-builder/src/upload.rs index 2d98592f..c59b0ef7 100644 --- a/kernel-builder/src/upload.rs +++ b/kernel-builder/src/upload.rs @@ -934,7 +934,7 @@ mod tests { const METADATA_V3: &str = r#"{"name": "test-kernel", "id": "kernel_id", "version": 3, "license": "Apache-2.0", "python-depends": [], "backend": {"type": "cuda"}}"#; const METADATA_V0: &str = r#"{"name": "test-kernel", "id": "kernel_id", "version": 0, "license": "Apache-2.0", "python-depends": [], "backend": {"type": "cuda"}}"#; - const METADATA_DIRTY: &str = r#"{"name": "test-kernel", "id": "kernel_id", "version": 1, "license": "Apache-2.0", "python-depends": [], "backend": {"type": "cuda"}, "provenance": {"kernel-builder": {"version": "0.1.0", "sha": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "dirty": false}, "kernel": {"sha": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "dirty": true}}}"#; + const METADATA_DIRTY: &str = r#"{"name": "test-kernel", "id": "kernel_id", "version": 1, "license": "Apache-2.0", "python-depends": [], "backend": {"type": "cuda"}, "provenance": {"kernel-builder": {"version": "0.1.0", "commit": "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", "dirty": false}, "kernel": {"commit": "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", "dirty": true}}}"#; #[test] fn test_dirty_variant_names() { diff --git a/kernels-data/bindings/python/kernels_data.pyi b/kernels-data/bindings/python/kernels_data.pyi index 1a23f240..a294f891 100644 --- a/kernels-data/bindings/python/kernels_data.pyi +++ b/kernels-data/bindings/python/kernels_data.pyi @@ -9,7 +9,7 @@ __all__ = [ "BackendInfo", "Provenance", "DigestAlgorithm", - "GitHash", + "GitStatus", "KernelBuilderVersion", "KernelDependency", "KernelName", @@ -70,12 +70,12 @@ class BackendInfo: def __repr__(self) -> str: ... @final -class GitHash: - """Git provenance (commit SHA and dirty state) of a source tree.""" +class GitStatus: + """The state of a git working tree.""" @property - def sha(self) -> str: - """Full 40-character commit SHA.""" + def commit(self) -> str: + """Identifier of the `HEAD` commit, as lowercase hexadecimal digits.""" ... @property @@ -95,8 +95,8 @@ class KernelBuilderVersion: ... @property - def git(self) -> Optional[GitHash]: - """Commit SHA and dirty state of the `kernel-builder` source, when known.""" + def git(self) -> Optional[GitStatus]: + """Git state of the `kernel-builder` source, when known.""" ... def __repr__(self) -> str: ... @@ -111,7 +111,7 @@ class Provenance: ... @property - def kernel(self) -> Optional[GitHash]: + def kernel(self) -> Optional[GitStatus]: """Git provenance of the kernel source that was built.""" ... diff --git a/kernels-data/bindings/python/src/lib.rs b/kernels-data/bindings/python/src/lib.rs index 67dc990b..32088ed4 100644 --- a/kernels-data/bindings/python/src/lib.rs +++ b/kernels-data/bindings/python/src/lib.rs @@ -6,7 +6,8 @@ use std::str::FromStr; use kernels_data::config::{Backend, KernelDependency, KernelName, KernelVersion}; use kernels_data::digest::{Digest, DigestAlgorithm, DigestViolation}; -use kernels_data::metadata::{BackendInfo, GitHash, KernelBuilderVersion, Metadata, Provenance}; +use kernels_data::git::{GitStatus, Oid}; +use kernels_data::metadata::{BackendInfo, KernelBuilderVersion, Metadata, Provenance}; use kernels_data::version::Version; use pyo3::Bound as PyBound; use pyo3::exceptions::{PyException, PyOSError, PyRuntimeError, PyValueError}; @@ -193,27 +194,27 @@ impl PyBackendInfo { } } -#[pyclass(name = "GitHash", frozen)] +#[pyclass(name = "GitStatus", frozen)] #[derive(Clone, Debug)] -struct PyGitHash { - sha: String, +struct PyGitStatus { + commit: Oid, dirty: bool, } -impl From for PyGitHash { - fn from(g: GitHash) -> Self { +impl From for PyGitStatus { + fn from(status: GitStatus) -> Self { Self { - sha: g.sha, - dirty: g.dirty, + commit: status.commit, + dirty: status.dirty, } } } #[pymethods] -impl PyGitHash { +impl PyGitStatus { #[getter] - fn sha(&self) -> &str { - &self.sha + fn commit(&self) -> &str { + self.commit.as_str() } #[getter] @@ -222,7 +223,11 @@ impl PyGitHash { } fn __repr__(&self) -> String { - format!("GitHash(sha={:?}, dirty={})", self.sha, self.dirty) + format!( + "GitStatus(commit={:?}, dirty={})", + self.commit.as_str(), + self.dirty + ) } } @@ -230,7 +235,7 @@ impl PyGitHash { #[derive(Clone, Debug)] struct PyKernelBuilderVersion { version: String, - git: Option, + git: Option, } impl From for PyKernelBuilderVersion { @@ -249,9 +254,9 @@ impl PyKernelBuilderVersion { &self.version } - /// Commit SHA + dirty state of the `kernel-builder` source, when known. + /// Git state of the `kernel-builder` source, when known. #[getter] - fn git(&self) -> Option { + fn git(&self) -> Option { self.git.clone() } @@ -270,7 +275,7 @@ impl PyKernelBuilderVersion { #[derive(Clone, Debug)] struct PyProvenance { kernel_builder: PyKernelBuilderVersion, - kernel: Option, + kernel: Option, } impl From for PyProvenance { @@ -290,7 +295,7 @@ impl PyProvenance { } #[getter] - fn kernel(&self) -> Option { + fn kernel(&self) -> Option { self.kernel.clone() } @@ -745,7 +750,7 @@ fn kernels_data_py(m: &PyBound<'_, PyModule>) -> PyResult<()> { m.add_class::()?; m.add_class::()?; m.add_class::()?; - m.add_class::()?; + m.add_class::()?; m.add_class::()?; m.add_class::()?; m.add_class::()?; diff --git a/kernels-data/src/git.rs b/kernels-data/src/git.rs new file mode 100644 index 00000000..e6521d39 --- /dev/null +++ b/kernels-data/src/git.rs @@ -0,0 +1,167 @@ +use std::fmt::Display; +use std::str::FromStr; + +use serde::{Deserialize, Deserializer, Serialize, Serializer, de}; +use thiserror::Error; + +/// A git object identifier. +/// +/// The identifier is stored in its canonical (lowercase hexadecimal) form and +/// is validated on construction: it must be a full SHA-1 (40 hexadecimal +/// digits) or SHA-256 (64 hexadecimal digits) identifier. Abbreviated +/// identifiers are rejected. +/// +/// The representation is intentionally opaque, use [`Oid::as_str`] or +/// [`Display`] to get at the identifier. +#[derive(Clone, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub struct Oid(String); + +impl Oid { + /// The identifier as a lowercase hexadecimal string. + pub fn as_str(&self) -> &str { + &self.0 + } +} + +/// Error parsing a git object identifier. +#[derive(Clone, Debug, Eq, Error, PartialEq)] +#[error("Invalid git object id, expected 40 or 64 hexadecimal digits: {0}")] +pub struct OidError(String); + +impl FromStr for Oid { + type Err = OidError; + + fn from_str(s: &str) -> Result { + let valid_length = matches!(s.len(), 40 | 64); + if !valid_length || !s.bytes().all(|b| b.is_ascii_hexdigit()) { + return Err(OidError(s.to_owned())); + } + Ok(Oid(s.to_ascii_lowercase())) + } +} + +impl AsRef for Oid { + fn as_ref(&self) -> &str { + self.as_str() + } +} + +impl Display for Oid { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.0) + } +} + +impl<'de> Deserialize<'de> for Oid { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Oid::from_str(&s).map_err(de::Error::custom) + } +} + +impl Serialize for Oid { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + serializer.serialize_str(&self.0) + } +} + +/// The state of a git working tree. +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +#[serde(rename_all = "kebab-case")] +pub struct GitStatus { + /// Identifier of the `HEAD` commit. + /// + /// `sha` is accepted as an alias when deserializing, since older metadata + /// used that name. + #[serde(alias = "sha")] + pub commit: Oid, + + /// Whether the working tree had uncommitted changes to tracked files. + pub dirty: bool, +} + +#[cfg(test)] +mod tests { + use std::str::FromStr; + + use super::{GitStatus, Oid}; + + const SHA1: &str = "d0610aa58db33b142c86b59598a2a1c730f52996"; + const SHA256: &str = "9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08"; + + #[test] + fn sha1_and_sha256_oids_are_accepted() { + assert_eq!(Oid::from_str(SHA1).unwrap().as_str(), SHA1); + assert_eq!(Oid::from_str(SHA256).unwrap().as_str(), SHA256); + } + + #[test] + fn oids_are_normalized_to_lowercase() { + let oid = Oid::from_str(&SHA1.to_ascii_uppercase()).unwrap(); + assert_eq!(oid.as_str(), SHA1); + assert_eq!(oid, Oid::from_str(SHA1).unwrap()); + } + + #[test] + fn invalid_oids_are_rejected() { + for invalid in [ + "", + "d0610aa", + &SHA1[..39], + &format!("{SHA1}0"), + &format!("{SHA256}0"), + &SHA1.replace('d', "z"), + &format!("{} ", &SHA1[..39]), + ] { + assert!( + Oid::from_str(invalid).is_err(), + "should be rejected: {invalid:?}" + ); + } + } + + #[test] + fn oid_displays_as_hex() { + assert_eq!(Oid::from_str(SHA1).unwrap().to_string(), SHA1); + } + + #[test] + fn oid_round_trips_through_json() { + let oid = Oid::from_str(SHA1).unwrap(); + let json = serde_json::to_string(&oid).unwrap(); + assert_eq!(json, format!("\"{SHA1}\"")); + assert_eq!(serde_json::from_str::(&json).unwrap(), oid); + } + + #[test] + fn invalid_oid_fails_to_deserialize() { + assert!(serde_json::from_str::("\"not-an-oid\"").is_err()); + assert!(serde_json::from_str::("42").is_err()); + } + + #[test] + fn git_status_round_trips_through_json() { + let status = GitStatus { + commit: Oid::from_str(SHA1).unwrap(), + dirty: true, + }; + + let json = serde_json::to_string(&status).unwrap(); + assert_eq!(json, format!(r#"{{"commit":"{SHA1}","dirty":true}}"#)); + assert_eq!(serde_json::from_str::(&json).unwrap(), status); + } + + #[test] + fn git_status_accepts_legacy_sha_field() { + let json = format!(r#"{{"sha":"{SHA1}","dirty":false}}"#); + let status: GitStatus = serde_json::from_str(&json).unwrap(); + assert_eq!(status.commit, Oid::from_str(SHA1).unwrap()); + assert!(!status.dirty); + } +} diff --git a/kernels-data/src/lib.rs b/kernels-data/src/lib.rs index d5863c5d..0ede6472 100644 --- a/kernels-data/src/lib.rs +++ b/kernels-data/src/lib.rs @@ -1,4 +1,5 @@ pub mod config; pub mod digest; +pub mod git; pub mod metadata; pub mod version; diff --git a/kernels-data/src/metadata.rs b/kernels-data/src/metadata.rs index 51fa93b9..29763599 100644 --- a/kernels-data/src/metadata.rs +++ b/kernels-data/src/metadata.rs @@ -5,6 +5,7 @@ use serde::{Deserialize, Serialize}; use crate::config::{Backend, Build, GitUrl, KernelDependency, KernelName}; use crate::digest::Digest; +use crate::git::GitStatus; #[derive(Debug, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] @@ -15,21 +16,17 @@ pub struct BackendInfo { pub archs: Option>, } -#[derive(Clone, Debug, Deserialize, Serialize)] -#[serde(rename_all = "kebab-case")] -pub struct GitHash { - pub sha: String, - pub dirty: bool, -} - /// Provenance of the `kernel-builder` that produced a build. #[derive(Clone, Debug, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] pub struct KernelBuilderVersion { pub version: String, - /// Commit SHA + dirty state of the `kernel-builder` source, when known. + /// Git state of the `kernel-builder` source, when known. + /// + /// Flattened so that the `kernel-builder` and kernel provenance have the + /// same shape in the serialized form. #[serde(flatten)] - pub git: Option, + pub git: Option, } /// Provenance of a kernel build: the git state of the `kernel-builder` and of @@ -43,7 +40,7 @@ pub struct Provenance { /// Git provenance of the kernel source. `None` when the kernel source was /// not built from a git repository (so its revision is unknown). #[serde(skip_serializing_if = "Option::is_none")] - pub kernel: Option, + pub kernel: Option, } impl Provenance { @@ -129,22 +126,35 @@ impl FromStr for Metadata { #[cfg(test)] mod tests { use std::collections::HashMap; + use std::str::FromStr; use crate::config::{Backend, Build, Framework, General, KernelName, TorchNoarch, TvmFfi}; + use crate::git::{GitStatus, Oid}; + + use super::{KernelBuilderVersion, Metadata, Provenance}; - use super::{GitHash, KernelBuilderVersion, Metadata, Provenance}; + /// During the development of kernels 0.17, we had a time period where + /// `sha` was used before `oid`. The Hub as a bunch of kernels with `sha` + /// in their metadata. For this reason, we have a compatibility alias. + /// This older metadata is used to validate that metadata with `sha` can + /// be parsed correctly. + const LEGACY_HUB_METADATA: &str = include_str!("../tests/data/legacy-sha-provenance.json"); + + fn oid(hex_digit: &str) -> Oid { + Oid::from_str(&hex_digit.repeat(40)).unwrap() + } fn sample_provenance(kernel_builder_dirty: bool, kernel_dirty: bool) -> Provenance { Provenance { kernel_builder: KernelBuilderVersion { version: "0.1.0".to_string(), - git: Some(GitHash { - sha: "a".repeat(40), + git: Some(GitStatus { + commit: oid("a"), dirty: kernel_builder_dirty, }), }, - kernel: Some(GitHash { - sha: "b".repeat(40), + kernel: Some(GitStatus { + commit: oid("b"), dirty: kernel_dirty, }), } @@ -284,38 +294,38 @@ mod tests { let parsed: Metadata = serde_json::from_str(&json).unwrap(); let provenance = parsed.provenance.expect("provenance should round-trip"); assert!(provenance.is_dirty()); - assert_eq!(provenance.kernel.unwrap().sha, "b".repeat(40)); + assert_eq!(provenance.kernel.unwrap().commit, oid("b")); let kernel_builder = provenance.kernel_builder; assert_eq!(kernel_builder.version, "0.1.0"); - // The embedded `GitHash` is flattened into the `kernel-builder` object. + // The embedded `GitStatus` is flattened into the `kernel-builder` object. let git = kernel_builder .git .expect("kernel-builder git should round-trip"); - assert_eq!(git.sha, "a".repeat(40)); + assert_eq!(git.commit, oid("a")); assert!(!git.dirty); } #[test] - fn kernel_builder_version_flattens_git_hash() { + fn kernel_builder_version_flattens_git_status() { let kernel_builder = KernelBuilderVersion { version: "0.1.0".to_string(), - git: Some(GitHash { - sha: "c".repeat(40), + git: Some(GitStatus { + commit: oid("c"), dirty: true, }), }; - // The `GitHash` fields are flattened into the same object as `version`. + // The `GitStatus` fields are flattened into the same object as `version`. let value: serde_json::Value = serde_json::to_value(&kernel_builder).unwrap(); assert_eq!(value["version"], "0.1.0"); - assert_eq!(value["sha"], "c".repeat(40)); + assert_eq!(value["commit"], "c".repeat(40)); assert_eq!(value["dirty"], true); assert!(value.get("git").is_none()); let parsed: KernelBuilderVersion = serde_json::from_value(value).unwrap(); let git = parsed.git.expect("git should round-trip"); - assert_eq!(git.sha, "c".repeat(40)); + assert_eq!(git.commit, oid("c")); assert!(git.dirty); } @@ -334,4 +344,115 @@ mod tests { let parsed: KernelBuilderVersion = serde_json::from_str(&json).unwrap(); assert!(parsed.git.is_none()); } + + #[test] + fn legacy_hub_metadata_still_parses() { + let metadata = Metadata::from_str(LEGACY_HUB_METADATA).unwrap(); + + // Provenance is read through the `sha` alias rather than discarded. + let provenance = metadata.provenance.expect("provenance should be read"); + assert!(!provenance.is_dirty()); + + let kernel_builder = provenance.kernel_builder; + assert_eq!(kernel_builder.version, "0.17.0-dev0"); + let git = kernel_builder + .git + .expect("kernel-builder git should be read"); + assert_eq!( + git.commit, + Oid::from_str("d0610aa58db33b142c86b59598a2a1c730f52996").unwrap() + ); + assert!(!git.dirty); + + let kernel = provenance.kernel.expect("kernel git should be read"); + assert_eq!( + kernel.commit, + Oid::from_str("d45addadb0c380f3d7cc2813310b3bcd75f8aa9a").unwrap() + ); + assert!(!kernel.dirty); + + // The rest of the metadata is unaffected. + assert_eq!(metadata.name.as_ref(), "einops"); + assert_eq!(metadata.id, "_einops_cpu_d45adda"); + assert_eq!(metadata.version, 1); + assert_eq!(metadata.license, "MIT"); + assert!(metadata.upstream.is_some()); + assert!(metadata.source.is_none()); + assert!(metadata.python_depends.is_empty()); + assert!(metadata.kernel_depends.is_empty()); + assert_eq!(metadata.backend.backend_type, Backend::Cpu); + assert!(metadata.digest.is_some()); + } + + #[test] + fn legacy_hub_metadata_is_reserialized_with_commit() { + let metadata = Metadata::from_str(LEGACY_HUB_METADATA).unwrap(); + let json = serde_json::to_string(&metadata).unwrap(); + + assert!(json.contains(r#""commit":"d0610aa58db33b142c86b59598a2a1c730f52996""#)); + assert!(json.contains(r#""commit":"d45addadb0c380f3d7cc2813310b3bcd75f8aa9a""#)); + assert!(!json.contains("\"sha\"")); + } + + #[test] + fn metadata_from_newer_writer_ignores_unknown_fields() { + // Simulate metadata written by a newer `kernel-builder` that added + // fields we do not know about. + let mut value: serde_json::Value = serde_json::from_str(LEGACY_HUB_METADATA).unwrap(); + value["future-field"] = serde_json::json!("surprise"); + value["provenance"]["future-field"] = serde_json::json!({"nested": [1, 2, 3]}); + value["provenance"]["kernel"]["future-field"] = serde_json::json!(true); + + let metadata: Metadata = serde_json::from_value(value).unwrap(); + let provenance = metadata.provenance.expect("provenance should be read"); + assert_eq!( + provenance.kernel.unwrap().commit, + Oid::from_str("d45addadb0c380f3d7cc2813310b3bcd75f8aa9a").unwrap() + ); + } + + #[test] + fn unreadable_provenance_is_an_error() { + for provenance in [ + serde_json::json!(42), + serde_json::json!({}), + serde_json::json!({"kernel-builder": {"version": "0.1.0"}, "kernel": "nonsense"}), + serde_json::json!({ + "kernel-builder": {"version": "0.1.0"}, + "kernel": {"commit": "not-an-oid", "dirty": false}, + }), + ] { + let mut value: serde_json::Value = serde_json::from_str(LEGACY_HUB_METADATA).unwrap(); + value["provenance"] = provenance.clone(); + + assert!( + serde_json::from_value::(value).is_err(), + "should be rejected: {provenance}" + ); + } + } + + #[test] + fn unreadable_kernel_builder_git_is_dropped() { + // `KernelBuilderVersion::git` is flattened, and serde deserializes a + // flattened `Option` by discarding the error rather than propagating + // it. So an unreadable `kernel-builder` git status drops that status + // instead of rejecting the metadata. + let mut value: serde_json::Value = serde_json::from_str(LEGACY_HUB_METADATA).unwrap(); + value["provenance"]["kernel-builder"]["sha"] = serde_json::json!("not-an-oid"); + + let metadata: Metadata = serde_json::from_value(value).unwrap(); + let provenance = metadata.provenance.expect("provenance should be read"); + assert!(provenance.kernel_builder.git.is_none()); + assert_eq!(provenance.kernel_builder.version, "0.17.0-dev0"); + assert!(provenance.kernel.is_some()); + } + + #[test] + fn unreadable_digest_is_an_error() { + let mut value: serde_json::Value = serde_json::from_str(LEGACY_HUB_METADATA).unwrap(); + value["digest"]["algorithm"] = serde_json::json!("sha3-512"); + + assert!(serde_json::from_value::(value).is_err()); + } } diff --git a/kernels-data/tests/data/legacy-sha-provenance.json b/kernels-data/tests/data/legacy-sha-provenance.json new file mode 100644 index 00000000..8c6865c4 --- /dev/null +++ b/kernels-data/tests/data/legacy-sha-provenance.json @@ -0,0 +1,45 @@ +{ + "name": "einops", + "id": "_einops_cpu_d45adda", + "version": 1, + "license": "MIT", + "upstream": "https://github.com/arogozhnikov/einops.git", + "python-depends": [], + "backend": { + "type": "cpu" + }, + "digest": { + "algorithm": "sha256", + "files": { + "__init__.py": "Zi5UF5cdv0EjT90oh0zsv0Jfi4QbTrfZnE0Tv5on8F8=", + "_backends.py": "7BAiKWdBLLvAiryfhwDbwxglGDW9Dh5FNMT1ERSuzxk=", + "_ops.py": "11R2mMPUK6wLQIwJo8geCYiUgC/WM4OCjMiT15zK3hQ=", + "_torch_specific.py": "CxpGcgqlBn4BH+Qkpmr3dFBPTid1Rz/R0f9TGb36Bsw=", + "array_api.py": "jOb8RhwLS9wob/Y/e/KrnBR6ihQPoB2Ly0tfrHr+/Zk=", + "einops.py": "sXvD8SWFqufziyQJKRPmfAGHVN1cMDvYOPNuZ8L1XQU=", + "experimental/__init__.py": "47DEQpj8HBSa+/TImW+5JCeuQeRkm5NMpJWZG3hSuFU=", + "experimental/indexing.py": "yFFflW3+kV6/5PPJU7/jOJsJBCWCWlE4dGlu9gwSPXo=", + "layers/__init__.py": "vBtnAt2afs4QlqpeFU4dlZNxBuC9IXl3fmilk+2OzHM=", + "layers/_einmix.py": "9cDMcCmn2y1jN9xLx587GAY1GCb+9TvEjJWaNNX4Vps=", + "layers/flax.py": "zFy83gSLRm31cLuKFRvZ82/HsefnXPbRvkKZh1KkC1I=", + "layers/keras.py": "+7So0w94phvf9HdW0xi2mSeBg02qVPvAyfp/1XR02NM=", + "layers/oneflow.py": "YEPzz4xc7BDRQfb8ulD3teqQJdbO6qQg7Z4KIPVTLz8=", + "layers/paddle.py": "8cRZQ8BT9vYEczh7pNProuTM/3XjLty2ht2sdvXNFiI=", + "layers/tensorflow.py": "T9uhSVwbXREahc31ARAHoN5K+7zsuS8NRNPdY6Zk1Bc=", + "layers/torch.py": "504G99kEgy7dk1UPBbj9hzJmZkAHwVhMDFN/8J+p3C8=", + "packing.py": "vBjwbVWs3OwmI83BMNoeu3jBFAbAiy10i3ClmZxZtCQ=", + "parsing.py": "tXcSr4W1mbePUu+oIYgG0cKYNxsPXuVYFIHynj1FCg4=" + } + }, + "provenance": { + "kernel-builder": { + "version": "0.17.0-dev0", + "sha": "d0610aa58db33b142c86b59598a2a1c730f52996", + "dirty": false + }, + "kernel": { + "sha": "d45addadb0c380f3d7cc2813310b3bcd75f8aa9a", + "dirty": false + } + } +} diff --git a/kernels/tests/test_dirty_provenance.py b/kernels/tests/test_dirty_provenance.py index 7a039c1c..b9119d63 100644 --- a/kernels/tests/test_dirty_provenance.py +++ b/kernels/tests/test_dirty_provenance.py @@ -24,16 +24,16 @@ def _write_variant(tmp_path, provenance): CLEAN_PROVENANCE = { - "kernel-builder": {"version": "0.1.0", "sha": "a" * 40, "dirty": False}, - "kernel": {"sha": "b" * 40, "dirty": False}, + "kernel-builder": {"version": "0.1.0", "commit": "a" * 40, "dirty": False}, + "kernel": {"commit": "b" * 40, "dirty": False}, } DIRTY_KERNEL = { - "kernel-builder": {"version": "0.1.0", "sha": "a" * 40, "dirty": False}, - "kernel": {"sha": "b" * 40, "dirty": True}, + "kernel-builder": {"version": "0.1.0", "commit": "a" * 40, "dirty": False}, + "kernel": {"commit": "b" * 40, "dirty": True}, } DIRTY_BUILDER = { - "kernel-builder": {"version": "0.1.0", "sha": "a" * 40, "dirty": True}, - "kernel": {"sha": "b" * 40, "dirty": False}, + "kernel-builder": {"version": "0.1.0", "commit": "a" * 40, "dirty": True}, + "kernel": {"commit": "b" * 40, "dirty": False}, }