-
Notifications
You must be signed in to change notification settings - Fork 40
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
be more consistent about workspace members #5921
Conversation
.github/buildomat/jobs/clippy.sh
Outdated
@@ -30,4 +30,4 @@ ptime -m bash ./tools/install_builder_prerequisites.sh -y | |||
banner clippy | |||
export CARGO_INCREMENTAL=0 | |||
ptime -m cargo xtask clippy | |||
ptime -m cargo doc | |||
RUSTDOCFLAGS="-Dwarnings" ptime -m cargo doc --workspace --keep-going |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want the --keep-going
flag here? Wouldn't that increase CI times for these workflows?
(I agree that it would be convenient locally, but I figured we'd want the CI to return any failures ASAP)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would increase the amount of time until GitHub tells you the job failed, yeah.
I don't know whether it's better to have the red X show up faster or have the logs show as many errors as possible (not necessarily all errors).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(If you don't have a preference either, I'll hedge towards not changing it and drop it from this PR.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i have a mild preference for the default, aka, the "stop early" option, just because anything that gets results back from CI faster seems like a strong default unless we're convinced otherwise.
dev-tools/xtask/src/clippy.rs
Outdated
@@ -42,6 +42,8 @@ pub fn run_cmd(args: ClippyArgs) -> Result<()> { | |||
command | |||
// Make sure we check everything. | |||
.arg("--all-targets") | |||
.arg("--workspace") | |||
.arg("--keep-going") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above, re: CI time
We have reasons for excluding some packages from the default workspace members, but there was nothing checking that newly-added packages were added to both. I added an additional check to
cargo xtask check-workspace-deps
to ensure thatdefault-members
is equal tomembers
minus xtask and end-to-end-tests.This had the effect that the tests in
api_identity
were not being run in CI!Add
--workspace
tocargo clippy
andcargo doc
in CI to run checks across the entire tree. (Also added--keep-going
so that CI shows as many errors as possible.)Fix clippy lints / doc errors that were not being caught in the previous setup.