[crater] Initial support for richer targets #1065
Conversation
fontc_crater/Cargo.toml
Outdated
| @@ -10,7 +10,7 @@ readme = "README.md" | |||
| [dependencies] | |||
| fontc = { version = "0.0.1", path = "../fontc" } | |||
|
|
|||
| google-fonts-sources = "0.3.1" | |||
| google-fonts-sources = { path = "../../google-fonts-sources" } | |||
There was a problem hiding this comment.
Reminder to move back to a released version pre-submit
| pub(super) limit: Option<usize>, | ||
| } | ||
| #[arg(short, long = "cache")] | ||
| cache_dir: Option<PathBuf>, |
There was a problem hiding this comment.
Why did we remove the default value from here? - now it won't show up in --help
There was a problem hiding this comment.
If we have to keep the env var lets document it and the interaction with this flag here
There was a problem hiding this comment.
added a note about the default to the help docs.
As I think you inferred this was removed so that we could check for None and then use the env var if it's set.
|
|
||
| use clap::{Parser, Subcommand}; | ||
|
|
||
| // this env var can be set by the runner in order to reuse git checkouts | ||
| // between runs. | ||
| static GIT_CACHE_DIR_VAR: &str = "CRATER_GIT_CACHE"; |
There was a problem hiding this comment.
It's not obvious why we need to probe an environment variable and support a cli option. Tbh, I'd prefer only cli arg but if we think we really need this lets document why.
There was a problem hiding this comment.
The reason this uses an env var is because in the CI environment we don't call this executable directly; we call the run.sh script in the fontc_crater repo, which in turn calls the ci.sh script in this repo, which executes this binary. The environment variable is set in the config for the scheduler that launches the CI task on the host machine.
| for source in &config.sources { | ||
| let src_path = config_path | ||
| .parent() | ||
| .expect("config path always in sources dir") |
There was a problem hiding this comment.
nit: here and below we don't actually verify what dir, should it just be config path is a file in a directory?
There was a problem hiding this comment.
I use expect as basically an alternative to an assert, or an unwrap + comment.
These are both trying to communicate the invariant that the paths returns from iter_configs always start with the cache_dir argument, and necessarily include the source (or Source) directory component.
This will facilitate gftools mode. Ultimately I decided to just rip out a bunch of the old functionality, because keeping it working was becoming a chore and it really wasn't being used much.
cmyr
force-pushed
the
crater-ci-targets
branch
from
ba41ec4
to
1dcc62d
Compare
this is blocked until I do a release of google-fonts-sources
This will facilitate gftools mode.
Ultimately I decided to just rip out a bunch of the old functionality, because keeping it working was becoming a chore and it really wasn't being used much.