cargo-dist icon indicating copy to clipboard operation
cargo-dist copied to clipboard

plan should warn if target that has no default github runner is specified w/o a configured custom runner

Open ashleygwilliams opened this issue 1 year ago • 1 comments

we currently support an infinite number of targets in the targets array, but github has a finite and specified number of default runners. if someone adds a target that does not have a default runner, AND does not specify a custom runner, we should have plan error. ideally, this means that cargo-dist will catch the error on the configuration update and not when someone is trying to release.

ashleygwilliams avatar Apr 30 '24 15:04 ashleygwilliams

So this is an interesting one in that there's a few different interacting checks.

Here we do a preliminary selection of what github runner to use for a given target triple, but in a very loose/permissive way:

https://github.com/axodotdev/cargo-dist/blob/e64776d69f7dda13f4e7dcb99d2714efc187740a/cargo-dist/src/backend/ci/github.rs#L292-L313

And here we check that result and warn if it returned None:

https://github.com/axodotdev/cargo-dist/blob/e64776d69f7dda13f4e7dcb99d2714efc187740a/cargo-dist/src/backend/ci/github.rs#L271-L273

So we do have a warning and it does fire after init/plan, but the "real" issue is that the preliminary selection is really generous, and arguably should be changed to be more strict, especially now that we have a proper config for the user to disambiguate (the looseness here was basically trying to let stuff work in a world where the user was doomed if we didn't pick something).

Also of note is this code that selects and expression for how to install cargo-dist for a given target triple, again very loose:

https://github.com/axodotdev/cargo-dist/blob/e64776d69f7dda13f4e7dcb99d2714efc187740a/cargo-dist/src/backend/ci/github.rs#L316-L331

This code is ~fine for now since we need to have builds for the platform for it to work.


So the "fix" here is:

  • change github_runner_for_target to an explicit allow-list of full target triples, using constants defined in axoproject https://docs.rs/axoproject/latest/axoproject/platforms/index.html
  • improve the warning to mention the custom runner setting
  • maybe make the warning a hard error?

Gankra avatar May 09 '24 18:05 Gankra