From a151847730e3d41db607f3fd146f84d06ad65dba Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 7 Nov 2024 16:12:02 +0100 Subject: [PATCH 1/4] Support version strings with commit hashes --- crates/build/re_build_info/src/build_info.rs | 8 +++-- .../build/re_build_info/src/crate_version.rs | 35 ++++++++++--------- 2 files changed, 25 insertions(+), 18 deletions(-) diff --git a/crates/build/re_build_info/src/build_info.rs b/crates/build/re_build_info/src/build_info.rs index e81cc03dab01..34b75fa65c93 100644 --- a/crates/build/re_build_info/src/build_info.rs +++ b/crates/build/re_build_info/src/build_info.rs @@ -175,8 +175,12 @@ fn crate_version_from_build_info_string() { { let expected_crate_version = build_info.version; - let crate_version = CrateVersion::try_parse_from_build_info_string(build_info_str); + let crate_version = CrateVersion::try_parse_from_build_info_string(&build_info_str); - assert_eq!(Ok(expected_crate_version), crate_version); + assert_eq!( + crate_version, + Ok(expected_crate_version), + "Failed to parse {build_info_str:?}" + ); } } diff --git a/crates/build/re_build_info/src/crate_version.rs b/crates/build/re_build_info/src/crate_version.rs index 0035a14404a0..90730618430e 100644 --- a/crates/build/re_build_info/src/crate_version.rs +++ b/crates/build/re_build_info/src/crate_version.rs @@ -41,7 +41,6 @@ mod meta { /// - `01NNNNNN` -> `-rc.N` /// - `00000000` -> none of the above #[derive(Clone, Copy, Debug, PartialEq, Eq)] -#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))] pub struct CrateVersion { pub major: u8, pub minor: u8, @@ -312,20 +311,6 @@ impl CrateVersion { }}; } - macro_rules! eat_token { - ($s:ident, $token:expr, $msg:literal) => {{ - let token = $token; - if token.len() > $s.len() { - return Err($msg); - } - let (left, right) = split_at($s, token.len()); - if !equals(left, token) { - return Err($msg); - } - right - }}; - } - macro_rules! eat_u8 { ($s:ident, $msg:literal) => {{ if $s.is_empty() { @@ -393,8 +378,16 @@ impl CrateVersion { s = remainder; match meta { Some(Meta::Alpha(build)) => { - s = eat_token!(s, b"dev", "expected `dev` after `+`"); meta = Some(Meta::DevAlpha(build)); + if let (true, remainder) = maybe_token(s, b"dev") { + s = remainder; + } else if s.is_empty() { + return Err("expected `dev` after `+`"); + } else { + // It's a commit hash + let commit_hash = s; // TODO: use + s = &[]; + } } Some(..) => return Err("unexpected `-rc` with `+dev`"), None => return Err("unexpected `+dev` without `-alpha`"), @@ -480,6 +473,16 @@ fn test_parse_version() { meta: Some(Meta::DevAlpha(63)), } ); + // We use commit hash suffixes in some cases: + assert_parse_ok!( + "12.23.24-alpha.63+aab0b4e", + CrateVersion { + major: 12, + minor: 23, + patch: 24, + meta: Some(Meta::DevAlpha(63)), + } + ); } #[test] From 791299ca96d40272cbcde02413c70a1471094fa4 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 7 Nov 2024 17:07:27 +0100 Subject: [PATCH 2/4] Store commit hash --- crates/build/re_build_info/src/build_info.rs | 7 +- .../build/re_build_info/src/crate_version.rs | 68 ++++++++++++++----- 2 files changed, 57 insertions(+), 18 deletions(-) diff --git a/crates/build/re_build_info/src/build_info.rs b/crates/build/re_build_info/src/build_info.rs index 34b75fa65c93..616dae91bf23 100644 --- a/crates/build/re_build_info/src/build_info.rs +++ b/crates/build/re_build_info/src/build_info.rs @@ -142,7 +142,7 @@ impl CrateVersion { /// [] /// ``` pub fn try_parse_from_build_info_string(s: impl AsRef) -> Result { - let s = s.as_ref(); + let s = Box::leak(s.as_ref().to_owned().into_boxed_str()); // We leak it here to get a 'static str, so it can be parsed by the same const function. Yes, this is ugly. let parts = s.split_whitespace().collect::>(); if parts.len() < 2 { return Err(format!("{s:?} is not a valid BuildInfo string")); @@ -160,7 +160,10 @@ fn crate_version_from_build_info_string() { major: 0, minor: 10, patch: 0, - meta: Some(crate::crate_version::Meta::DevAlpha(7)), + meta: Some(crate::crate_version::Meta::DevAlpha { + alpha: 7, + commit: None, + }), }, rustc_version: "1.76.0 (d5c2e9c34 2023-09-13)", llvm_version: "16.0.5", diff --git a/crates/build/re_build_info/src/crate_version.rs b/crates/build/re_build_info/src/crate_version.rs index 90730618430e..aed2f169a786 100644 --- a/crates/build/re_build_info/src/crate_version.rs +++ b/crates/build/re_build_info/src/crate_version.rs @@ -86,15 +86,21 @@ impl CrateVersion { pub enum Meta { Rc(u8), Alpha(u8), - DevAlpha(u8), + DevAlpha { + alpha: u8, + /// Utf8 + commit: Option<&'static [u8]>, + }, } impl Meta { - pub const fn to_byte(self) -> u8 { + pub const fn to_byte(&self) -> u8 { match self { - Self::Rc(value) => value | meta::RC, - Self::Alpha(value) => value | meta::ALPHA, - Self::DevAlpha(value) => value | meta::DEV_ALPHA, + Self::Rc(value) => *value | meta::RC, + Self::Alpha(value) => *value | meta::ALPHA, + + // We ignore the commit hash, if any + Self::DevAlpha { alpha, .. } => *alpha | meta::DEV_ALPHA, } } @@ -104,7 +110,10 @@ impl Meta { match tag { meta::RC => Some(Self::Rc(value)), meta::ALPHA => Some(Self::Alpha(value)), - meta::DEV_ALPHA => Some(Self::DevAlpha(value)), + meta::DEV_ALPHA => Some(Self::DevAlpha { + alpha: value, + commit: None, + }), _ => None, } } @@ -202,12 +211,12 @@ impl CrateVersion { /// This is used to identify builds which are not explicit releases, /// such as local builds and CI builds for every commit. pub fn is_dev(&self) -> bool { - matches!(self.meta, Some(Meta::DevAlpha(..))) + matches!(self.meta, Some(Meta::DevAlpha { .. })) } /// Whether or not this is an alpha version (`-alpha.N` or `-alpha.N+dev`). pub fn is_alpha(&self) -> bool { - matches!(self.meta, Some(Meta::Alpha(..) | Meta::DevAlpha(..))) + matches!(self.meta, Some(Meta::Alpha(..) | Meta::DevAlpha { .. })) } /// Whether or not this is a release candidate (`-rc.N`). @@ -231,7 +240,7 @@ impl CrateVersion { self.major, self.minor, self.patch, - self.meta.map(Meta::to_byte).unwrap_or_default(), + self.meta.as_ref().map(Meta::to_byte).unwrap_or_default(), ] } @@ -260,11 +269,13 @@ impl CrateVersion { self.major == other.major } } +} +impl CrateVersion { /// Parse a version string according to our subset of semver. /// /// See [`CrateVersion`] for more information. - pub const fn parse(version_string: &str) -> Self { + pub const fn parse(version_string: &'static str) -> Self { match Self::try_parse(version_string) { Ok(version) => version, Err(_err) => { @@ -278,7 +289,7 @@ impl CrateVersion { /// Parse a version string according to our subset of semver. /// /// See [`CrateVersion`] for more information. - pub const fn try_parse(version_string: &str) -> Result { + pub const fn try_parse(version_string: &'static str) -> Result { // Note that this is a const function, which means we are extremely limited in what we can do! const fn maybe(s: &[u8], c: u8) -> (bool, &[u8]) { @@ -378,15 +389,22 @@ impl CrateVersion { s = remainder; match meta { Some(Meta::Alpha(build)) => { - meta = Some(Meta::DevAlpha(build)); if let (true, remainder) = maybe_token(s, b"dev") { s = remainder; + meta = Some(Meta::DevAlpha { + alpha: build, + commit: None, + }); } else if s.is_empty() { return Err("expected `dev` after `+`"); } else { // It's a commit hash let commit_hash = s; // TODO: use s = &[]; + meta = Some(Meta::DevAlpha { + alpha: build, + commit: Some(commit_hash), + }); } } Some(..) => return Err("unexpected `-rc` with `+dev`"), @@ -412,7 +430,17 @@ impl std::fmt::Display for Meta { match self { Self::Rc(build) => write!(f, "-rc.{build}"), Self::Alpha(build) => write!(f, "-alpha.{build}"), - Self::DevAlpha(build) => write!(f, "-alpha.{build}+dev"), + Self::DevAlpha { alpha, commit } => { + if let Some(commit) = commit { + if let Ok(commit) = std::str::from_utf8(commit) { + write!(f, "-alpha.{alpha}+{commit}") + } else { + write!(f, "-alpha.{alpha}+dev") + } + } else { + write!(f, "-alpha.{alpha}+dev") + } + } } } } @@ -470,7 +498,10 @@ fn test_parse_version() { major: 12, minor: 23, patch: 24, - meta: Some(Meta::DevAlpha(63)), + meta: Some(Meta::DevAlpha { + alpha: 63, + commit: None + }), } ); // We use commit hash suffixes in some cases: @@ -480,7 +511,10 @@ fn test_parse_version() { major: 12, minor: 23, patch: 24, - meta: Some(Meta::DevAlpha(63)), + meta: Some(Meta::DevAlpha { + alpha: 63, + commit: Some(b"aab0b4e") + }), } ); } @@ -494,6 +528,7 @@ fn test_format_parse_roundtrip() { "12.23.24-rc.63", "12.23.24-alpha.63", "12.23.24-alpha.63+dev", + "12.23.24-alpha.63+aab0b4e", ] { assert_eq!(CrateVersion::parse(version).to_string(), version); } @@ -508,6 +543,7 @@ fn test_format_parse_roundtrip_bytes() { "12.23.24-rc.63", "12.23.24-alpha.63", "12.23.24-alpha.63+dev", + // "12.23.24-alpha.63+aab0b4e", // we don't serialize commit hashes! ] { let version = CrateVersion::parse(version); let bytes = version.to_bytes(); @@ -517,7 +553,7 @@ fn test_format_parse_roundtrip_bytes() { #[test] fn test_compatibility() { - fn are_compatible(a: &str, b: &str) -> bool { + fn are_compatible(a: &'static str, b: &'static str) -> bool { CrateVersion::parse(a).is_compatible_with(CrateVersion::parse(b)) } From d95fba610d7275a2c35a638959d35934a4acf882 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 7 Nov 2024 17:14:10 +0100 Subject: [PATCH 3/4] &self -> self --- crates/build/re_build_info/src/crate_version.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/build/re_build_info/src/crate_version.rs b/crates/build/re_build_info/src/crate_version.rs index aed2f169a786..50f0e3f2a1ee 100644 --- a/crates/build/re_build_info/src/crate_version.rs +++ b/crates/build/re_build_info/src/crate_version.rs @@ -94,13 +94,13 @@ pub enum Meta { } impl Meta { - pub const fn to_byte(&self) -> u8 { + pub const fn to_byte(self) -> u8 { match self { - Self::Rc(value) => *value | meta::RC, - Self::Alpha(value) => *value | meta::ALPHA, + Self::Rc(value) => value | meta::RC, + Self::Alpha(value) => value | meta::ALPHA, // We ignore the commit hash, if any - Self::DevAlpha { alpha, .. } => *alpha | meta::DEV_ALPHA, + Self::DevAlpha { alpha, .. } => alpha | meta::DEV_ALPHA, } } @@ -240,7 +240,7 @@ impl CrateVersion { self.major, self.minor, self.patch, - self.meta.as_ref().map(Meta::to_byte).unwrap_or_default(), + self.meta.map(Meta::to_byte).unwrap_or_default(), ] } From b01e0d51541c8e197c089a146fc0cfaf55b16f89 Mon Sep 17 00:00:00 2001 From: Emil Ernerfeldt Date: Thu, 7 Nov 2024 17:59:35 +0100 Subject: [PATCH 4/4] Cleanup --- crates/build/re_build_info/src/build_info.rs | 5 +++- .../build/re_build_info/src/crate_version.rs | 26 ++++++++++++------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/crates/build/re_build_info/src/build_info.rs b/crates/build/re_build_info/src/build_info.rs index 616dae91bf23..de9167dd3c35 100644 --- a/crates/build/re_build_info/src/build_info.rs +++ b/crates/build/re_build_info/src/build_info.rs @@ -142,7 +142,10 @@ impl CrateVersion { /// [] /// ``` pub fn try_parse_from_build_info_string(s: impl AsRef) -> Result { - let s = Box::leak(s.as_ref().to_owned().into_boxed_str()); // We leak it here to get a 'static str, so it can be parsed by the same const function. Yes, this is ugly. + // `CrateVersion::try_parse` is `const` (for good reasons), and needs a `&'static str`. + // In order to accomplish this, we need to leak the string here. + let s = Box::leak(s.as_ref().to_owned().into_boxed_str()); + let parts = s.split_whitespace().collect::>(); if parts.len() < 2 { return Err(format!("{s:?} is not a valid BuildInfo string")); diff --git a/crates/build/re_build_info/src/crate_version.rs b/crates/build/re_build_info/src/crate_version.rs index 50f0e3f2a1ee..a886c4c828b1 100644 --- a/crates/build/re_build_info/src/crate_version.rs +++ b/crates/build/re_build_info/src/crate_version.rs @@ -86,9 +86,22 @@ impl CrateVersion { pub enum Meta { Rc(u8), Alpha(u8), + + /// `0.19.1-alpha.2+dev` or `0.19.1-alpha.2+aab0b4e` DevAlpha { alpha: u8, - /// Utf8 + + /// The commit hash, if known + /// + /// `None` corresponds to `+dev`. + /// + /// In order to support compile-time parsing of versions strings + /// this needs to be `&'static` and `[u8]` instead of `String`. + /// But in practice this is guaranteed to be valid UTF-8. + /// + /// The commit hash is NOT sent over the wire, + /// so `0.19.1-alpha.2+aab0b4e` will end up as `0.19.1-alpha.2+dev` + /// on the other end. commit: Option<&'static [u8]>, }, } @@ -398,8 +411,7 @@ impl CrateVersion { } else if s.is_empty() { return Err("expected `dev` after `+`"); } else { - // It's a commit hash - let commit_hash = s; // TODO: use + let commit_hash = s; s = &[]; meta = Some(Meta::DevAlpha { alpha: build, @@ -431,12 +443,8 @@ impl std::fmt::Display for Meta { Self::Rc(build) => write!(f, "-rc.{build}"), Self::Alpha(build) => write!(f, "-alpha.{build}"), Self::DevAlpha { alpha, commit } => { - if let Some(commit) = commit { - if let Ok(commit) = std::str::from_utf8(commit) { - write!(f, "-alpha.{alpha}+{commit}") - } else { - write!(f, "-alpha.{alpha}+dev") - } + if let Some(commit) = commit.and_then(|s| std::str::from_utf8(s).ok()) { + write!(f, "-alpha.{alpha}+{commit}") } else { write!(f, "-alpha.{alpha}+dev") }