diff --git a/Cargo.lock b/Cargo.lock index 9cb9ae36..d1223a0b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1883,6 +1883,7 @@ version = "0.1.0" dependencies = [ "fontc", "glyphs-reader", + "libc", "norad 0.16.0", "plist", "quick-xml", diff --git a/crates/shift-backends/Cargo.toml b/crates/shift-backends/Cargo.toml index 881e0e9f..947d73a9 100644 --- a/crates/shift-backends/Cargo.toml +++ b/crates/shift-backends/Cargo.toml @@ -20,3 +20,6 @@ plist = "1" quick-xml = "0.37.5" tempfile = "3" thiserror = "2.0.18" + +[target.'cfg(unix)'.dependencies] +libc = "0.2" diff --git a/crates/shift-backends/src/binary/reader.rs b/crates/shift-backends/src/binary/reader.rs index 58fe011b..6afc2d5b 100644 --- a/crates/shift-backends/src/binary/reader.rs +++ b/crates/shift-backends/src/binary/reader.rs @@ -122,6 +122,9 @@ fn detect_smooth_points(contours: &mut [Contour]) { fn font_from_skrifa(font: &FontRef<'_>) -> FormatBackendResult { let outlines = font.outline_glyphs(); let char_map = font.charmap(); + let hmtx = font + .hmtx() + .map_err(|e| FormatBackendError::Binary(format!("failed to read hmtx table: {e}")))?; let metrics = font.metrics(Size::unscaled(), LocationRef::default()); let mut ir_font = Font::new(); @@ -143,13 +146,24 @@ fn font_from_skrifa(font: &FontRef<'_>) -> FormatBackendResult { } for (unicode, glyph_id) in char_map.mappings() { - let outline = outlines.get(glyph_id).unwrap(); + let outline = outlines.get(glyph_id).ok_or_else(|| { + FormatBackendError::Binary(format!( + "missing outline for glyph {glyph_id} (U+{unicode:04X})" + )) + })?; let settings = DrawSettings::unhinted(Size::unscaled(), LocationRef::default()); let mut pen = ShiftPen::default(); - outline.draw(settings, &mut pen).unwrap(); - - let hmtx = font.hmtx().unwrap(); - let advance_width = hmtx.advance(glyph_id).unwrap(); + outline.draw(settings, &mut pen).map_err(|e| { + FormatBackendError::Binary(format!( + "failed to draw outline for glyph {glyph_id} (U+{unicode:04X}): {e}" + )) + })?; + + let advance_width = hmtx.advance(glyph_id).ok_or_else(|| { + FormatBackendError::Binary(format!( + "missing advance width for glyph {glyph_id} (U+{unicode:04X})" + )) + })?; let glyph_name = char::from_u32(unicode) .map(|c| c.to_string()) diff --git a/crates/shift-backends/src/errors.rs b/crates/shift-backends/src/errors.rs index fc351b09..f4de719c 100644 --- a/crates/shift-backends/src/errors.rs +++ b/crates/shift-backends/src/errors.rs @@ -72,6 +72,9 @@ pub enum FormatBackendError { #[error("UFO backend error: {0}")] Ufo(String), + #[error("invalid {kind} name {name:?}: UFO names must be non-empty and contain no control characters")] + UfoName { kind: &'static str, name: String }, + #[error("Glyphs backend error: {0}")] Glyphs(String), diff --git a/crates/shift-backends/src/export.rs b/crates/shift-backends/src/export.rs index 32157745..3c63b63b 100644 --- a/crates/shift-backends/src/export.rs +++ b/crates/shift-backends/src/export.rs @@ -103,7 +103,9 @@ impl FontExporter { UfoWriter::new() .save_view(font, ufo_path_str) - .map_err(|message| ExportError::PrepareUfo { message })?; + .map_err(|error| ExportError::PrepareUfo { + message: error.to_string(), + })?; compile_ttf(ufo_path_str, &build_dir, output_path) } diff --git a/crates/shift-backends/src/ufo/mod.rs b/crates/shift-backends/src/ufo/mod.rs index 3743edba..341eed23 100644 --- a/crates/shift-backends/src/ufo/mod.rs +++ b/crates/shift-backends/src/ufo/mod.rs @@ -113,7 +113,7 @@ mod tests { } #[test] - fn writer_rounds_coordinates_and_skips_empty_contours() { + fn writer_preserves_fractional_coordinates_and_skips_empty_contours() { let mut font = Font::new(); let default_source_id = font.default_source_id().unwrap(); @@ -146,13 +146,10 @@ mod tests { glyph.set_layer(layer); font.insert_glyph(glyph).unwrap(); - let temp_dir = std::env::temp_dir().join("shift_test_ufo_writer_format"); - let ufo_path = temp_dir.join("writer_format.ufo"); + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("writer_format.ufo"); let ufo_path_str = ufo_path.to_str().unwrap(); - let _ = fs::remove_dir_all(&temp_dir); - fs::create_dir_all(&temp_dir).unwrap(); - UfoWriter::new().save(&font, ufo_path_str).unwrap(); let expected_glif_filename = norad::user_name_to_file_name("A", "", ".glif", |_| true); @@ -163,13 +160,224 @@ mod tests { assert!(contents.contains("A")); assert!(contents.contains("A_.glif")); assert!(glif.contains("")); - assert!(glif.contains("")); + assert!(glif.contains("")); assert!(glif.contains("").count(), 1); + } - let _ = fs::remove_dir_all(&temp_dir); + #[test] + fn failed_save_preserves_existing_ufo() { + let font = create_test_font(); + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("target.ufo"); + let ufo_path_str = ufo_path.to_str().unwrap(); + + UfoWriter::new().save(&font, ufo_path_str).unwrap(); + + // norad refuses to serialize fonts carrying public.objectLibs, so this + // save fails during the write phase, after conversion succeeded. + let mut broken = create_test_font(); + broken.lib_mut().set( + "public.objectLibs".to_string(), + shift_font::LibValue::Dict(std::collections::HashMap::new()), + ); + UfoWriter::new() + .save(&broken, ufo_path_str) + .expect_err("save should fail"); + + let reloaded = UfoReader::new() + .load(ufo_path_str) + .expect("original UFO should still load after a failed save"); + assert_eq!( + reloaded.metadata().family_name.as_deref(), + Some("TestFamily") + ); + assert_eq!(reloaded.glyph_count(), font.glyph_count()); + + let entries: Vec<_> = fs::read_dir(temp_dir.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec!["target.ufo"], "no staging leftovers"); + } + + #[test] + fn saves_multi_glyph_multi_layer_font() { + let mut font = create_test_font(); + let default_source_id = font.default_source_id().unwrap(); + let bold_source_id = font.add_source(shift_font::Source::new( + "Bold".to_string(), + shift_font::Location::new(), + )); + + for (name, unicode) in [("B", 0x0042_u32), ("C", 0x0043)] { + let mut glyph = Glyph::with_unicode(name.to_string(), unicode); + glyph.set_layer(GlyphLayer::with_width( + LayerId::new(), + default_source_id.clone(), + 500.0, + )); + glyph.set_layer(GlyphLayer::with_width( + LayerId::new(), + bold_source_id.clone(), + 550.0, + )); + font.insert_glyph(glyph).unwrap(); + } + + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("multi.ufo"); + let ufo_path_str = ufo_path.to_str().unwrap(); + + UfoWriter::new().save(&font, ufo_path_str).unwrap(); + + let loaded = UfoReader::new().load(ufo_path_str).unwrap(); + assert_eq!(loaded.glyph_count(), 3); + assert_eq!(loaded.sources().len(), 2); + } + + #[test] + fn save_replaces_existing_ufo() { + let font = create_test_font(); + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("target.ufo"); + let ufo_path_str = ufo_path.to_str().unwrap(); + + UfoWriter::new().save(&font, ufo_path_str).unwrap(); + + let mut updated = create_test_font(); + updated.metadata_mut().family_name = Some("UpdatedFamily".to_string()); + UfoWriter::new().save(&updated, ufo_path_str).unwrap(); + + let reloaded = UfoReader::new().load(ufo_path_str).unwrap(); + assert_eq!( + reloaded.metadata().family_name.as_deref(), + Some("UpdatedFamily") + ); + + let entries: Vec<_> = fs::read_dir(temp_dir.path()) + .unwrap() + .map(|entry| entry.unwrap().file_name()) + .collect(); + assert_eq!(entries, vec!["target.ufo"], "no staging leftovers"); + } + + #[test] + fn round_trip_preserves_feature_source() { + let mut font = create_test_font(); + font.features_mut() + .set_fea_source(Some("feature liga {\n} liga;\n".to_string())); + + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("features.ufo"); + let ufo_path_str = ufo_path.to_str().unwrap(); + + UfoWriter::new().save(&font, ufo_path_str).unwrap(); + let loaded = UfoReader::new().load(ufo_path_str).unwrap(); + + assert_eq!( + loaded.features().fea_source(), + Some("feature liga {\n} liga;\n") + ); + } + + #[test] + fn save_fails_loudly_on_invalid_glyph_name() { + let mut font = Font::new(); + let default_source_id = font.default_source_id().unwrap(); + + let bad_name = "A\u{0001}B".to_string(); + let mut glyph = Glyph::with_unicode(bad_name.clone(), 0x0041); + glyph.set_layer(GlyphLayer::with_width( + LayerId::new(), + default_source_id, + 600.0, + )); + font.insert_glyph(glyph).unwrap(); + + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("invalid_name.ufo"); + + let error = UfoWriter::new() + .save(&font, ufo_path.to_str().unwrap()) + .expect_err("glyph name with control character should fail to save"); + + let message = error.to_string(); + assert!(message.contains("glyph"), "unexpected error: {message}"); + assert!( + message.contains("A\\u{1}B"), + "error should include the offending name: {message}" + ); + assert!(!ufo_path.exists(), "no partial UFO should be left behind"); + } + + #[test] + fn save_fails_loudly_on_invalid_kerning_group_name() { + let mut font = Font::new(); + font.kerning_mut().set_group1( + "public.kern1.bad\u{0000}group".to_string(), + vec!["A".to_string().into()], + ); + + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("invalid_group.ufo"); + + let error = UfoWriter::new() + .save(&font, ufo_path.to_str().unwrap()) + .expect_err("kerning group name with control character should fail to save"); + + let message = error.to_string(); + assert!( + message.contains("kerning group"), + "unexpected error: {message}" + ); + } + + #[test] + fn round_trip_preserves_fractional_coordinates_exactly() { + let mut font = Font::new(); + let default_source_id = font.default_source_id().unwrap(); + + let mut glyph = Glyph::with_unicode("A".to_string(), 0x0041); + let mut layer = GlyphLayer::with_width(LayerId::new(), default_source_id, 512.5); + + let mut contour = Contour::new(); + contour.add_point(100.5, 33.25, PointType::OnCurve, false); + contour.add_point(300.125, 700.75, PointType::OnCurve, false); + contour.add_point(600.0625, 0.5, PointType::OnCurve, false); + contour.close(); + layer.add_contour(contour); + layer.add_anchor(shift_font::Anchor::new("top".to_string(), 10.75, 720.5)); + + glyph.set_layer(layer); + font.insert_glyph(glyph).unwrap(); + + let temp_dir = tempfile::tempdir().unwrap(); + let ufo_path = temp_dir.path().join("fractional.ufo"); + let ufo_path_str = ufo_path.to_str().unwrap(); + + UfoWriter::new().save(&font, ufo_path_str).unwrap(); + let loaded = UfoReader::new().load(ufo_path_str).unwrap(); + + let loaded_glyph = loaded.glyph_by_name("A").unwrap(); + let loaded_layer = loaded_glyph + .layer_for_source(loaded.default_source_id().unwrap()) + .unwrap(); + + assert_eq!(loaded_layer.width(), 512.5); + + let expected = [(100.5, 33.25), (300.125, 700.75), (600.0625, 0.5)]; + let loaded_contour = loaded_layer.contours_iter().next().unwrap(); + assert_eq!(loaded_contour.points().len(), expected.len()); + for (point, (x, y)) in loaded_contour.points().iter().zip(expected) { + assert_eq!(point.x(), x); + assert_eq!(point.y(), y); + } + + let anchor = loaded_layer.anchors_iter().next().unwrap(); + assert_eq!(anchor.x(), 10.75); + assert_eq!(anchor.y(), 720.5); } } diff --git a/crates/shift-backends/src/ufo/writer.rs b/crates/shift-backends/src/ufo/writer.rs index 871552e2..f3178a8c 100644 --- a/crates/shift-backends/src/ufo/writer.rs +++ b/crates/shift-backends/src/ufo/writer.rs @@ -8,16 +8,111 @@ use std::path::Path; pub struct UfoWriter; -trait UfoRound { - fn ufo_round(self) -> f64; +fn ufo_name(kind: &'static str, name: &str) -> FormatBackendResult { + Name::new(name).map_err(|_| FormatBackendError::UfoName { + kind, + name: name.to_string(), + }) } -impl UfoRound for f64 { - fn ufo_round(self) -> f64 { - self.round() +fn io_error(action: &str, path: &Path, error: std::io::Error) -> FormatBackendError { + FormatBackendError::Ufo(format!("failed to {action} '{}': {error}", path.display())) +} + +#[cfg(unix)] +fn sync_parent(path: &Path) -> std::io::Result<()> { + if let Some(parent) = path.parent() { + if !parent.as_os_str().is_empty() { + std::fs::File::open(parent)?.sync_all()?; + } + } + Ok(()) +} + +// Windows cannot open directory handles this way; NTFS journals metadata itself. +#[cfg(not(unix))] +fn sync_parent(_path: &Path) -> std::io::Result<()> { + Ok(()) +} + +/// Fsyncs every file under `root`, then each directory bottom-up, so the +/// staged UFO is durable before it is renamed into place. +#[cfg(unix)] +fn sync_tree(root: &Path) -> std::io::Result<()> { + for entry in std::fs::read_dir(root)? { + let path = entry?.path(); + if path.is_dir() { + sync_tree(&path)?; + } else { + std::fs::File::open(&path)?.sync_all()?; + } + } + std::fs::File::open(root)?.sync_all()?; + Ok(()) +} + +#[cfg(not(unix))] +fn sync_tree(_root: &Path) -> std::io::Result<()> { + Ok(()) +} + +/// Atomically exchanges `staged` and `target` in one filesystem operation, +/// so there is never an instant without a UFO at `target`. +#[cfg(target_os = "macos")] +fn exchange(staged: &Path, target: &Path) -> std::io::Result<()> { + use std::os::unix::ffi::OsStrExt; + + let staged = std::ffi::CString::new(staged.as_os_str().as_bytes())?; + let target = std::ffi::CString::new(target.as_os_str().as_bytes())?; + let result = unsafe { libc::renamex_np(staged.as_ptr(), target.as_ptr(), libc::RENAME_SWAP) }; + + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) } } +#[cfg(target_os = "linux")] +fn exchange(staged: &Path, target: &Path) -> std::io::Result<()> { + use std::os::unix::ffi::OsStrExt; + + let staged = std::ffi::CString::new(staged.as_os_str().as_bytes())?; + let target = std::ffi::CString::new(target.as_os_str().as_bytes())?; + let result = unsafe { + libc::renameat2( + libc::AT_FDCWD, + staged.as_ptr(), + libc::AT_FDCWD, + target.as_ptr(), + libc::RENAME_EXCHANGE, + ) + }; + + if result == 0 { + Ok(()) + } else { + Err(std::io::Error::last_os_error()) + } +} + +#[cfg(not(any(target_os = "macos", target_os = "linux")))] +fn exchange(_staged: &Path, _target: &Path) -> std::io::Result<()> { + Err(std::io::ErrorKind::Unsupported.into()) +} + +fn exchange_unsupported(error: &std::io::Error) -> bool { + #[cfg(unix)] + if matches!( + error.raw_os_error(), + Some(libc::ENOTSUP) | Some(libc::EINVAL) | Some(libc::ENOSYS) + ) { + return true; + } + + error.kind() == std::io::ErrorKind::Unsupported +} + impl UfoWriter { pub fn new() -> Self { Self @@ -63,8 +158,8 @@ impl UfoWriter { .enumerate() .map(|(i, p)| { norad::ContourPoint::new( - p.x().ufo_round(), - p.y().ufo_round(), + p.x(), + p.y(), Self::convert_point_type(p, i, contour.points(), contour.is_closed()), p.is_smooth(), None, @@ -76,55 +171,50 @@ impl UfoWriter { norad::Contour::new(points, None) } - fn convert_component(component: &shift_font::Component) -> norad::Component { + fn convert_component( + component: &shift_font::Component, + ) -> FormatBackendResult { let matrix = component.matrix(); - norad::Component::new( - Name::new(component.base_glyph_name()).unwrap(), + Ok(norad::Component::new( + ufo_name("component base glyph", component.base_glyph_name())?, norad::AffineTransform { x_scale: matrix.xx, xy_scale: matrix.xy, yx_scale: matrix.yx, y_scale: matrix.yy, - x_offset: matrix.dx.ufo_round(), - y_offset: matrix.dy.ufo_round(), + x_offset: matrix.dx, + y_offset: matrix.dy, }, None, - ) + )) } - fn convert_anchor(anchor: &shift_font::Anchor) -> norad::Anchor { - norad::Anchor::new( - anchor.x().ufo_round(), - anchor.y().ufo_round(), - anchor.name().map(|name| Name::new(name).unwrap()), - None, - None, - ) + fn convert_anchor(anchor: &shift_font::Anchor) -> FormatBackendResult { + let name = anchor + .name() + .map(|name| ufo_name("anchor", name)) + .transpose()?; + Ok(norad::Anchor::new(anchor.x(), anchor.y(), name, None, None)) } - fn convert_guideline(guideline: &Guideline) -> norad::Guideline { + fn convert_guideline(guideline: &Guideline) -> FormatBackendResult { let line = match (guideline.x(), guideline.y(), guideline.angle()) { - (None, Some(y), None) => Line::Horizontal(y.ufo_round()), - (Some(x), None, None) => Line::Vertical(x.ufo_round()), + (None, Some(y), None) => Line::Horizontal(y), + (Some(x), None, None) => Line::Vertical(x), (Some(x), Some(y), Some(angle)) => Line::Angle { - x: x.ufo_round(), - y: y.ufo_round(), + x, + y, degrees: angle, }, - (Some(x), Some(y), None) => Line::Angle { - x: x.ufo_round(), - y: y.ufo_round(), - degrees: 0.0, - }, + (Some(x), Some(y), None) => Line::Angle { x, y, degrees: 0.0 }, _ => Line::Horizontal(0.0), }; - norad::Guideline::new( - line, - guideline.name().map(|n| Name::new(n).unwrap()), - None, - None, - ) + let name = guideline + .name() + .map(|name| ufo_name("guideline", name)) + .transpose()?; + Ok(norad::Guideline::new(line, name, None, None)) } fn convert_lib_value_to_plist(value: &LibValue) -> plist::Value { @@ -155,11 +245,13 @@ impl UfoWriter { dict } - fn convert_glyph(glyph: &Glyph, layer: &GlyphLayer) -> NoradGlyph { - let mut norad_glyph = NoradGlyph::new(glyph.name()); + fn convert_glyph(glyph: &Glyph, layer: &GlyphLayer) -> FormatBackendResult { + // NoradGlyph::new panics on invalid names, so validate first. + let name = ufo_name("glyph", glyph.name())?; + let mut norad_glyph = NoradGlyph::new(name.as_str()); - norad_glyph.width = layer.width().ufo_round(); - norad_glyph.height = layer.height().unwrap_or(0.0).ufo_round(); + norad_glyph.width = layer.width(); + norad_glyph.height = layer.height().unwrap_or(0.0); for codepoint in glyph.unicodes() { if let Some(c) = char::from_u32(*codepoint) { @@ -177,24 +269,24 @@ impl UfoWriter { for component in layer.components_iter() { norad_glyph .components - .push(Self::convert_component(component)); + .push(Self::convert_component(component)?); } for anchor in layer.anchors_iter() { - norad_glyph.anchors.push(Self::convert_anchor(anchor)); + norad_glyph.anchors.push(Self::convert_anchor(anchor)?); } for guideline in layer.guidelines() { norad_glyph .guidelines - .push(Self::convert_guideline(guideline)); + .push(Self::convert_guideline(guideline)?); } if !layer.lib().is_empty() { norad_glyph.lib = Self::convert_lib(layer.lib()); } - norad_glyph + Ok(norad_glyph) } } @@ -205,13 +297,12 @@ impl Default for UfoWriter { } impl UfoWriter { - pub fn save_view(&self, font: &impl FontView, path: &str) -> Result<(), String> { - let path_obj = Path::new(path); - if path_obj.exists() { - std::fs::remove_dir_all(path_obj) - .map_err(|e| format!("Failed to remove existing UFO: {e}"))?; - } + pub fn save_view(&self, font: &impl FontView, path: &str) -> FormatBackendResult<()> { + let norad_font = Self::build_norad_font(font)?; + Self::write_atomic(&norad_font, Path::new(path)) + } + fn build_norad_font(font: &impl FontView) -> FormatBackendResult { let mut norad_font = NoradFont::new(); norad_font.font_info.family_name = font.metadata().family_name.clone(); @@ -237,28 +328,29 @@ impl UfoWriter { norad_font.font_info.x_height = font.metrics().x_height; norad_font.font_info.italic_angle = font.metrics().italic_angle; - for (group_name, members) in font.kerning().groups1() { - norad_font.groups.insert( - Name::new(group_name).unwrap(), - members.iter().map(|n| Name::new(n).unwrap()).collect(), - ); - } - - for (group_name, members) in font.kerning().groups2() { + let groups = font + .kerning() + .groups1() + .iter() + .chain(font.kerning().groups2()); + for (group_name, members) in groups { norad_font.groups.insert( - Name::new(group_name).unwrap(), - members.iter().map(|n| Name::new(n).unwrap()).collect(), + ufo_name("kerning group", group_name)?, + members + .iter() + .map(|n| ufo_name("kerning group member", n)) + .collect::>()?, ); } for pair in font.kerning().pairs() { let first = match &pair.first { - KerningSide::Glyph(g) => Name::new(g).unwrap(), - KerningSide::Group(g) => Name::new(g).unwrap(), + KerningSide::Glyph(g) => ufo_name("kerning glyph", g)?, + KerningSide::Group(g) => ufo_name("kerning group", g)?, }; let second = match &pair.second { - KerningSide::Glyph(g) => Name::new(g).unwrap(), - KerningSide::Group(g) => Name::new(g).unwrap(), + KerningSide::Glyph(g) => ufo_name("kerning glyph", g)?, + KerningSide::Group(g) => ufo_name("kerning group", g)?, }; norad_font @@ -271,7 +363,7 @@ impl UfoWriter { for guideline in font.guidelines() { norad_font .guidelines_mut() - .push(Self::convert_guideline(guideline)); + .push(Self::convert_guideline(guideline)?); } if !font.lib().is_empty() { @@ -286,7 +378,7 @@ impl UfoWriter { continue; }; if let Some(layer_data) = glyph.layer_for_source(source_id.clone()) { - let norad_glyph = Self::convert_glyph(glyph, layer_data); + let norad_glyph = Self::convert_glyph(glyph, layer_data)?; default_layer.insert_glyph(norad_glyph); } } @@ -299,29 +391,158 @@ impl UfoWriter { let norad_layer = norad_font .layers .new_layer(source.name()) - .map_err(|e| e.to_string())?; + .map_err(|e| FormatBackendError::Ufo(e.to_string()))?; for glyph in font.glyphs() { if let Some(layer_data) = glyph.layer_for_source(source.id()) { - let norad_glyph = Self::convert_glyph(glyph, layer_data); + let norad_glyph = Self::convert_glyph(glyph, layer_data)?; norad_layer.insert_glyph(norad_glyph); } } } if let Some(fea_source) = font.features().fea_source() { - std::fs::create_dir_all(path).map_err(|e| e.to_string())?; - let fea_path = Path::new(path).join("features.fea"); - std::fs::write(fea_path, fea_source).map_err(|e| e.to_string())?; + norad_font.features = fea_source.to_string(); + } + + Ok(norad_font) + } + + /// Writes the UFO without ever destroying the existing target: the new + /// UFO is fully staged (and fsynced) in a temp sibling directory, then + /// swapped into place — atomically where the platform supports a rename + /// exchange, otherwise via move-aside-and-replace with restore on failure. + fn write_atomic(norad_font: &NoradFont, target: &Path) -> FormatBackendResult<()> { + let file_name = target.file_name().ok_or_else(|| { + FormatBackendError::Ufo(format!("invalid UFO path '{}'", target.display())) + })?; + let parent = match target.parent() { + Some(parent) if !parent.as_os_str().is_empty() => parent, + _ => Path::new("."), + }; + std::fs::create_dir_all(parent) + .map_err(|e| io_error("create parent directory for", target, e))?; + + // Staged in the target's parent so the swap is a same-filesystem rename. + let staging = tempfile::Builder::new() + .prefix(".shift-ufo-staging-") + .tempdir_in(parent) + .map_err(|e| io_error("create staging directory for", target, e))?; + let staged_ufo = staging.path().join(file_name); + norad_font + .save(&staged_ufo) + .map_err(|e| FormatBackendError::Ufo(e.to_string()))?; + sync_tree(&staged_ufo).map_err(|e| io_error("sync staged UFO at", &staged_ufo, e))?; + + if target.exists() { + Self::exchange_or_swap(target, &staged_ufo, staging)?; + } else { + std::fs::rename(&staged_ufo, target) + .map_err(|e| io_error("move new UFO into place at", target, e))?; } - norad_font.save(path).map_err(|e| e.to_string()) + sync_parent(target).map_err(|e| io_error("sync parent directory of", target, e))?; + Ok(()) + } + + fn exchange_or_swap( + target: &Path, + staged: &Path, + staging: tempfile::TempDir, + ) -> FormatBackendResult<()> { + match exchange(staged, target) { + // The previous UFO now sits at the staged path and is removed + // when the staging directory drops. + Ok(()) => Ok(()), + Err(error) if exchange_unsupported(&error) => { + Self::swap_with_backup(target, staged, staging) + } + Err(error) => Err(io_error("swap new UFO into place at", target, error)), + } + } + + fn swap_with_backup( + target: &Path, + staged: &Path, + staging: tempfile::TempDir, + ) -> FormatBackendResult<()> { + let backup = staging.path().join("previous"); + std::fs::rename(target, &backup) + .map_err(|e| io_error("move aside existing UFO at", target, e))?; + + if let Err(error) = std::fs::rename(staged, target) { + if std::fs::rename(&backup, target).is_err() { + // Keep the staging directory alive so the original UFO survives. + let staging_path = staging.keep(); + return Err(FormatBackendError::Ufo(format!( + "failed to move new UFO into place at '{}': {error}; the original UFO was preserved at '{}'", + target.display(), + staging_path.join("previous").display(), + ))); + } + return Err(io_error("move new UFO into place at", target, error)); + } + + Ok(()) } } impl FontWriter for UfoWriter { fn save(&self, font: &Font, path: &str) -> FormatBackendResult<()> { self.save_view(font, path) - .map_err(|error| FormatBackendError::Ufo(error.to_string())) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn exchange_swaps_directories() { + let temp = tempfile::tempdir().unwrap(); + let a = temp.path().join("a"); + let b = temp.path().join("b"); + std::fs::create_dir(&a).unwrap(); + std::fs::create_dir(&b).unwrap(); + std::fs::write(a.join("marker"), b"a").unwrap(); + std::fs::write(b.join("marker"), b"b").unwrap(); + + match exchange(&a, &b) { + Ok(()) => { + assert_eq!(std::fs::read(a.join("marker")).unwrap(), b"b"); + assert_eq!(std::fs::read(b.join("marker")).unwrap(), b"a"); + } + Err(error) => assert!( + exchange_unsupported(&error), + "exchange failed with unexpected error: {error}" + ), + } + } + + #[test] + fn swap_with_backup_restores_target_when_swap_fails() { + let temp = tempfile::tempdir().unwrap(); + let target = temp.path().join("target.ufo"); + std::fs::create_dir(&target).unwrap(); + std::fs::write(target.join("metainfo.plist"), b"original").unwrap(); + + let staging = tempfile::Builder::new() + .prefix(".shift-ufo-staging-") + .tempdir_in(temp.path()) + .unwrap(); + // The staged UFO is deliberately missing so the swap-in rename fails. + let staged = staging.path().join("target.ufo"); + + let error = UfoWriter::swap_with_backup(&target, &staged, staging) + .expect_err("swap should fail without a staged UFO"); + + assert!( + error.to_string().contains("move new UFO into place"), + "unexpected error: {error}" + ); + assert_eq!( + std::fs::read(target.join("metainfo.plist")).unwrap(), + b"original" + ); } } diff --git a/crates/shift-backends/tests/loading.rs b/crates/shift-backends/tests/loading.rs index 96ef538e..39f08cf3 100644 --- a/crates/shift-backends/tests/loading.rs +++ b/crates/shift-backends/tests/loading.rs @@ -146,6 +146,45 @@ fn loads_binary_fonts_with_contours() { } } +#[test] +fn binary_font_missing_hmtx_returns_error_instead_of_panicking() { + let mut bytes = std::fs::read(mutatorsans_ttf_path()).unwrap(); + + // Rename the hmtx tag in the table directory so the table lookup fails + // while the rest of the font stays parseable. + let num_tables = u16::from_be_bytes([bytes[4], bytes[5]]) as usize; + let record_offset = (0..num_tables) + .map(|index| 12 + index * 16) + .find(|&offset| &bytes[offset..offset + 4] == b"hmtx") + .expect("fixture should contain an hmtx table"); + bytes[record_offset..record_offset + 4].copy_from_slice(b"zzzz"); + + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("missing-hmtx.ttf"); + std::fs::write(&path, bytes).unwrap(); + + let error = FontLoader::new() + .read_font(path.to_str().unwrap()) + .expect_err("font without hmtx should fail to load"); + assert!( + error.to_string().contains("hmtx"), + "unexpected error: {error}" + ); +} + +#[test] +fn truncated_binary_font_returns_error_instead_of_panicking() { + let bytes = std::fs::read(mutatorsans_ttf_path()).unwrap(); + + let temp = tempfile::tempdir().unwrap(); + let path = temp.path().join("truncated.ttf"); + std::fs::write(&path, &bytes[..200]).unwrap(); + + FontLoader::new() + .read_font(path.to_str().unwrap()) + .expect_err("truncated font should fail to load"); +} + #[test] fn loads_glyphs_file_features_kerning_components_and_anchors() { let font = load_font(&homenaje_glyphs_path()); diff --git a/crates/shift-source/src/package.rs b/crates/shift-source/src/package.rs index 7fc4737f..9d36a4ca 100644 --- a/crates/shift-source/src/package.rs +++ b/crates/shift-source/src/package.rs @@ -814,6 +814,7 @@ fn tmp_path_for(path: &Path) -> PathBuf { path.with_file_name(filename) } +#[cfg(unix)] fn sync_parent(path: &Path) -> Result<(), SourcePackageError> { if let Some(parent) = path.parent() && !parent.as_os_str().is_empty() @@ -823,6 +824,12 @@ fn sync_parent(path: &Path) -> Result<(), SourcePackageError> { Ok(()) } +// Windows cannot open directory handles this way; NTFS journals metadata itself. +#[cfg(not(unix))] +fn sync_parent(_path: &Path) -> Result<(), SourcePackageError> { + Ok(()) +} + fn validate_shift_extension(path: &Path) -> Result<(), SourcePackageError> { if ShiftSourcePackage::is_package_path(path) { Ok(())