-
Notifications
You must be signed in to change notification settings - Fork 323
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.
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
Fix the Engine version check in GUI #6570
Changes from 1 commit
f5390e3
e7078ec
6b50907
7788a82
59777c7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,17 +20,62 @@ use enso_json_to_struct::json_to_struct; | |
|
||
|
||
// ============== | ||
// === Config === | ||
// === Errors === | ||
// ============== | ||
|
||
/// A wrapper for an error with information that the Engine version does not meet the requirements. | ||
#[derive(Clone, Debug, thiserror::Error)] | ||
#[error("Unsupported Engine version: required {required} (or newer), found {found}.")] | ||
pub struct UnsupportedEngineVersion { | ||
/// The version of the Engine that is required. | ||
pub required: semver::Version, | ||
/// The version of the Engine that was found. | ||
pub found: semver::Version, | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More whitespace. |
||
// =============== | ||
// === Version === | ||
// =============== | ||
|
||
include!(concat!(env!("OUT_DIR"), "/config.rs")); | ||
|
||
pub use generated::*; | ||
|
||
pub fn engine_version_requirement() -> semver::VersionReq { | ||
semver::VersionReq::parse(&format!(">={engine_version_supported}")).unwrap() | ||
/// The minimum supported engine version. | ||
pub fn engine_version_required() -> semver::Version { | ||
// Safe to unwrap, as `engine_version_supported` compile-time and is validated by the test. | ||
semver::Version::parse(engine_version_supported).unwrap() | ||
} | ||
|
||
/// Check if the given Engine version meets the requirements. | ||
/// | ||
/// Effectively, this checks if the given version is greater or equal to the minimum supported. | ||
/// "Greater or equal" is defined by the [Semantic Versioning specification](https://semver.org/) | ||
/// term of precedence. | ||
pub fn check_engine_version( | ||
engine_version: &semver::Version, | ||
) -> Result<(), UnsupportedEngineVersion> { | ||
// We don't want to rely on the `semver::VersionReq` semantics here. Unfortunately the | ||
// [Semantic Versioning specification](https://semver.org/) does not define the semantics of | ||
// of the version requirement operators, so different implementations may behave differently. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doubled |
||
// | ||
// The `semver::VersionReq` implementation follows the Cargo's implementation, namely: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
// ``` | ||
// In particular, in order for any VersionReq to match a pre-release version, the VersionReq | ||
// must contain at least one Comparator that has an explicit major, minor, and patch version | ||
// identical to the pre-release being matched, and that has a nonempty pre-release component. | ||
// ``` | ||
Comment on lines
+66
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a source you can readily link for this? |
||
// This leads to counter-intuitive behavior, where `2023.0.0-dev` does not fulfill the | ||
// `>= 2022.0.0-dev` requirement. | ||
if engine_version < &engine_version_required() { | ||
Err(UnsupportedEngineVersion { | ||
required: engine_version_required(), | ||
found: engine_version.clone(), | ||
}) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
|
||
|
||
// ============ | ||
|
@@ -64,3 +109,24 @@ pub fn read_args() -> Args { | |
lazy_static! { | ||
pub static ref ARGS: Args = read_args(); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing vertical space. |
||
// ============= | ||
// === Tests === | ||
// ============= | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use super::*; | ||
|
||
#[test] | ||
fn check_that_version_requirement_parses() { | ||
// We just expect that it won't panic. | ||
let _ = engine_version_required(); | ||
} | ||
|
||
#[test] | ||
fn new_project_engine_version_fills_requirements() { | ||
// Sanity check: required version must be supported. | ||
assert!(check_engine_version(&engine_version_required()).is_ok()); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about a test that would fail with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a wrapper for an error or simply an error type?