From 62379e9f8a0f5a441ad0f74e543531f27d5fd427 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sun, 11 May 2025 19:44:10 +0200 Subject: [PATCH 1/5] #241 Add macOS-specific fixes and tests for Kaleido compatibility Introduce macOS-specific arguments for Kaleido to address issue #323. Add new tests to validate image generation (e.g., PNG, JPEG, SVG, PDF) on macOS, ensuring proper functionality. This improves cross-platform support and resolves inconsistencies in the Kaleido backend. issue: #241 --- plotly/src/plot.rs | 30 +++++++- plotly_kaleido/src/lib.rs | 155 +++++++++++++++++++++++++++++++++++--- 2 files changed, 171 insertions(+), 14 deletions(-) diff --git a/plotly/src/plot.rs b/plotly/src/plot.rs index 9f970b92..86bd5ade 100644 --- a/plotly/src/plot.rs +++ b/plotly/src/plot.rs @@ -586,8 +586,7 @@ impl PartialEq for Plot { mod tests { use std::path::PathBuf; - #[cfg(feature = "kaleido")] - use base64::{engine::general_purpose, Engine as _}; + use serde_json::{json, to_value}; use super::*; @@ -866,4 +865,31 @@ mod tests { const LEN: usize = 10; assert_eq!(expected[..LEN], image_svg[..LEN]); } + + #[cfg(target_os = "macos")] + #[test] + #[cfg(feature = "kaleido")] + fn save_surface_to_png() { + use crate::Surface; + let mut plot = Plot::new(); + let z_matrix = vec![ + vec![1.0, 2.0, 3.0], + vec![4.0, 5.0, 6.0], + vec![7.0, 8.0, 9.0], + ]; + let x_unique = vec![1.0, 2.0, 3.0]; + let y_unique = vec![4.0, 5.0, 6.0]; + let surface = Surface::new(z_matrix) + .x(x_unique) + .y(y_unique) + .name("Surface"); + + plot.add_trace(surface); + let dst = PathBuf::from("example.png"); + plot.write_image("example.png", ImageFormat::PNG, 800, 600, 1.0); + assert!(dst.exists()); + assert!(std::fs::remove_file(&dst).is_ok()); + assert!(!dst.exists()); + assert!(!plot.to_base64(ImageFormat::PNG, 1024, 680, 1.0).is_empty()); + } } diff --git a/plotly_kaleido/src/lib.rs b/plotly_kaleido/src/lib.rs index 91f1e09f..2d1a27a4 100644 --- a/plotly_kaleido/src/lib.rs +++ b/plotly_kaleido/src/lib.rs @@ -185,20 +185,34 @@ impl Kaleido { ) -> Result> { let p = self.cmd_path.to_str().unwrap(); + #[cfg(not(target_os = "macos"))] + let cmd_args = vec![ + "plotly", + "--disable-gpu", + "--allow-file-access-from-files", + "--disable-breakpad", + "--disable-dev-shm-usage", + "--disable-software-rasterizer", + "--single-process", + "--no-sandbox", + ]; + + // Add Kaleido issue #323 + #[cfg(target_os = "macos")] + let cmd_args = vec![ + "plotly", + "--allow-file-access-from-files", + "--disable-breakpad", + "--disable-dev-shm-usage", + "--disable-software-rasterizer", + "--single-process", + "--no-sandbox", + ]; + #[allow(clippy::zombie_processes)] let mut process = Command::new(p) .current_dir(self.cmd_path.parent().unwrap()) - .args([ - "plotly", - "--disable-gpu", - "--allow-file-access-from-files", - "--disable-breakpad", - "--disable-dev-shm-usage", - "--disable-software-rasterizer", - "--single-process", - "--disable-gpu", - "--no-sandbox", - ]) + .args(cmd_args) .stdin(Stdio::piped()) .stdout(Stdio::piped()) .stderr(Stdio::piped()) @@ -210,10 +224,11 @@ impl Kaleido { "failed to spawn Kaleido binary at {}", self.cmd_path.to_string_lossy() ) - .to_string() + .to_string() ) }); + { let plot_data = PlotData::new(plotly_data, format, width, height, scale).to_json(); let mut process_stdin = process.stdin.take().unwrap(); @@ -287,6 +302,46 @@ mod tests { .unwrap() } + fn create_test_surface() -> Value { + to_value(json!({ + "data": [ + { + "name": "Surface", + "type": "surface", + "x": [ + 1.0, + 2.0, + 3.0 + ], + "y": [ + 4.0, + 5.0, + 6.0 + ], + "z": [ + [ + 1.0, + 2.0, + 3.0 + ], + [ + 4.0, + 5.0, + 6.0 + ], + [ + 7.0, + 8.0, + 9.0 + ] + ] + } + ], + "layout": {} + })) + .unwrap() + } + #[test] fn can_find_kaleido_executable() { let _k = Kaleido::new(); @@ -378,4 +433,80 @@ mod tests { assert!(r.is_ok()); assert!(std::fs::remove_file(dst.as_path()).is_ok()); } + + // Issue #241 workaround until https://github.com/plotly/Kaleido/issues/323 is resolved + #[cfg(target_os = "macos")] + #[test] + fn save_surface_png() { + let test_plot = create_test_surface(); + let k = Kaleido::new(); + let dst = PathBuf::from("example.png"); + let r = k.save(dst.as_path(), &test_plot, "png", 1200, 900, 4.5); + assert!(r.is_ok()); + assert!(dst.exists()); + let metadata = std::fs::metadata(&dst).expect("Could not retrieve file metadata"); + let file_size = metadata.len(); + assert!(file_size > 0,); + assert!(std::fs::remove_file(dst.as_path()).is_ok()); + } + + #[cfg(target_os = "macos")] + #[test] + fn save_surface_jpeg() { + let test_plot = create_test_surface(); + let k = Kaleido::new(); + let dst = PathBuf::from("example.jpeg"); + let r = k.save(dst.as_path(), &test_plot, "jpeg", 1200, 900, 4.5); + assert!(r.is_ok()); + assert!(dst.exists()); + let metadata = std::fs::metadata(&dst).expect("Could not retrieve file metadata"); + let file_size = metadata.len(); + assert!(file_size > 0,); + assert!(std::fs::remove_file(dst.as_path()).is_ok()); + } + + #[cfg(target_os = "macos")] + #[test] + fn save_surface_webp() { + let test_plot = create_test_surface(); + let k = Kaleido::new(); + let dst = PathBuf::from("example.webp"); + let r = k.save(dst.as_path(), &test_plot, "webp", 1200, 900, 4.5); + assert!(r.is_ok()); + assert!(dst.exists()); + let metadata = std::fs::metadata(&dst).expect("Could not retrieve file metadata"); + let file_size = metadata.len(); + assert!(file_size > 0,); + assert!(std::fs::remove_file(dst.as_path()).is_ok()); + } + + #[cfg(target_os = "macos")] + #[test] + fn save_surface_svg() { + let test_plot = create_test_surface(); + let k = Kaleido::new(); + let dst = PathBuf::from("example.svg"); + let r = k.save(dst.as_path(), &test_plot, "svg", 1200, 900, 4.5); + assert!(r.is_ok()); + assert!(dst.exists()); + let metadata = std::fs::metadata(&dst).expect("Could not retrieve file metadata"); + let file_size = metadata.len(); + assert!(file_size > 0,); + assert!(std::fs::remove_file(dst.as_path()).is_ok()); + } + + #[cfg(target_os = "macos")] + #[test] + fn save_surface_pdf() { + let test_plot = create_test_surface(); + let k = Kaleido::new(); + let dst = PathBuf::from("example.pdf"); + let r = k.save(dst.as_path(), &test_plot, "pdf", 1200, 900, 4.5); + assert!(r.is_ok()); + assert!(dst.exists()); + let metadata = std::fs::metadata(&dst).expect("Could not retrieve file metadata"); + let file_size = metadata.len(); + assert!(file_size > 0,); + assert!(std::fs::remove_file(dst.as_path()).is_ok()); + } } From 70f9d80156654b028544cb5d3dd9503e4bee36fd Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sun, 11 May 2025 20:10:45 +0200 Subject: [PATCH 2/5] Refactor code formatting and remove extraneous whitespace. Cleaned up unnecessary blank lines and adjusted minor formatting issues to improve code readability and maintain consistency. No functional changes were made. --- plotly/src/plot.rs | 4 +--- plotly_kaleido/src/lib.rs | 10 ++-------- 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/plotly/src/plot.rs b/plotly/src/plot.rs index 86bd5ade..b9616420 100644 --- a/plotly/src/plot.rs +++ b/plotly/src/plot.rs @@ -584,10 +584,8 @@ impl PartialEq for Plot { #[cfg(test)] mod tests { - use std::path::PathBuf; - - use serde_json::{json, to_value}; + use std::path::PathBuf; use super::*; use crate::Scatter; diff --git a/plotly_kaleido/src/lib.rs b/plotly_kaleido/src/lib.rs index 2d1a27a4..d7bbd6cf 100644 --- a/plotly_kaleido/src/lib.rs +++ b/plotly_kaleido/src/lib.rs @@ -224,11 +224,9 @@ impl Kaleido { "failed to spawn Kaleido binary at {}", self.cmd_path.to_string_lossy() ) - .to_string() + .to_string() ) }); - - { let plot_data = PlotData::new(plotly_data, format, width, height, scale).to_json(); let mut process_stdin = process.stdin.take().unwrap(); @@ -339,7 +337,7 @@ mod tests { ], "layout": {} })) - .unwrap() + .unwrap() } #[test] @@ -449,7 +447,6 @@ mod tests { assert!(file_size > 0,); assert!(std::fs::remove_file(dst.as_path()).is_ok()); } - #[cfg(target_os = "macos")] #[test] fn save_surface_jpeg() { @@ -464,7 +461,6 @@ mod tests { assert!(file_size > 0,); assert!(std::fs::remove_file(dst.as_path()).is_ok()); } - #[cfg(target_os = "macos")] #[test] fn save_surface_webp() { @@ -479,7 +475,6 @@ mod tests { assert!(file_size > 0,); assert!(std::fs::remove_file(dst.as_path()).is_ok()); } - #[cfg(target_os = "macos")] #[test] fn save_surface_svg() { @@ -494,7 +489,6 @@ mod tests { assert!(file_size > 0,); assert!(std::fs::remove_file(dst.as_path()).is_ok()); } - #[cfg(target_os = "macos")] #[test] fn save_surface_pdf() { From 2cb95e6d16e8dce10a66516b5bddd6cd30a8e196 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sun, 11 May 2025 20:16:02 +0200 Subject: [PATCH 3/5] Add macOS-specific test surface creation and adjust imports Introduced a macOS-specific `create_test_surface` function in `plotly_kaleido` and adjusted test-related imports in `plotly`. Ensures compatibility with macOS while keeping non-macOS logic intact. --- plotly/src/plot.rs | 8 ++++---- plotly_kaleido/src/lib.rs | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/plotly/src/plot.rs b/plotly/src/plot.rs index b9616420..9272c47b 100644 --- a/plotly/src/plot.rs +++ b/plotly/src/plot.rs @@ -584,12 +584,12 @@ impl PartialEq for Plot { #[cfg(test)] mod tests { - use serde_json::{json, to_value}; - use std::path::PathBuf; - use super::*; use crate::Scatter; - + #[cfg(not(target_os = "macos"))] + use base64::engine::general_purpose; + use serde_json::{json, to_value}; + use std::path::PathBuf; fn create_test_plot() -> Plot { let trace1 = Scatter::new(vec![0, 1, 2], vec![6, 10, 2]).name("trace1"); let mut plot = Plot::new(); diff --git a/plotly_kaleido/src/lib.rs b/plotly_kaleido/src/lib.rs index d7bbd6cf..5ad78737 100644 --- a/plotly_kaleido/src/lib.rs +++ b/plotly_kaleido/src/lib.rs @@ -300,6 +300,7 @@ mod tests { .unwrap() } + #[cfg(target_os = "macos")] fn create_test_surface() -> Value { to_value(json!({ "data": [ From 20a29d283e89cf585cd887c864f6ae778f143357 Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sun, 11 May 2025 21:14:16 +0200 Subject: [PATCH 4/5] Refactor imports in plot.rs tests for macOS compatibility Reorganized and added conditional imports to improve clarity and maintain consistent functionality across platforms. This ensures the required dependencies are properly included, particularly handling the macOS-specific conditions. --- plotly/src/plot.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/plotly/src/plot.rs b/plotly/src/plot.rs index 9272c47b..1d8db6d9 100644 --- a/plotly/src/plot.rs +++ b/plotly/src/plot.rs @@ -586,10 +586,12 @@ impl PartialEq for Plot { mod tests { use super::*; use crate::Scatter; - #[cfg(not(target_os = "macos"))] - use base64::engine::general_purpose; + use serde_json::{json, to_value}; use std::path::PathBuf; + #[cfg(not(target_os = "macos"))] + use {base64::engine::general_purpose, base64::Engine}; + fn create_test_plot() -> Plot { let trace1 = Scatter::new(vec![0, 1, 2], vec![6, 10, 2]).name("trace1"); let mut plot = Plot::new(); From 3a5f7f766811be130aa1e7659abb56113467f01a Mon Sep 17 00:00:00 2001 From: Joaquin Bejar Date: Sun, 11 May 2025 21:25:43 +0200 Subject: [PATCH 5/5] Refactor import statements in tests module for readability. Reorganized imports in the tests module to improve clarity and maintain consistency. Non-standard library imports are now grouped separately, following a logical and readable structure. --- plotly/src/plot.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/plotly/src/plot.rs b/plotly/src/plot.rs index 1d8db6d9..826f82ba 100644 --- a/plotly/src/plot.rs +++ b/plotly/src/plot.rs @@ -584,14 +584,15 @@ impl PartialEq for Plot { #[cfg(test)] mod tests { - use super::*; - use crate::Scatter; + use std::path::PathBuf; use serde_json::{json, to_value}; - use std::path::PathBuf; #[cfg(not(target_os = "macos"))] use {base64::engine::general_purpose, base64::Engine}; + use super::*; + use crate::Scatter; + fn create_test_plot() -> Plot { let trace1 = Scatter::new(vec![0, 1, 2], vec![6, 10, 2]).name("trace1"); let mut plot = Plot::new();