Skip to content
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: Compilation fix under Ubuntu #109

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
1 change: 1 addition & 0 deletions proto-compiler/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ ureq = { "version" = "2.10" }
zip = { version = "2.2", default-features = false, features = ["deflate"] }
fs_extra = { version = "1.3.0" }
tonic-build = { version = "0.12", optional = true }
semver = "1.0"


[features]
Expand Down
3 changes: 2 additions & 1 deletion proto-compiler/src/constants.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
//! Tenderdash protobuf implementation

// Requirements
pub const DEP_PROTOC_VERSION: f32 = 25.0;
pub const DEP_PROTOC_VERSION_UBUNTU: &str = "3.12.4";
pub const DEP_PROTOC_VERSION_OTHER: &str = "25.0.0";

/// Tenderdash repository URL.
pub const TENDERDASH_REPO: &str = "https://github.com/dashpay/tenderdash";
Expand Down
95 changes: 64 additions & 31 deletions proto-compiler/src/functions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,11 @@ use std::{
path::{Path, PathBuf},
process::Command,
};
use semver::Version;

use walkdir::WalkDir;

use crate::constants::{GenerationMode, DEFAULT_TENDERDASH_COMMITISH, DEP_PROTOC_VERSION};
use crate::constants::{GenerationMode, DEFAULT_TENDERDASH_COMMITISH, DEP_PROTOC_VERSION_OTHER, DEP_PROTOC_VERSION_UBUNTU};

/// Check out a specific commitish of the tenderdash repository.
///
Expand Down Expand Up @@ -360,53 +361,85 @@ pub(crate) fn check_state(dir: &Path, commitish: &str) -> bool {
}
}

fn get_required_protoc_version() -> &'static str {
#[cfg(target_os = "linux")]
{
// Further refine detection if needed
// For example, detect if it's Ubuntu
DEP_PROTOC_VERSION_UBUNTU
}

#[cfg(not(target_os = "linux"))]
{
DEP_PROTOC_VERSION_OTHER
}
}
Comment on lines +367 to +379
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider refining OS detection to specifically identify Ubuntu

Currently, the get_required_protoc_version function uses #[cfg(target_os = "linux")] to determine if the operating system is Linux and then assumes it's Ubuntu by returning DEP_PROTOC_VERSION_UBUNTU. This may not be accurate for all Linux distributions, as different distributions may have different protoc versions. Consider implementing a more precise detection mechanism to specifically identify Ubuntu. This can help ensure that the correct required protoc version is used for the appropriate operating system.


/// Check if all dependencies are met
pub(crate) fn check_deps() -> Result<(), String> {
dep_protoc(DEP_PROTOC_VERSION).map(|_| ())
dep_protoc(get_required_protoc_version()).map(|_| ())
}

