avoid recompiling project

[marcospb19-cw]

changes build.rs to avoid recompilation

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the PR involves changes to the build process which can have wide-ranging implications on the project's build and deployment pipeline. The changes are moderate in size but critical in nature, requiring careful review to ensure they don't introduce new issues.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The comparison in update_file_if_contents_changed uses == to compare byte arrays, which might not work as expected if the file read operation returns a Result type. This could lead to unnecessary file writes or missed updates.
🔒 Security concerns	No

Code feedback:

---

relevant file	build.rs
suggestion	Consider using Result::as_ref combined with Option::map to safely compare file contents in update_file_if_contents_changed. This ensures that the comparison is done on the actual byte content rather than on the Result or Option wrappers. [important]
relevant line	if current_file_contents.is_ok_and(|contents| contents == new_contents.as_bytes()) {

---

relevant file	build.rs
suggestion	Add error handling for the glob function in generate_signature_maps_file to prevent panics and provide a more robust error message, improving the maintainability and reliability of the build script. [important]
relevant line	let signature_files: Vec = glob(SIGNATURES_DIR)

---

relevant file	build.rs
suggestion	Replace the panic in update_file_if_contents_changed with a more controlled error handling strategy, such as logging the error and continuing with the build process. This can prevent the build process from stopping unexpectedly due to non-critical errors. [important]
relevant line	panic!("failed to write to file {path:?}: reason={err:?}");

---

relevant file	build.rs
suggestion	Use a more efficient method for file comparison in update_file_if_contents_changed, such as computing and comparing hashes of the contents, which can be faster for large files and reduce I/O operations. [medium]
relevant line	if current_file_contents.is_ok_and(|contents| contents == new_contents.as_bytes()) {

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Error handling	Use error handling instead of panicking when listing files fails Instead of using expect which panics, consider using a match statement to handle the error gracefully when listing signature files fails. This could log the error and continue, or handle it in another appropriate way. build.rs [41] -glob(SIGNATURES_DIR).expect("Listing signature files should not fail") +match glob(SIGNATURES_DIR) { + Ok(files) => files, + Err(e) => { + eprintln!("Failed to list signature files: {:?}", e); + return; + } +} Suggestion importance[1-10]: 9 Why: This suggestion addresses error handling by preventing a panic and allowing for a more graceful failure management. This is crucial for robustness and reliability.	9
	Handle potential errors from glob without panicking Add error handling for the glob function in generate_signature_maps_file to prevent panics and allow for cleaner failure management. build.rs [41] -let signature_files: Vec<PathBuf> = glob(SIGNATURES_DIR).expect("Listing signature files should not fail") +let signature_files: Vec<PathBuf> = match glob(SIGNATURES_DIR) { + Ok(files) => files.collect(), + Err(e) => { + eprintln!("Error listing signature files: {:?}", e); + Vec::new() // Continue with an empty list or handle differently + } +} Suggestion importance[1-10]: 9 Why: This suggestion enhances error handling by preventing a panic and allowing for cleaner failure management, which is crucial for the robustness of the build script.	9
Maintainability	Improve error handling for missing OUT_DIR environment variable Consider handling the case where env::var_os("OUT_DIR") returns None more gracefully instead of panicking directly. This can improve the robustness of the build script by providing a clearer error message or a fallback mechanism. build.rs [128] -env::var_os("OUT_DIR").map(PathBuf::from).expect("Compiler should set OUT_DIR") +env::var_os("OUT_DIR").map(PathBuf::from).unwrap_or_else(|| panic!("OUT_DIR environment variable is not set. Please ensure the compiler is configured correctly.")) Suggestion importance[1-10]: 8 Why: The suggestion improves error handling by providing a clearer error message, which enhances the robustness of the build script. This is a significant improvement in maintainability.	8
	Simplify logic in update_file_if_contents_changed by using early return Refactor the update_file_if_contents_changed function to reduce nesting by using early return for the unchanged content case. build.rs [115-121] if current_file_contents.is_ok_and(|contents| contents == new_contents.as_bytes()) { - // do nothing, file is already written - // by not writting it again, we avoid retriggering compilation -} else { - // update it - if let Err(err) = fs::write(&path, new_contents) { - panic!("failed to write to file {path:?}: reason={err:?}"); - } + return; // No changes needed +} +if let Err(err) = fs::write(&path, new_contents) { + panic!("failed to write to file {path:?}: reason={err:?}"); } Suggestion importance[1-10]: 7 Why: The suggestion simplifies the logic and improves readability by reducing nesting, which is a good practice for maintainability. However, it is a minor improvement compared to error handling.	7

[marcospb19-cw]

Argh, from my tests locally it seems like this is actually not necessary, as cargo doesn't look at OUT_DIR for retriggering compilation, therefore, I guess I'll just remove this