fix: Compilation fix under Ubuntu #109
Conversation
WalkthroughThe changes introduce a new dependency on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
proto-compiler/src/constants.rs (1)
4-5: Improved version handling, but consider future maintainabilityThe change from a single
f32constant to two&strconstants for different platforms is a good step towards addressing the compilation issues on Ubuntu. This allows for more precise version specification and accommodates the different versioning schemes used by Ubuntu and other platforms.However, there are a few points to consider:
Maintainability: Having separate constants for different platforms might lead to maintenance overhead in the future. Consider using a more dynamic approach to determine the required version based on the current platform.
Flexibility: The current implementation assumes only two categories (Ubuntu and Others). This might not be flexible enough if other platforms require different versions in the future.
To improve maintainability and flexibility, consider the following suggestions:
Use a configuration file or environment variables to specify version requirements for different platforms. This would allow easier updates without changing the code.
Implement a function that determines the required version based on the current platform, possibly using the
cfgattributes for conditional compilation. For example:pub fn get_required_protoc_version() -> &'static str { #[cfg(target_os = "linux")] { if is_ubuntu() { "3.12.4" } else { "25.0.0" } } #[cfg(not(target_os = "linux"))] { "25.0.0" } } #[cfg(target_os = "linux")] fn is_ubuntu() -> bool { // Implement logic to detect Ubuntu // This could involve checking /etc/os-release or using a crate like `os_info` todo!() }This approach would centralize version management and make it easier to add support for other platforms in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- proto-compiler/Cargo.toml (1 hunks)
- proto-compiler/src/constants.rs (1 hunks)
- proto-compiler/src/functions.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (1)
proto-compiler/Cargo.toml (1)
24-24: LGTM: Addition of semver dependency aligns with PR objectives.The inclusion of the
semvercrate (version 1.0) is a good choice for improving version comparison capabilities. This directly supports the PR's goal of addressing the compilation issue on Ubuntu by enabling more robust handling of "x.x.x" version formats.
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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() | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
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()
);
}
}
proto-compiler/src/functions.rs
Outdated
| let installed_version_str = version_output.trim().split_whitespace().nth(1) | ||
| .ok_or_else(|| "Failed to parse protoc version output".to_string())?; |
There was a problem hiding this comment.
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.
| 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(".") | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
proto-compiler/src/functions.rs (3)
367-379: LGTM: OS-specific version requirement implementedThe
get_required_protoc_versionfunction successfully implements the PR objective of splitting the minimum version requirement based on the target OS. This is a good approach to handle potential differences between Linux (assumed to be Ubuntu) and other systems.However, consider the following improvement:
As noted in the comment, you might want to further refine the Linux detection to specifically identify Ubuntu. This could be done using the
std::env::consts::OSand additional system information. For example:#[cfg(target_os = "linux")] { // Check if it's specifically Ubuntu if let Ok(os_release) = std::fs::read_to_string("/etc/os-release") { if os_release.contains("Ubuntu") { return DEP_PROTOC_VERSION_UBUNTU; } } // Default to OTHER for non-Ubuntu Linux DEP_PROTOC_VERSION_OTHER }This would provide more accurate version requirements for different Linux distributions.
386-428: LGTM: Improved version checking with semantic versioningThe modifications to
dep_protocsuccessfully implement semantic versioning for more accurate and robust version comparisons. This addresses the PR objective of changing the version check from float to string format.A minor suggestion for improvement:
Consider adding a check for the "libprotoc" prefix in the version output to ensure the parsing is correct. You can add this check right after parsing the version string:
let version_output = String::from_utf8(output.stdout) .map_err(|e| format!("Invalid UTF-8 output from protoc: {}", e))?; if !version_output.trim().starts_with("libprotoc") { return Err("Unexpected protoc version output format".to_string()); } // Rest of the function remains the sameThis additional check will make the function more robust against unexpected
protocoutput formats.
431-457: LGTM: Version normalization and updated testsThe addition of the
normalize_versionfunction and the updates to the test case are good improvements. They ensure consistent version formatting and properly test the new string-based version comparison.A suggestion for improvement:
The
normalize_versionfunction currently doesn't handle pre-release versions or build metadata (e.g., "3.14.0-beta" or "3.14.0+build.1"). Consider updating it to preserve this information:fn normalize_version(version_str: &str) -> String { let mut parts: Vec<&str> = version_str.split(|c| c == '.' || c == '-' || c == '+').collect(); let mut result = String::new(); // Normalize the main version components for i in 0..3 { if i > 0 { result.push('.'); } result.push_str(parts.get(i).unwrap_or(&"0")); } // Append pre-release version if present if let Some(pre_release) = version_str.split('-').nth(1) { result.push('-'); result.push_str(pre_release); } // Append build metadata if present if let Some(build_meta) = version_str.split('+').nth(1) { result.push('+'); result.push_str(build_meta); } result }This will ensure that the function handles a wider range of valid semantic version strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- proto-compiler/src/functions.rs (2 hunks)
🧰 Additional context used
🔇 Additional comments (3)
proto-compiler/src/functions.rs (3)
9-9: LGTM: Good use of semantic versioningThe addition of the
semvercrate is a good choice for handling version comparisons. This will provide more robust and standardized version handling throughout the code.
12-15: LGTM: Added new constant for Ubuntu-specific versionThe addition of
DEP_PROTOC_VERSION_UBUNTUallows for more flexible version requirements based on the operating system. This is a good approach to handle potential differences between Ubuntu and other systems.
Line range hint
1-457: Overall assessment: Good improvements with minor suggestionsThe changes in this file successfully address the PR objectives by implementing OS-specific version requirements and improving the version comparison logic using semantic versioning. The code is now more robust and flexible.
Key improvements:
- OS-specific protoc version requirements
- Use of semantic versioning for accurate comparisons
- More robust error handling in version parsing and comparison
- Consistent version normalization
The suggestions provided in the review comments, if implemented, would further enhance the code's robustness and flexibility. Great work overall!
|
I don't think we want to support Ubuntu-shipped versions of protoc, as these might be missing some features (like optional fields). I would suggest to refactor this PR to check installed version of protoc, and if it's not correct, display informative error message like 'Your version of protoc is not supported. Please install protoc version {} or above from https://github.com/protocolbuffers/protobuf/releases`. |
Issue being fixed or feature implemented
Compilation under Ubuntu was failing.
This PR fixes issue #108.
Originally the code was checking minimum protobuf version as a float.
In Ubuntu although, the version was in the major, minor, patch format.
What was done?
How Has This Been Tested?
Compiled locally in macOS and in an Ubuntu 22 VM.
Breaking Changes
no
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
semverlibrary.protocversion based on the operating system.Changes
Bug Fixes
protocversion checks to ensure compatibility across different environments.