Review feedback

utils/wasm-builder/src/lib.rs

@@ -115,9 +115,11 @@ use std::{
 	path::{Path, PathBuf},
 	process::Command,
 };
+use version::Version;
 
 mod builder;
 mod prerequisites;
+mod version;
 mod wasm_project;
 
 pub use builder::{WasmBuilder, WasmBuilderSelectProject};
@@ -210,7 +212,7 @@ fn get_rustup_command() -> Option<CargoCommand> {
 	let output = Command::new("rustup").args(&["toolchain", "list"]).output().ok()?.stdout;
 	let lines = output.as_slice().lines();
 
-	let mut found_version: Option<(String, Version)> = None;
+	let mut versions = Vec::new();
 	for line in lines.filter_map(|l| l.ok()) {
 		let rustup_version = line.trim_end_matches(&host);
 
@@ -222,48 +224,15 @@ fn get_rustup_command() -> Option<CargoCommand> {
 
 		let Some(cargo_version) = cmd.version() else { continue; };
 
-		if let Some((rustup_version_other, cargo_version_other)) = &mut found_version {
-			let is_older = cargo_version_other.is_older(&cargo_version);
-
-			// Nightly should not overwrite a stable version.
-			if cargo_version_other.is_stable() && cargo_version.is_nightly {
-				continue
-			}
-
-			// Stable versions overwrite nightly versions or otherwise just check which one is
-			// older.
-			if cargo_version_other.is_nightly && cargo_version.is_stable() || is_older {
-				*rustup_version_other = rustup_version.into();
-				*cargo_version_other = cargo_version;
-			}
-		} else {
-			found_version = Some((rustup_version.into(), cargo_version));
-		}
+		versions.push((cargo_version, rustup_version.to_string()));
 	}
 
-	let version = found_version?.0;
-	Some(CargoCommand::new_with_args("rustup", &["run", &version, "cargo"]))
-}
-
-/// The version of rustc/cargo.
-#[derive(Clone, Copy, Debug)]
-struct Version {
-	pub major: u32,
-	pub minor: u32,
-	pub patch: u32,
-	pub is_nightly: bool,
-}
-
-impl Version {
-	/// Returns if `self` is older than `other`.
-	fn is_older(&self, other: &Self) -> bool {
-		self.major < other.major || self.minor < other.minor || self.patch < other.patch
-	}
+	// Sort by the parsed version to get the latest version (greatest version) at the end of the
+	// vec.
+	versions.sort_by_key(|v| v.0);
+	let version = &versions.last()?.1;
 
-	/// Returns if `self` is a stable version.
-	fn is_stable(&self) -> bool {
-		!self.is_nightly
-	}
+	Some(CargoCommand::new_with_args("rustup", &["run", &version, "cargo"]))
 }
 
 /// Wraps a specific command which represents a cargo invocation.
@@ -305,28 +274,7 @@ impl CargoCommand {
 			.ok()
 			.and_then(|o| String::from_utf8(o.stdout).ok())?;
 
-		if let Some(version) = version.split(" ").nth(1) {
-			let mut is_nightly = false;
-			let parts = version
-				.split(".")
-				.filter_map(|v| {
-					if let Some(rest) = v.strip_suffix("-nightly") {
-						is_nightly = true;
-						rest.parse().ok()
-					} else {
-						v.parse().ok()
-					}
-				})
-				.collect::<Vec<u32>>();
-
-			if parts.len() != 3 {
-				return None
-			}
-
-			Some(Version { major: parts[0], minor: parts[1], patch: parts[2], is_nightly })
-		} else {
-			None
-		}
+		Version::extract(&version)
 	}
 
 	/// Returns the version of this cargo command or `None` if it failed to extract the version.

utils/wasm-builder/src/version.rs

@@ -0,0 +1,180 @@
+// This file is part of Substrate.
+
+// Copyright (C) Parity Technologies (UK) Ltd.
+// SPDX-License-Identifier: Apache-2.0
+
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// 	http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+use std::cmp::Ordering;
+
+/// The version of rustc/cargo.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct Version {
+	pub major: u32,
+	pub minor: u32,
+	pub patch: u32,
+	pub is_nightly: bool,
+	pub year: u32,
+	pub month: u32,
+	pub day: u32,
+}
+
+impl Version {
+	/// Returns if `self` is a stable version.
+	pub fn is_stable(&self) -> bool {
+		!self.is_nightly
+	}
+
+	/// Return if `self` is a nightly version.
+	pub fn is_nightly(&self) -> bool {
+		self.is_nightly
+	}
+
+	/// Extract from the given `version` string.
+	pub fn extract(version: &str) -> Option<Self> {
+		let mut is_nightly = false;
+		let version_parts = version
+			.split(" ")
+			.nth(1)?
+			.split(".")
+			.filter_map(|v| {
+				if let Some(rest) = v.strip_suffix("-nightly") {
+					is_nightly = true;
+					rest.parse().ok()
+				} else {
+					v.parse().ok()
+				}
+			})
+			.collect::<Vec<u32>>();
+
+		if version_parts.len() != 3 {
+			return None
+		}
+
+		let date = version.split(" ").nth(3)?;
+
+		let date_parts = date
+			.split("-")
+			.filter_map(|v| v.strip_suffix(")").unwrap_or(v).parse().ok())
+			.collect::<Vec<u32>>();
+
+		if date_parts.len() != 3 {
+			return None
+		}
+
+		Some(Version {
+			major: version_parts[0],
+			minor: version_parts[1],
+			patch: version_parts[2],
+			is_nightly,
+			year: date_parts[0],
+			month: date_parts[1],
+			day: date_parts[2],
+		})
+	}
+}
+
+/// Ordering is done in the following way:
+///
+/// 1. `stable` > `nightly`
+/// 2. Then compare major, minor and patch.
+/// 3. Last compare the date.
+impl Ord for Version {
+	fn cmp(&self, other: &Self) -> Ordering {
+		if self == other {
+			return Ordering::Equal
+		}
+
+		// Ensure that `stable > nightly`
+		if self.is_stable() && other.is_nightly() {
+			return Ordering::Greater
+		} else if self.is_nightly() && other.is_stable() {
+			return Ordering::Less
+		}
+
+		let to_compare = [
+			(self.major, other.major),
+			(self.minor, other.minor),
+			(self.patch, other.patch),
+			(self.year, other.year),
+			(self.month, other.month),
+			(self.day, other.day),
+		];
+
+		to_compare
+			.iter()
+			.find_map(|(l, r)| if l != r { l.partial_cmp(&r) } else { None })
+			// We already checked this right at the beginning, so we should never return here
+			// `Equal`.
+			.unwrap_or(Ordering::Equal)
+	}
+}
+
+impl PartialOrd for Version {
+	fn partial_cmp(&self, other: &Self) -> Option<Ordering> {
+		Some(self.cmp(other))
+	}
+}
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+
+	#[test]
+	fn version_compare_and_extract_works() {
+		let version_1_66_0 = Version::extract("cargo 1.66.0 (d65d197ad 2022-11-15)").unwrap();
+		let version_1_66_1 = Version::extract("cargo 1.66.1 (d65d197ad 2022-11-15)").unwrap();
+		let version_1_66_0_nightly =
+			Version::extract("cargo 1.66.0-nightly (d65d197ad 2022-10-15)").unwrap();
+		let version_1_66_0_nightly_older_date =
+			Version::extract("cargo 1.66.0-nightly (d65d197ad 2022-10-14)").unwrap();
+		let version_1_65_0 = Version::extract("cargo 1.65.0 (d65d197ad 2022-10-15)").unwrap();
+		let version_1_65_0_older_date =
+			Version::extract("cargo 1.65.0 (d65d197ad 2022-10-14)").unwrap();
+
+		assert!(version_1_66_1 > version_1_66_0);
+		assert!(version_1_66_1 > version_1_65_0);
+		assert!(version_1_66_1 > version_1_66_0_nightly);
+		assert!(version_1_66_1 > version_1_66_0_nightly_older_date);
+		assert!(version_1_66_1 > version_1_65_0_older_date);
+
+		assert!(version_1_66_0 > version_1_65_0);
+		assert!(version_1_66_0 > version_1_66_0_nightly);
+		assert!(version_1_66_0 > version_1_66_0_nightly_older_date);
+		assert!(version_1_66_0 > version_1_65_0_older_date);
+
+		assert!(version_1_65_0 > version_1_66_0_nightly);
+		assert!(version_1_65_0 > version_1_66_0_nightly_older_date);
+		assert!(version_1_65_0 > version_1_65_0_older_date);
+
+		let mut versions = vec![
+			version_1_66_0,
+			version_1_66_0_nightly,
+			version_1_66_0_nightly_older_date,
+			version_1_65_0_older_date,
+			version_1_65_0,
+			version_1_66_1,
+		];
+		versions.sort_by(|a, b| b.cmp(a));
+
+		let expected_versions_order = vec![
+			version_1_66_1,
+			version_1_66_0,
+			version_1_65_0,
+			version_1_65_0_older_date,
+			version_1_66_0_nightly,
+			version_1_66_0_nightly_older_date,
+		];
+		assert_eq!(expected_versions_order, versions);
+	}
+}