Make all paths optional #1791
Make all paths optional #1791
Conversation
simoncozens
force-pushed
the
paths-all-optional
branch
from
2238f6f to
3372085
Compare
simoncozens
force-pushed
the
paths-all-optional
branch
from
3372085 to
48318b6
Compare
fontc/src/lib.rs
Outdated
| })?; | ||
| } else if !font_file.exists() { | ||
| return Err(Error::FileExpected(font_file)); | ||
| return Err(Error::FileExpected(font_file.to_path_buf())); |
There was a problem hiding this comment.
oh wait, there's a problem here. When --emit-ir is enabled the font is written (via PersistentStorage) to Paths.output_file, however write_font_file expects it at args.output_file.
Before this PR, init_paths would call Paths::with_output_file() which set Paths.output_file = args.output_file, keeping them in sync. Now it only calls Paths::new(build_dir) which always sets Paths.output_file = build_dir/font.ttf which means they can diverge and lead to this error if one uses both --emit-ir and --output-file
$ mkdir -p /tmp/fontc-test/build
$ ./target/release/fontc \
--emit-ir \
--build-dir /tmp/fontc-test/build \
--output-file /tmp/fontc-test/custom.ttf \
resources/testdata/static.designspace
[2025-12-02T16:52:46.782299Z ThreadId(1) fontc ERROR] Missing file '/tmp/fontc-test/custom.ttf'
There was a problem hiding this comment.
If the intent is for Paths to only handle IR/debug storage and not final output, then we could simplify write_font_file to always write the font from memory, removing the emit_ir conditonal entirely. The font is always in memory (be_context.font), so we could just write it to the output file.
We could consider decoupling the final font output from PersistentStorage entirely. Does WorkId::Font need to persist? Probably not because the font is the final output, not intermediate state. No other work depends on a previously-built font, and it's always available in memory.
If we do that then we can get also rid of Paths.output_file field (and output_file() getter), alongside Paths::with_output_file() constructor. And we cleanly separate concerns: PersistentStorage handles IR and intermediate tables and write_font_file handles the final font output..
There was a problem hiding this comment.
The problem I see is that two different things are both responsible for writing out the font file...
There was a problem hiding this comment.
We could consider decoupling the final font output from PersistentStorage entirely. Does WorkId::Font need to persist?
The type system says that it does. :-) If we have a WorkId for it, then we impl PersistentStorage<AnyWorkId> for BePersistentStorage.
There was a problem hiding this comment.
what I had in mind is the following: in fontbe/src/orchestration.rs, we create the font's ContextItem with None for persistent storage, so that when context.font.set() is called, ContextItem::set() skips the persistence step because self.persistent_storage is None. Then in fontbe/src/paths.rs, we make target_file(&WorkId::Font) unreachable as a safety net to catch any accidental attempts to get a path for WorkId::Font. So te type system still says BePersistentStorage must handle all WorkId variants, but the font's ContextItem bypasses persistence entirely (via None) and the target_file match arm exists but panics if ever reached.
The WorkId::Font variant still exists (it's used for the work graph), it just doesn't participate in persistence anymore.
diff --git a/fontbe/src/orchestration.rs b/fontbe/src/orchestration.rs
index 64e9e8d4..0ce399bf 100644
--- a/fontbe/src/orchestration.rs
+++ b/fontbe/src/orchestration.rs
@@ -5,7 +5,7 @@ use std::{
fmt::Display,
fs::File,
io::{self, BufReader, BufWriter, Read, Write},
- path::{Path, PathBuf},
+ path::Path,
sync::Arc,
};
@@ -1023,9 +1023,10 @@ impl Context {
extra_fea_tables: ContextItem::new(
WorkId::ExtraFeaTables.into(),
acl.clone(),
- persistent_storage.clone(),
+ persistent_storage,
),
- font: ContextItem::new(WorkId::Font.into(), acl, persistent_storage),
+ // Font is not persisted to IR; it's written via write_font_file()
+ font: ContextItem::new(WorkId::Font.into(), acl, None),
}
}
@@ -1047,12 +1048,6 @@ impl Context {
.as_ref()
.map(|ps| ps.paths.debug_dir())
}
-
- pub fn font_file(&self) -> Option<PathBuf> {
- self.persistent_storage
- .as_ref()
- .map(|ps| ps.paths.target_file(&WorkId::Font))
- }
}
#[derive(PartialEq)]
diff --git a/fontbe/src/paths.rs b/fontbe/src/paths.rs
index 3864ebba..8a82ccee 100644
--- a/fontbe/src/paths.rs
+++ b/fontbe/src/paths.rs
@@ -11,7 +11,6 @@ pub struct Paths {
build_dir: PathBuf,
glyph_dir: PathBuf,
debug_dir: PathBuf,
- output_file: PathBuf,
}
impl Paths {
@@ -19,12 +18,10 @@ impl Paths {
let glyph_dir = build_dir.join("glyphs");
let debug_dir = build_dir.join("debug");
let build_dir = build_dir.to_path_buf();
- let output_file = build_dir.join("font.ttf");
Paths {
build_dir,
glyph_dir,
debug_dir,
- output_file,
}
}
@@ -40,10 +37,6 @@ impl Paths {
&self.glyph_dir
}
- pub fn output_file(&self) -> &Path {
- &self.output_file
- }
-
fn glyph_glyf_file(&self, name: &str) -> PathBuf {
self.glyph_dir.join(string_to_filename(name, ".glyf"))
}
@@ -97,7 +90,7 @@ impl Paths {
WorkId::Vmtx => self.build_dir.join("vmtx.table"),
WorkId::Vvar => self.build_dir.join("vvar.table"),
WorkId::ExtraFeaTables => self.build_dir.join("extra_tables.bin"),
- WorkId::Font => self.output_file.clone(),
+ WorkId::Font => unreachable!("Font is not persisted to IR; use write_font_file()"),
}
}
}
diff --git a/fontc/src/lib.rs b/fontc/src/lib.rs
index e871d5ae..63130afc 100644
--- a/fontc/src/lib.rs
+++ b/fontc/src/lib.rs
@@ -212,10 +212,9 @@ pub fn init_paths(build_dir: &Path, flags: Flags) -> Result<(IrPaths, BePaths),
let ir_paths = IrPaths::new(build_dir);
let be_paths = BePaths::new(build_dir);
- // the build dir stores the IR (for incremental builds) so we don't need
- // to create one unless we're writing to it
+ // The build dir is needed for IR and/or debug output
+ require_dir(build_dir)?;
if flags.contains(Flags::EMIT_IR) {
- require_dir(build_dir)?;
require_dir(ir_paths.anchor_ir_dir())?;
require_dir(ir_paths.glyph_ir_dir())?;
require_dir(be_paths.glyph_dir())?;
@@ -235,21 +234,16 @@ pub fn init_paths(build_dir: &Path, flags: Flags) -> Result<(IrPaths, BePaths),
#[cfg(feature = "cli")]
pub fn write_font_file(args: &Args, be_context: &BeContext) -> Result<(), Error> {
- // if IR is off the font didn't get written yet (nothing did), otherwise it's done already
let Some(font_file) = &args.output_file else {
return Ok(());
};
if let Some(parent) = font_file.parent() {
require_dir(parent)?;
}
- if !args.emit_ir {
- fs::write(font_file, be_context.font.get().get()).map_err(|source| Error::FileIo {
- path: font_file.to_path_buf(),
- source,
- })?;
- } else if !font_file.exists() {
- return Err(Error::FileExpected(font_file.to_path_buf()));
- }
+ fs::write(font_file, be_context.font.get().get()).map_err(|source| Error::FileIo {
+ path: font_file.to_path_buf(),
+ source,
+ })?;
Ok(())
}
The patch above is less invasive and more targeted than 626596a
In yours the Flags become Clone, not Copy anymore. And Font still persists to build_dir/font.ttf AND writes to output file so it's written twice when emit_ir is on.
How about you split this to follow up PR in which you propose the latter changes?
By the way, there is also #1775 where @cmyr removes Persistable and PersistentStorage entirely which may conflict with this. We may want to wait to land that one first if that's the intended direction we want to take.
There was a problem hiding this comment.
The patch above is less invasive and more targeted than 626596a
Yes, but it does less. :-) I suggest we need to do both things: stop the flags/build_dir duplication and also stop the font being written twice.
|
OK, I'm going to try and lean into the type system: i.e. rewrite Flags such that it encodes the paths, then you can't have a situation where EMIT_IR is set but build directory is empty and vice versa. |
|
Yes, This Is The Way. diff --git i/fontbe/src/features.rs w/fontbe/src/features.rs
index a999abaf..79e15044 100644
--- i/fontbe/src/features.rs
+++ w/fontbe/src/features.rs
@@ -433,10 +433,7 @@ impl FeatureCompilationWork {
}
}
-fn write_debug_glyph_order(context: &Context, glyphs: &GlyphOrder) {
- let debug_dir = context
- .debug_dir()
- .expect("EMIT_DEBUG is set but paths are empty");
+fn write_debug_glyph_order(debug_dir: &Path, glyphs: &GlyphOrder) {
let glyph_order_file = debug_dir.join("glyph_order.txt");
let glyph_order = glyphs.names().map(|g| g.as_str()).collect::<Vec<_>>();
let glyph_order = glyph_order.join("\n");
@@ -445,17 +442,7 @@ fn write_debug_glyph_order(context: &Context, glyphs: &GlyphOrder) {
}
}
-fn write_debug_fea(context: &Context, is_error: bool, why: &str, fea_content: &str) {
- if !context.flags.contains(Flags::EMIT_DEBUG) {
- if is_error {
- warn!("Debug fea not written for '{why}' because --emit-debug is off");
- }
- return;
- }
-
- let debug_dir = context
- .debug_dir()
- .expect("EMIT_DEBUG is set but paths are empty");
+fn write_debug_fea(debug_dir: &Path, is_error: bool, why: &str, fea_content: &str) {
let debug_file = debug_dir.join("features.fea");
match fs::write(&debug_file, fea_content) {
Ok(_) if is_error => warn!("{why}; fea written to {debug_file:?}"),
@@ -485,11 +472,11 @@ impl Work<Context, AnyWorkId, Error> for FeatureFirstPassWork {
let result = self.parse(&features, &glyph_map);
- if context.flags.contains(Flags::EMIT_DEBUG) {
- write_debug_glyph_order(context, &glyph_order);
- }
- if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() {
- write_debug_fea(context, result.is_err(), "compile failed", fea_content);
+ if let Some(debug_dir) = context.flags.debug_dir.as_deref() {
+ write_debug_glyph_order(debug_dir, &glyph_order);
+ if let FeaturesSource::Memory { fea_content, .. } = features.as_ref() {
+ write_debug_fea(debug_dir, result.is_err(), "compile failed", fea_content);
+ }
}
let ast = result?;
@@ -671,9 +658,7 @@ impl Work<Context, AnyWorkId, Error> for FeatureCompilationWork {
}
// Enables the assumption that if the file exists features were compiled
- if context.flags.contains(Flags::EMIT_IR)
- && let Some(persistent_storage) = &context.persistent_storage
- {
+ if let Some(persistent_storage) = &context.persistent_storage {
fs::write(persistent_storage.paths.target_file(&WorkId::Features), "1")
.map_err(Error::IoError)?;
} |
|
OK, I think the right answer here is IMO:
However that probably requires a collective decision and then a lot of work, and I need to get on with other stuff for now. I still need a fontc which doesn't assume a filesystem but I guess I can temporarily depend on this branch. |
Pragmatic but unsustainable, if we're using wasm for anything real we need a better solution. #1775 is arguably blocked, in which case this would be blocked as well? To ask the obvious, are we using wasm for anything real or only for explorations as yet?
The general direction here sounds preferable if feasible (seems like it should be, but I haven't tried), and also like it should be a relatively small change if made in isolation? |
the font is the final output, not intermediate state. No other work depends on a previously-built font, it's always available in memory. write_font_file can just write the final font to args.output_file without worrying about Paths.output_file. see #1791 (comment)
Only for explorations until it works. :-) Then we will use it in font editors. |
|
#1796 aspires to fix this |
3 participants
Another thing that's a bit of a downer for WASM is that fontc currently stores a lot of paths and does make-directory operations on those paths, even though sometimes we're not storing anything in those paths except the final font file. (and in the case of WASM, not even that!)
The first commit makes
PersistentStorageobjects anOption<T>instead of having a flag calledactive; that's relatively standalone. On top of that, we then make thePathsobject optional except whereEMIT_IR | EMIT_DEBUGis set.The end result is you can run
generate_fontwithbuild_dirset toNone, and you don't have to do this kind of rubbish any more:fontc/fontir/src/glyph.rs
Line 937 in f453ef0