-
Notifications
You must be signed in to change notification settings - Fork 82
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
plan should warn if target that has no default github runner is specified w/o a configured custom runner #977
Comments
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: cargo-dist/cargo-dist/src/backend/ci/github.rs Lines 292 to 313 in e64776d
And here we check that result and warn if it returned None: cargo-dist/cargo-dist/src/backend/ci/github.rs Lines 271 to 273 in e64776d
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: cargo-dist/cargo-dist/src/backend/ci/github.rs Lines 316 to 331 in e64776d
This code is ~fine for now since we need to have builds for the platform for it to work. So the "fix" here is:
|
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.
The text was updated successfully, but these errors were encountered: