-
Notifications
You must be signed in to change notification settings - Fork 122
[orion-server]: auto targets generation for buck2 #1312
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| BUILD_LOG_DIR="/tmp/megadir/buck2ctl" | ||
| DATABASE_URL="postgres://postgres:postgres@localhost/orion" | ||
| PORT=80 | ||
| PORT=80 | ||
| MONOBASE_URL="http://git.gitmega.com" |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::buck2::download_and_get_buck2_targets; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::model::builds; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use axum::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Json, Router, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -61,7 +62,8 @@ fn create_log_file(task_id: &str) -> Result<std::fs::File, std::io::Error> { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[derive(Debug, Deserialize, ToSchema)] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub struct BuildRequest { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| target: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buck_hash: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buckconfig_hash: String, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
+66
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| buck_hash: String, | |
| buckconfig_hash: String, | |
| buck_file_hash: String, | |
| buckconfig_file_hash: String, |
Copilot
AI
Aug 6, 2025
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.
The buck_hash and buckconfig_hash parameters are not validated before being used to download files. Consider validating these are proper hash formats to prevent potential security issues.
Copilot
AI
Aug 6, 2025
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.
The buck2 target generation happens synchronously in the request handler, which could cause request timeouts for large projects. Consider moving this to an async background task or implementing a timeout mechanism.
| // Download and get buck2 targets first | |
| let target = match download_and_get_buck2_targets(&req.buck_hash, &req.buckconfig_hash).await { | |
| Ok(target) => target, | |
| Err(e) => { | |
| tracing::error!("Failed to download buck2 targets: {}", e); | |
| return ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| Json(serde_json::json!({ "message": format!("Failed to download buck2 targets: {}", e) })), | |
| ).into_response(); | |
| } | |
| // Download and get buck2 targets first, with a timeout | |
| let buck2_timeout = Duration::from_secs(30); | |
| let target = match timeout( | |
| buck2_timeout, | |
| download_and_get_buck2_targets(&req.buck_hash, &req.buckconfig_hash) | |
| ).await { | |
| Ok(Ok(target)) => target, | |
| Ok(Err(e)) => { | |
| tracing::error!("Failed to download buck2 targets: {}", e); | |
| return ( | |
| StatusCode::INTERNAL_SERVER_ERROR, | |
| Json(serde_json::json!({ "message": format!("Failed to download buck2 targets: {}", e) })), | |
| ).into_response(); | |
| } | |
| Err(_) => { | |
| tracing::error!("Timeout while downloading buck2 targets"); | |
| return ( | |
| StatusCode::REQUEST_TIMEOUT, | |
| Json(serde_json::json!({ "message": "Timeout while downloading buck2 targets" })), | |
| ).into_response(); | |
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,174 @@ | ||||||||||||||||||||||||||||
| use std::process::Command; | ||||||||||||||||||||||||||||
| use std::fs; | ||||||||||||||||||||||||||||
| use std::path::Path; | ||||||||||||||||||||||||||||
| use tokio_retry::{strategy::ExponentialBackoff, Retry}; | ||||||||||||||||||||||||||||
| use uuid::Uuid; | ||||||||||||||||||||||||||||
| use std::time::Duration; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Download files from file blob API using two hash values and save them to a new folder in tmp directory | ||||||||||||||||||||||||||||
| async fn download_files_to_tmp(hash1: &str, hash2: &str) -> Result<String, Box<dyn std::error::Error>> { | ||||||||||||||||||||||||||||
| // Create tmp directory path | ||||||||||||||||||||||||||||
| // Generate a unique folder name using UUID | ||||||||||||||||||||||||||||
| let folder_name = Uuid::now_v7().to_string(); | ||||||||||||||||||||||||||||
| println!("Generated folder name: {folder_name},buck:{hash1},.buckconfig:{hash2}"); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Create tmp directory path | ||||||||||||||||||||||||||||
| let tmp_dir = std::env::temp_dir().join(folder_name); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| fs::create_dir_all(&tmp_dir)?; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Download first file as BUCK | ||||||||||||||||||||||||||||
| let buck_path = tmp_dir.join("BUCK"); | ||||||||||||||||||||||||||||
| download_file_with_retry(hash1, &buck_path, 3).await?; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Download second file as .buckconfig | ||||||||||||||||||||||||||||
| let buckconfig_path = tmp_dir.join(".buckconfig"); | ||||||||||||||||||||||||||||
| download_file_with_retry(hash2, &buckconfig_path, 3).await?; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Ok(tmp_dir.to_string_lossy().to_string()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Download a single file with retry mechanism using tokio-retry | ||||||||||||||||||||||||||||
| async fn download_file_with_retry( | ||||||||||||||||||||||||||||
| hash: &str, | ||||||||||||||||||||||||||||
| file_path: &Path, | ||||||||||||||||||||||||||||
| max_retries: usize | ||||||||||||||||||||||||||||
| ) -> Result<(), Box<dyn std::error::Error>> { | ||||||||||||||||||||||||||||
| let api_endpoint = file_blob_endpoint(); | ||||||||||||||||||||||||||||
| let url = format!("{api_endpoint}/{hash}"); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| // Create retry strategy: exponential backoff starting from 100ms, max 3 attempts | ||||||||||||||||||||||||||||
| let retry_strategy = ExponentialBackoff::from_millis(100) | ||||||||||||||||||||||||||||
| .max_delay(Duration::from_secs(2)) | ||||||||||||||||||||||||||||
| .take(max_retries); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Retry::spawn(retry_strategy, || { | ||||||||||||||||||||||||||||
| download_single_file(&url, file_path) | ||||||||||||||||||||||||||||
| }) | ||||||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||||||
| .map_err(|e| format!("Failed to download {hash} after {max_retries} attempts: {e}").into()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Download a single file from URL and save to specified path | ||||||||||||||||||||||||||||
| async fn download_single_file(url: &str, file_path: &Path) -> Result<(), Box<dyn std::error::Error>> { | ||||||||||||||||||||||||||||
| let response = reqwest::get(url).await?; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if !response.status().is_success() { | ||||||||||||||||||||||||||||
| return Err(format!("HTTP error: {}", response.status()).into()); | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| let content = response.bytes().await?; | ||||||||||||||||||||||||||||
| fs::write(file_path, &content)?; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Ok(()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Get the base URL for API requests | ||||||||||||||||||||||||||||
| fn base_url() -> String { | ||||||||||||||||||||||||||||
| std::env::var("MONOBASE_URL").unwrap_or_else(|_| "http://localhost:8000".to_string()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Get the file blob API endpoint | ||||||||||||||||||||||||||||
| pub fn file_blob_endpoint() -> String { | ||||||||||||||||||||||||||||
| format!("{}/api/v1/file/blob", base_url()) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| /// Execute buck2 targets //... command in the given directory and return the last line string | ||||||||||||||||||||||||||||
| pub fn get_buck2_targets_last_line(directory: &str) -> Result<String, Box<dyn std::error::Error>> { | ||||||||||||||||||||||||||||
|
Comment on lines
+78
to
+79
|
||||||||||||||||||||||||||||
| /// Execute buck2 targets //... command in the given directory and return the last line string | |
| pub fn get_buck2_targets_last_line(directory: &str) -> Result<String, Box<dyn std::error::Error>> { | |
| /// Execute `buck2 targets //...` in the given directory and return all target lines as a Vec<String> | |
| pub fn get_buck2_targets(directory: &str) -> Result<Vec<String>, Box<dyn std::error::Error>> { |
Copilot
AI
Aug 6, 2025
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.
Executing buck2 command without validating the directory path could be a security risk. Consider validating that the directory is within expected bounds and doesn't contain malicious paths.
| let output = Command::new("buck2") | |
| .args(["targets", "//..."]) | |
| .current_dir(directory) | |
| // Validate that the directory is within the system temp directory | |
| let tmp_dir = std::env::temp_dir().canonicalize()?; | |
| let dir_path = Path::new(directory).canonicalize()?; | |
| if !dir_path.starts_with(&tmp_dir) { | |
| return Err("Provided directory is outside the allowed temporary directory".into()); | |
| } | |
| let output = Command::new("buck2") | |
| .args(["targets", "//..."]) | |
| .current_dir(&dir_path) |
Copilot
AI
Aug 6, 2025
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.
Remove extra space before the closing parenthesis in the format! macro call.
| return Err(format!("buck2 command failed: {stderr}" ).into()); | |
| return Err(format!("buck2 command failed: {stderr}").into()); |
Copilot
AI
Aug 6, 2025
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.
The TODO comment identifies a legitimate issue. The current cleanup logic can fail to execute if the buck2 command panics or if there's an early return. Consider implementing a proper RAII guard using a struct with Drop trait or using the tempfile crate's TempDir.
Copilot
AI
Aug 6, 2025
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.
Hard-coded relative path './test' makes the test fragile and dependent on current working directory. Consider using a more robust approach or creating test fixtures within the test itself.
| let test_dir = "./test"; | |
| if Path::new(test_dir).exists() { | |
| let test_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join("test"); | |
| if test_dir.exists() { |
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.
[nitpick] The field name 'buck_hash' is ambiguous. Consider renaming to 'buck_file_hash' or 'buck_build_file_hash' to clearly indicate this is the hash for the BUCK build file.