Skip to content

Allow version regex to match 4.0.stable.arch_linux #168

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Allow version regex to match 4.0.stable.arch_linux #168

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c6caebf
Allow version regex to match 4.0.stable.arch_linux
ttencate Mar 11, 2023
725c280
Fix version parsing if surrounded by other lines
ttencate Mar 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 72 additions & 31 deletions godot-codegen/src/godot_version.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@

//#![allow(unused_variables, dead_code)]

use regex::Regex;
use regex::{Captures, Regex};
use std::error::Error;
use std::str::FromStr;

#[derive(Debug, Eq, PartialEq)]
pub struct GodotVersion {
/// the original string (trimmed, stripped of text around)
pub full_string: String,
Expand All @@ -20,36 +22,64 @@ pub struct GodotVersion {
pub patch: u8,

/// alpha|beta|dev|stable
pub stability: String,
pub status: String,

/// Git revision 'custom_build.{rev}' or '{official}.rev', if available
pub custom_rev: Option<String>,
}

pub fn parse_godot_version(version_str: &str) -> Result<GodotVersion, Box<dyn Error>> {
// Format of the string emitted by `godot --version`:
// https://github.com/godot-rust/gdext/issues/118#issuecomment-1465748123
// We assume that it's on a line of its own, but it may be surrounded by other lines.
let regex = Regex::new(
// major minor [patch] official|custom_build|gentoo
// v v v v
r#"(\d+)\.(\d+)(?:\.(\d+))?\.(alpha|beta|dev|rc|stable)[0-9]*\.(?:mono\.)?(?:[a-z_]+\.([a-f0-9]+)|official)"#,
r#"(?xm)
# x: ignore whitespace and allow line comments (starting with `#`)
# m: multi-line mode, ^ and $ match start and end of line
^
(?P<major>\d+)
\.(?P<minor>\d+)
# Patch version is omitted if it's zero.
(?:\.(?P<patch>\d+))?
# stable|dev|alpha|beta|rc12|... Can be set through an env var when the engine is built.
\.(?P<status>[^.]+)
# Capture both module config and build, could be multiple components:
# mono|official|custom_build|gentoo|arch_linux|...
# Notice +? for non-greedy match.
(\.[^.]+)+?
# Git commit SHA1, currently truncated to 9 chars, but accept the full thing
(?:\.(?P<custom_rev>[a-f0-9]{9,40}))?
$
"#,
)?;

let fail = || format!("Version substring cannot be parsed: `{version_str}`");
let caps = regex.captures(version_str).ok_or_else(fail)?;

Ok(GodotVersion {
full_string: caps.get(0).unwrap().as_str().to_string(),
major: caps.get(1).ok_or_else(fail)?.as_str().parse::<u8>()?,
minor: caps.get(2).ok_or_else(fail)?.as_str().parse::<u8>()?,
patch: caps
.get(3)
.map(|m| m.as_str().parse::<u8>())
.transpose()?
.unwrap_or(0),
stability: caps.get(4).ok_or_else(fail)?.as_str().to_string(),
custom_rev: caps.get(5).map(|m| m.as_str().to_string()),
major: cap(&caps, "major")?.unwrap(),
minor: cap(&caps, "minor")?.unwrap(),
patch: cap(&caps, "patch")?.unwrap_or(0),
status: cap(&caps, "status")?.unwrap(),
custom_rev: cap(&caps, "custom_rev")?,
})
}

/// Extracts and parses a named capture group from a regex match.
fn cap<T: FromStr>(caps: &Captures, key: &str) -> Result<Option<T>, Box<dyn Error>> {
caps.name(key)
.map(|m| m.as_str().parse())
.transpose()
.map_err(|_| {
format!(
"Version string cannot be parsed: `{}`",
caps.get(0).unwrap().as_str()
)
.into()
})
}

// ----------------------------------------------------------------------------------------------------------------------------------------------

#[test]
Expand All @@ -64,35 +94,46 @@ fn test_godot_versions() {
("3.0.1.stable.official", 3, 0, 1, "stable", None),
("3.2.stable.official", 3, 2, 0, "stable", None),
("3.37.stable.official", 3, 37, 0, "stable", None),
("3.4.stable.official.206ba70f4", 3, 4, 0, "stable", s("206ba70f4")),
("3.4.1.stable.official.aa1b95889", 3, 4, 1, "stable", s("aa1b95889")),
("3.4.stable.official.206ba70f4", 3, 4, 0, "stable", s("206ba70f4")),
("3.4.1.stable.official.aa1b95889", 3, 4, 1, "stable", s("aa1b95889")),
("3.5.beta.custom_build.837f2c5f8", 3, 5, 0, "beta", s("837f2c5f8")),
("4.0.beta8.gentoo.45cac42c0", 4, 0, 0, "beta", s("45cac42c0")),
("4.0.beta8.gentoo.45cac42c0", 4, 0, 0, "beta8", s("45cac42c0")),
("4.0.dev.custom_build.e7e9e663b", 4, 0, 0, "dev", s("e7e9e663b")),
("4.0.alpha.custom_build.faddbcfc0", 4, 0, 0, "alpha", s("faddbcfc0")),
("4.0.beta8.mono.custom_build.b28ddd918", 4, 0, 0, "beta", s("b28ddd918")),
("4.0.rc1.official.8843d9ad3", 4, 0, 0, "rc", s("8843d9ad3")),
("4.0.beta8.mono.custom_build.b28ddd918", 4, 0, 0, "beta8", s("b28ddd918")),
("4.0.rc1.official.8843d9ad3", 4, 0, 0, "rc1", s("8843d9ad3")),
("4.0.stable.arch_linux", 4, 0, 0, "stable", None),
// Output from 4.0.stable on MacOS in debug mode:
// https://github.com/godotengine/godot/issues/74906
("arguments
0: /Users/runner/work/_temp/godot_bin/godot.macos.editor.dev.x86_64
1: --version
Current path: /Users/runner/work/gdext/gdext/godot-core
4.1.dev.custom_build.79454bfd3", 4, 1, 0, "dev", s("79454bfd3")),
];

let bad_versions = [
"4.0.unstable.custom_build.e7e9e663b", // 'unstable'
"4.0.3.custom_build.e7e9e663b", // no stability
"3.stable.official.206ba70f4", // no minor
"4.0.alpha.custom_build", // no rev after 'custom_build' (this is allowed for 'official' however)
Copy link
Contributor Author

@ttencate ttencate Mar 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this removal is on purpose. I don't see why we need to reject this form.

Copy link
Member

@Bromeon Bromeon Mar 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See discussion in #118. Eventually we should probably accept them, but require manual mapping, warning, etc. Maybe the easiest approach for this would be parsing the version into something like:

enum GodotVersion {
    Release{major, minor, patch},
    Git{sha},
    Unknown
}

There's of course a separate discussion to be had whether we should still support alpha/beta/rc now, but at least rc can still occur before minor releases. And since such binaries may still exist, we might want to detect them, to provide more helpful errors. It also separates the version parsing from any semantic implications.

"Godot Engine v4.0.stable.arch_linux - https://godotengine.org", // Surrounding cruft
"3.stable.official.206ba70f4", // No minor version
"4.0.stable", // No build type
];

for (full, major, minor, patch, stability, custom_rev) in good_versions {
for (full, major, minor, patch, status, custom_rev) in good_versions {
let expected = GodotVersion {
// Version line is last in every test at the moment.
full_string: full.lines().last().unwrap().to_owned(),
major,
minor,
patch,
status: status.to_owned(),
custom_rev,
};
let parsed: GodotVersion = parse_godot_version(full).unwrap();
assert_eq!(parsed.major, major);
assert_eq!(parsed.minor, minor);
assert_eq!(parsed.patch, patch);
assert_eq!(parsed.stability, stability);
assert_eq!(parsed.custom_rev, custom_rev);
assert_eq!(parsed.full_string, full);
assert_eq!(parsed, expected, "{}", full);
}

for full in bad_versions {
let parsed = parse_godot_version(full);
assert!(parsed.is_err());
assert!(parsed.is_err(), "{}", full);
}
}