/// Check if protoc is installed and has the required version
fn dep_protoc(expected_version: f32) -> Result<f32, String> {
let protoc = prost_build::protoc_from_env();

// Run `protoc --version` and capture the output
let output = Command::new(protoc)
fn dep_protoc(required_version_str: &str) -> Result<Version, String> {
// Get the installed protoc version
let output = std::process::Command::new("protoc")
.arg("--version")
.output()
.map_err(|e| format!("failed to run: {}", e))?;

// Convert the output to a string
let out = output.stdout;
let version_output = String::from_utf8(out.clone())
.map_err(|e| format!("output {:?} is not utf8 string: {}", out, e))?;

// Extract the version number from string like `libprotoc 25.1`
let version: f32 = version_output
.split_whitespace()
.last()
.unwrap()
.parse()
.map_err(|e| format!("failed to parse protoc version {}: {}", version_output, e))?;

if version < expected_version {
.map_err(|e| format!("Failed to execute protoc: {}", e))?;

let version_output = String::from_utf8(output.stdout)
.map_err(|e| format!("Invalid UTF-8 output from protoc: {}", e))?;

// Extract the version number from the output
// Assuming the output is like "libprotoc 3.12.4"
let installed_version_str = version_output.trim().split_whitespace().nth(1)
.ok_or_else(|| "Failed to parse protoc version output".to_string())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Improve robustness of parsing protoc --version output

The current implementation assumes that the output of protoc --version is always in the format "libprotoc X.Y.Z". However, there might be variations in this output across different platforms or versions. To enhance robustness, consider verifying that the output starts with "libprotoc" before attempting to parse the version number. This will help prevent unexpected errors if the output format differs.

Apply this diff to improve the parsing logic:

 let installed_version_str = version_output.trim().split_whitespace().nth(1)
     .ok_or_else(|| "Failed to parse protoc version output".to_string())?;
+ if !version_output.trim().starts_with("libprotoc") {
+     return Err("Unexpected protoc version output format".to_string());
+ }

Committable suggestion was skipped due to low confidence.


// Parse the versions
let installed_version = Version::parse(&normalize_version(installed_version_str))
.map_err(|e| format!("Failed to parse installed protoc version '{}': {}", installed_version_str, e))?;

let required_version = Version::parse(required_version_str)
.map_err(|e| format!("Failed to parse required protoc version '{}': {}", required_version_str, e))?;

// Compare versions
if installed_version >= required_version {
Ok(installed_version)
} else {
Err(format!(
"protoc version must be {} or higher, but found {}; please upgrade: https://github.com/protocolbuffers/protobuf/releases/",
expected_version, version
"Installed protoc version {} is less than required version {}",
installed_version, required_version
))
} else {
Ok(version)
}
}

fn normalize_version(version_str: &str) -> String {
let mut parts: Vec<&str> = version_str.split('.').collect();
while parts.len() < 3 {
parts.push("0");
}
parts.join(".")
}
Comment on lines +431 to +437
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle pre-release and build metadata in version strings

The normalize_version function pads the version string to ensure it has three components. However, it does not account for pre-release identifiers or build metadata (e.g., "3.12.4-beta" or "3.12.4+meta"), which are valid in semantic versioning. This could lead to incorrect parsing or version comparisons. Consider updating the function to handle these cases or using Version::parse directly without normalization, as semver can parse incomplete versions and handle pre-release/build metadata appropriately.

Apply this diff to remove the normalization and rely on Version::parse:

-fn normalize_version(version_str: &str) -> String {
-    let mut parts: Vec<&str> = version_str.split('.').collect();
-    while parts.len() < 3 {
-        parts.push("0");
-    }
-    parts.join(".")
-}

Update the version parsing in dep_protoc:

 let installed_version = Version::parse(installed_version_str)
     .map_err(|e| format!("Failed to parse installed protoc version '{}': {}", installed_version_str, e))?;

-let required_version = Version::parse(required_version_str)
+let required_version = Version::parse(&normalize_version(required_version_str))
     .map_err(|e| format!("Failed to parse required protoc version '{}': {}", required_version_str, e))?;

Committable suggestion was skipped due to low confidence.

#[cfg(test)]
mod tests {
use super::*;

#[test]
fn test_protoc_dep() {
let expected_versions = vec![(10.1, true), (DEP_PROTOC_VERSION, true), (90.5, false)];
for expect in expected_versions {
assert_eq!(dep_protoc(expect.0).is_ok(), expect.1);
let expected_versions = vec![
("10.1.0", true),
(DEP_PROTOC_VERSION_OTHER, true),
("90.5.0", false),
];
for &(required_version, expected_result) in &expected_versions {
let result = dep_protoc(required_version);
assert_eq!(
result.is_ok(),
expected_result,
"Test case failed for required_version='{}', error='{:?}'",
required_version,
result.err()
);
Comment on lines +444 to +457
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

Unit tests depend on the system's installed protoc version

The test_protoc_dep function relies on the actual protoc version installed on the system where the tests are executed. This can lead to inconsistent test results across different environments, making the tests unreliable. To ensure consistent and reliable tests, consider mocking the calls to protoc or abstracting the system interactions so that you can inject test responses without depending on the environment's protoc installation.

For example, you can refactor dep_protoc to accept a function parameter that simulates the command output, allowing you to inject mock outputs during testing.

-fn dep_protoc(required_version_str: &str) -> Result<Version, String> {
+fn dep_protoc<F>(required_version_str: &str, get_protoc_version_output: F) -> Result<Version, String>
+    where F: Fn() -> Result<String, String>
{
-    let output = std::process::Command::new("protoc")
-        .arg("--version")
-        .output()
-        .map_err(|e| format!("Failed to execute protoc: {}", e))?;
-
-    let version_output = String::from_utf8(output.stdout)
-        .map_err(|e| format!("Invalid UTF-8 output from protoc: {}", e))?;
+    let version_output = get_protoc_version_output()?;

    // Parsing logic remains the same
}

Then, in your tests, you can provide a mock function:

#[test]
fn test_protoc_dep() {
    let expected_versions = vec![
        ("10.1.0", true),
        (DEP_PROTOC_VERSION_OTHER, true),
        ("90.5.0", false),
    ];
    for &(required_version, expected_result) in &expected_versions {
        let mock_output = Ok("libprotoc 28.0.0".to_string());
        let result = dep_protoc(required_version, |_| mock_output.clone());
        assert_eq!(
            result.is_ok(),
            expected_result,
            "Test case failed for required_version='{}', error='{:?}'",
            required_version,
            result.err()
        );
    }
}

}
}
}
Loading