-
Notifications
You must be signed in to change notification settings - Fork 39
fix: krane using lazy_static! caused uncleaned temp files #564
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
base: develop
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,60 +1,106 @@ | ||
| use anyhow::Result; | ||
| use include_env_compressed::{include_archive_from_env, Archive}; | ||
| use std::fs::{File, Permissions}; | ||
| use sha2::{Digest, Sha256}; | ||
| use std::fs; | ||
| use std::os::unix::fs::PermissionsExt; | ||
| use std::path::PathBuf; | ||
|
|
||
| use tempfile::TempDir; | ||
|
|
||
| pub const KRANE_BIN: Archive = include_archive_from_env!("KRANE_PATH"); | ||
|
|
||
| lazy_static::lazy_static! { | ||
| pub static ref KRANE: Krane = Krane::seal().unwrap(); | ||
| fn expected_checksum() -> String { | ||
| let mut hasher = Sha256::new(); | ||
| let mut reader = KRANE_BIN.reader(); | ||
| std::io::copy(&mut reader, &mut hasher).unwrap(); | ||
| format!("{:x}", hasher.finalize()) | ||
| } | ||
|
|
||
| #[derive(Debug)] | ||
| pub struct Krane { | ||
| // Hold the file in memory to keep the fd open | ||
| _tmp_dir: TempDir, | ||
| path: PathBuf, | ||
| fn file_checksum(path: &std::path::Path) -> Result<String> { | ||
| let mut hasher = Sha256::new(); | ||
| let mut file = fs::File::open(path)?; | ||
| std::io::copy(&mut file, &mut hasher)?; | ||
| Ok(format!("{:x}", hasher.finalize())) | ||
| } | ||
|
|
||
| impl Krane { | ||
| fn seal() -> Result<Krane> { | ||
| let tmp_dir = TempDir::new()?; | ||
| let path = tmp_dir.path().join("krane"); | ||
| pub fn ensure_krane_in_dir(tools_dir: impl AsRef<std::path::Path>) -> Result<std::path::PathBuf> { | ||
| let tools_dir = tools_dir.as_ref(); | ||
| let krane_path = tools_dir.join("krane"); | ||
|
|
||
| let mut krane_file = File::create(&path)?; | ||
| let permissions = Permissions::from_mode(0o755); | ||
| krane_file.set_permissions(permissions)?; | ||
| let needs_update = if krane_path.exists() { | ||
| match file_checksum(&krane_path) { | ||
| Ok(actual) => actual != expected_checksum(), | ||
| Err(_) => true, // If we can't read it, assume it needs update | ||
| } | ||
| } else { | ||
| true | ||
| }; | ||
|
|
||
| let mut krane_reader = KRANE_BIN.reader(); | ||
| if needs_update { | ||
| fs::create_dir_all(tools_dir)?; | ||
|
|
||
| std::io::copy(&mut krane_reader, &mut krane_file)?; | ||
| let temp_path = tools_dir.join(format!(".krane.tmp.{}", std::process::id())); | ||
|
|
||
| Ok(Krane { | ||
| _tmp_dir: tmp_dir, | ||
| path, | ||
| }) | ||
| } | ||
| { | ||
| let mut temp_file = fs::File::create(&temp_path)?; | ||
| let permissions = fs::Permissions::from_mode(0o755); | ||
| temp_file.set_permissions(permissions)?; | ||
|
|
||
| let mut krane_reader = KRANE_BIN.reader(); | ||
| std::io::copy(&mut krane_reader, &mut temp_file)?; | ||
|
|
||
| use std::io::Write; | ||
| temp_file.flush()?; | ||
| } | ||
|
|
||
| pub fn path(&self) -> &PathBuf { | ||
| &self.path | ||
| fs::rename(temp_path, &krane_path)?; | ||
| } | ||
|
|
||
| Ok(krane_path) | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod test { | ||
| use super::*; | ||
| use std::process::Command; | ||
|
|
||
| #[test] | ||
| fn test_krane_runs() { | ||
| let status = Command::new(KRANE.path()) | ||
| fn test_checksum_functions() { | ||
| let checksum1 = expected_checksum(); | ||
| let checksum2 = expected_checksum(); | ||
| assert_eq!(checksum1, checksum2); | ||
| assert!(!checksum1.is_empty()); | ||
| println!("Expected checksum: {}", checksum1); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_ensure_krane_in_dir() { | ||
| use tempfile::TempDir; | ||
|
|
||
| let temp_dir = TempDir::new().unwrap(); | ||
| println!("Testing ensure_krane_in_dir in: {:?}", temp_dir.path()); | ||
|
|
||
| let path1 = crate::ensure_krane_in_dir(temp_dir.path()).unwrap(); | ||
| assert!(path1.exists()); | ||
| assert!(path1.ends_with("krane")); | ||
| println!("Created krane at: {:?}", path1); | ||
|
|
||
| let metadata = std::fs::metadata(&path1).unwrap(); | ||
| let permissions = metadata.permissions(); | ||
| assert!(permissions.mode() & 0o111 != 0); | ||
| println!("File is executable: {}", permissions.mode() & 0o111 != 0); | ||
|
|
||
| let mtime1 = std::fs::metadata(&path1).unwrap().modified().unwrap(); | ||
| std::thread::sleep(std::time::Duration::from_millis(10)); | ||
|
|
||
| let path2 = crate::ensure_krane_in_dir(temp_dir.path()).unwrap(); | ||
| let mtime2 = std::fs::metadata(&path2).unwrap().modified().unwrap(); | ||
|
|
||
| assert_eq!(path1, path2); | ||
| assert_eq!(mtime1, mtime2); | ||
| println!("File was reused (same mtime): {}", mtime1 == mtime2); | ||
|
|
||
| let status = std::process::Command::new(&path1) | ||
| .arg("--help") | ||
| .output() | ||
| .expect("failed to run krane"); | ||
|
|
||
| assert_eq!(status.status.code().unwrap(), 0); | ||
| println!("Krane binary works correctly"); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,7 +17,7 @@ use std::{collections::HashMap, path::Path}; | |
| use async_trait::async_trait; | ||
| use cli::CommandLine; | ||
| use crane::CraneCLI; | ||
| use krane_bundle::KRANE; | ||
| use krane_bundle::ensure_krane_in_dir; | ||
| use olpc_cjson::CanonicalFormatter; | ||
| use serde::{Deserialize, Serialize}; | ||
| use snafu::ResultExt; | ||
|
|
@@ -31,12 +31,12 @@ pub struct ImageTool { | |
| } | ||
|
|
||
| impl ImageTool { | ||
| /// Uses the builtin `krane` provided by the `tools/krane` crate. | ||
| pub fn from_builtin_krane() -> Self { | ||
| let tools_dir = std::path::Path::new("./build/tools"); | ||
|
||
| let krane_path = ensure_krane_in_dir(tools_dir).expect("Failed to setup krane binary"); | ||
|
|
||
| let image_tool_impl = Box::new(CraneCLI { | ||
| cli: CommandLine { | ||
| path: KRANE.path().to_path_buf(), | ||
| }, | ||
| cli: CommandLine { path: krane_path }, | ||
| }); | ||
| Self { image_tool_impl } | ||
| } | ||
|
|
||
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.
is there some reason this
usestatement is mixed in with the rest of the code? I tend to prefer to putusestatements at the beginning of the block that is using it to make it clear where it applies, since it will apply to the whole blockThere 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.
Yes this should be moved up.