Skip to content

Conversation

@rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Dec 3, 2025

This PR aspires toward #1791's goal of making in-memory compiles for wasm happier by making it clearer how to say don't touch the disk

  1. Split cli args and compile args so the latter can be more interestingly typed
    • cli args just turn --flag --here into a compile arg
  2. Make all paths distinct and optional
    • Emit x is removed from Flags
    • build_dir is removed, each distinct file we write gets an Option for a path
    • It falls out of ^ that to write no files you set all output paths to None

@rsheeter rsheeter changed the title [WIP] Compile without paths To support wasm, improve uspport for in-memory compilation Dec 4, 2025
@rsheeter rsheeter marked this pull request as ready for review December 4, 2025 01:00
@rsheeter rsheeter changed the title To support wasm, improve uspport for in-memory compilation Improve support for in-memory compilation, helps wasm use Dec 4, 2025
@simoncozens
Copy link
Contributor

In #1684 I made it possible to pass a Source in; (useful if you are generating your own IR, not just one of the defined Inputs) this removes that ability, unfortunately.

@anthrotype
Copy link
Member

anthrotype commented Dec 4, 2025

I propose we separate input source from the compilation options and restore the ability to generate_font from Box<dyn Source> for library users. That is:

Before (Rod's PR):

  pub fn generate_font(args: Args) -> Result<Vec<u8>, Error>
  pub fn run(args: Args, timer: JobTimer) -> Result<(), Error>
  // where Args contains input: Input

After:

  pub fn generate_font(source: Box<dyn Source>, options: Options) -> Result<Vec<u8>, Error>  // public library entry point
  pub fn run(input: Input, options: Options, timer: JobTimer) -> Result<(), Error>  // CLI entry point

I'll make a PR now

…rce> support

Refactor the compile arguments API to:
- Rename `fontc::Args` to `fontc::Options` for clarity (avoids confusion
  with CLI `args::Args`)
- Remove `input` field from Options - source is now passed separately
- Update `generate_font()` to accept `Box<dyn Source>` + `Options`
- Update `run()` to accept `Input` + `Options`

This restores the ability to pass custom `Box<dyn Source>` implementations
that was added in #1684, which library users need for generating their own
IR programmatically.

Also:
- Add `Default` derive to `Options` for ergonomic in-memory/WASM usage
- Rename `cli_args.rs` back to `args.rs` and delete `compile_args.rs` (Options inlined in lib.rs since it's short enough)
@anthrotype
Copy link
Member

I'll make a PR now

see #1797 (which is based on this PR branch, not main)

Change require_dir() to use fs::create_dir_all() instead of
fs::create_dir() so that nested paths like /tmp/deep/nested/build
are created correctly.
…used

When both --emit-ir and --output-file are used, the font is first
written to {ir_dir}/font.ttf by the persistence mechanism. If output_file
differs, we move (rename) the font to the custom output path rather than
leaving it in the IR directory.

We rename (instead of fs::write or fs::copy) as this matches the current
behavior on `main` whereby the font ends up at output_file only and is not
duplicated in the IR directory.
Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

I've been holding off on merging #1775 because I wanted to discuss some of our options in a bit more depth, but I think it also provides an opportunity to simplify some of this logic.

@rsheeter
Copy link
Contributor Author

rsheeter commented Dec 4, 2025

In #1684 I made it possible to pass a Source in; (useful if you are generating your own IR, not just one of the defined Inputs) this removes that ability, unfortunately.

Oh good catch, will fix.

@anthrotype
Copy link
Member

Oh good catch, will fix.

I fixed it in #1797, please take a look at that before duplicating work, thanks!

Remove input from fontc::Args, rename as Options; restore `Box<dyn Source>` support
@rsheeter
Copy link
Contributor Author

rsheeter commented Dec 4, 2025

please take a look at that before duplicating work, thanks!

I feel seen :D

@rsheeter rsheeter added this pull request to the merge queue Dec 4, 2025
Merged via the queue into main with commit b5bf7bb Dec 4, 2025
12 checks passed
@rsheeter rsheeter deleted the no_path branch December 4, 2025 17:22
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.

4 participants