Skip to content

Conversation

@cmyr
Copy link
Member

@cmyr cmyr commented Nov 21, 2025

This is a relic from back when we were thinking about incremental compilation, and has been causing me headaches recently. I was curious how hard it would be to remove and it turns out to not be that hard?

The specific motivation here is that I have twice this week wanted to move GlyphOrder into fontdrasil but have been prevented because of the implementation of Persistable for that type; the trait is defined in fontir, which also has a blanket impl for write-fonts types, and that means I can't have another custom impl for a foreign type, and the solution would require either moving the whole trait into fontdrasil, having some ugly wrapper type, or having fontir be a dep of fontdrasil, which feels backwards.

In any case, this was actually quite clean, and might unlock additional cleanups.

Copy link
Contributor

@rsheeter rsheeter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM subject to confirmation --emit-ir still works, that is a useful capability. If it breaks it ... would have to think just a little about whether thats desirable.

@cmyr
Copy link
Member Author

cmyr commented Nov 25, 2025

so this does break --emit-ir, and there's not a great solution that doesn't involve larger changes.

That said, it feels like --emit-ir could be merged into --emit-debug, and be much more narrowly scoped; there are only a couple of different bits of work for which the IR seems important, and we could figure out some way to special-case those.

My preferred way of doing that would probably be to add some new method to the Work trait, like maybe_emit_debug; this would be a noop by default, but individual impls could override it, and it would be called on all tasks after they were run.

Is this an acceptable compromise? I don't want to spend too much more time on this, but also don't want to break anyone's workflow.

Also fwiw the followup work I've done (enabled by this refactor) is genuinely useful to me. (It's about adding a new mechanism for debug-printing types while resolving GlyphId to names, because it's really hard to figure out what's going on in places like kerning when all we have access to is glyph ids)

So my preferred solution, at this point:

  • land this patch
  • separately add a patch that deprecates --emit-ir, and then replicates the behaviour of --emit-ir for specific types (static metadata and glyph ir?) when the --emit-debug flag is set

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants