[crater] Rework representation of targets by cmyr · Pull Request #1564 · googlefonts/fontc

cmyr · 2025-07-17T18:25:20Z

This patch is kind of doing a bunch of stuff and I apologize for getting it all tied up together.

I. Preamble

Crater, like much software, was accreted more than it was designed. This has started to pose challanges.

The main problem is that historically we were only concerned with font source files; repositories and config files were just a way for us to discover sources.

Later on, we started to also care about config files, so that we could do gftools builds; but this work brought in a bunch of assumptions about where config files could be found, and how they related to sources.

II. This Commit

The main purpose of this commit is to move us from being focused on sources to being focused on configs. We will only build something if we have a config, and for a given config we will only build a source that is named by that config.
In addition to that, this commit tries to generally be more consistent about how we construct paths, and how we report errors if we cannot compile a given target.
Finally, this commit adds preliminary support for 'virtual configs', which occur when we want to provide a config file for a repository that we do not otherwise want to (or are unable to) modify. These config files live in the google/fonts repository.

More work will be required before we can actually build these virtual configs, but at least now we understand that they exist.

anthrotype · 2025-07-18T09:38:54Z

We will only build something if we have a config...

are there font source repositories without any configs in which we were previously discovering and building some sources whereas now we will no longer? Or is that covered by the 'virtual configs'?

cmyr · 2025-07-18T13:57:33Z

No, previously we would rely on configs to find sources, but we would then forget about them.

rsheeter · 2025-07-21T14:36:22Z

-            .expect("config path always in sources dir");
-        // config is always in sources, sources is always in org/repo
-        let repo_dir = relative_config_path.parent().unwrap().parent().unwrap();
+        // we map repos to URLS in a special sources.json file


I don't think I understand what "special" is trying to tell me? I wonder if we could describe what is in sources.json (or point to the description) more directly? - it's not entirely clear to me from this what I'm going to find there

rsheeter · 2025-07-21T14:37:12Z

-        // config is always in sources, sources is always in org/repo
-        let repo_dir = relative_config_path.parent().unwrap().parent().unwrap();
+        // we map repos to URLS in a special sources.json file
+        let repo_dir = repo.repo_path(Path::new(""));


nit: would read nicer without Path::new, could we modify repo_path to allow &str input?

we could, but it isn't defined in this crate so would be a bit of a hassle.

rsheeter · 2025-07-21T14:39:23Z

-            .insert(repo_dir.to_owned(), repo.repo_url.clone());
+            .insert(repo_dir.clone(), repo.repo_url.clone());
+
+        let sources_dir = if repo.config_is_virtual() {


"virtual" for a file that exists feels a touch off to me. Might "external" or even out_of_repo or some siuch, aiming to convey that the config doesn't live in this repo (but does live somewhere!) convey the concept more clearly?

config_in_repo seems like it would work? If the config is not in the repo { do something else } type of deal.

This method is also not defined here, we're just calling it.

I'm not sure I understand, are you proposing a new method called config_in_repo?

Finally, this commit adds preliminary support for 'virtual configs', which occur when we want to provide a config file for a repository that we do not otherwise want to (or are unable to) modify. These config files live in the google/fonts repository.

To me this sounds like a config_override.

rsheeter · 2025-07-21T14:41:09Z

 pub(crate) struct Target {
-    /// path to the source dir for this target (relative to the git cache root)
-    source_dir: PathBuf,
+    /// path to the source repo, from the cache dir root


Suggest "relative to cache root" instead of "from..."

rsheeter · 2025-07-21T14:42:04Z

+    ///
+    /// - From the cache root if it is virtual;
+    /// - From the repo root otherwise
+    pub(crate) config: PathBuf,


Similarly suggest "relative to..." instead of "from", clearer to at least this reader and consistent with how source is described below.

rsheeter · 2025-07-21T14:42:56Z

-                }
-            }
+impl Target {
+    pub(crate) fn new2(


is new2 temporary? If not perhaps we could use names that convey something? The purpose or when it shoudl be used or some such?

oops yes this was leftover from earlier in the refactor, when I hadn't removed the previous constructor.

rsheeter · 2025-07-21T14:46:56Z

+///
+/// $ORG/$REPO/$CONFIG_PATH?$SHA $SRC_PATH ($BUILD_TYPE)
+///
+/// where a virtual config's $CONFIG_PATH starts with the literal path element


I would find EXTERNAL or OUT_OF_REPO or similar more informative than VIRTUAL, virtual makes me think you synthesized it, not that it exists somewhere else.

So if this is a serious concern I'll need to go and make a separate PR + release in google_fonts_sources, which is where I first picked this terminology. Would you prefer that?

rsheeter

LGTM subject to "virtual" changing, that term leads me (and I suspect others) to assume things that are entirely wrong

This patch is kind of doing a bunch of stuff and I apologize for getting it all tied up together. I. Preamble Crater, like much software, was accreted more than it was designed. This has started to pose challanges. The main problem is that historically we were only concerned with font source files; repositories and config files were just a way for us to discover sources. Later on, we started to also care about config files, so that we could do gftools builds; but this work brought in a bunch of assumptions about where config files could be found, and how they related to sources. II. This Commit - The main purpose of this commit is to move us from being focused on sources to being focused on configs. We will only build something if we have a config, and for a given config we will only build a source that is named by that config. - In addition to that, this commit tries to generally be more consistent about how we construct paths, and how we report errors if we cannot compile a given target. - Finally, this commit adds preliminary support for 'virtual configs', which occur when we want to provide a config file for a repository that we do not otherwise want to (or are unable to) modify. These config files live in the google/fonts repository. More work will be required before we can actually build these virtual configs, but at least now we understand that they exist.

cmyr force-pushed the configs-are-truth branch from 972e479 to 2a6e5a9 Compare July 18, 2025 13:55

rsheeter reviewed Jul 21, 2025

View reviewed changes

rsheeter approved these changes Jul 21, 2025

View reviewed changes

cmyr force-pushed the configs-are-truth branch from 2a6e5a9 to 8a615f9 Compare July 22, 2025 18:34

cmyr force-pushed the configs-are-truth branch from 8a615f9 to bb45a1a Compare July 23, 2025 14:25

cmyr added this pull request to the merge queue Jul 23, 2025

Merged via the queue into main with commit 9ee3e14 Jul 23, 2025
12 checks passed

cmyr deleted the configs-are-truth branch July 23, 2025 15:12

Uh oh!

Conversation

cmyr commented Jul 17, 2025

Uh oh!

anthrotype commented Jul 18, 2025

Uh oh!

cmyr commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rsheeter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants