Skip to content

Add support for running crate specific extra_lints.sh #21

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 8, 2025
Merged

Add support for running crate specific extra_lints.sh #21

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14e5c1e
Fix indentation
jamillambert Oct 6, 2025
943ca06
Remove workspace no-default-features clippy run
jamillambert Oct 6, 2025
856e83d
Add support for running crate specific extra_lints
jamillambert Oct 6, 2025
7c6233a
Update README
jamillambert Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 49 additions & 43 deletions ci/run_task.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,40 +72,41 @@ main() {
fi

case $task in
stable)
# Test, run examples, do feature matrix.
# crate/contrib/test_vars.sh is sourced in this function.
build_and_test
;;

nightly)
build_and_test
;;

msrv)
build_and_test
;;

lint)
do_lint
do_dup_deps
;;

docs)
build_docs_with_stable_toolchain
;;

docsrs)
build_docs_with_nightly_toolchain
;;

bench)
do_bench
;;

*)
err "Error: unknown task $task"
;;
stable)
# Test, run examples, do feature matrix.
# crate/contrib/test_vars.sh is sourced in this function.
build_and_test
;;

nightly)
build_and_test
;;

msrv)
build_and_test
;;

lint)
Copy link
Contributor

@nyonson nyonson Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was just poking around this repo for the first time and appreciate the whitespace clean up

do_lint_workspace
do_lint_crates
do_dup_deps
;;

docs)
build_docs_with_stable_toolchain
;;

docsrs)
build_docs_with_nightly_toolchain
;;

bench)
do_bench
;;

*)
err "Error: unknown task $task"
;;
esac
}

Expand Down Expand Up @@ -228,17 +229,22 @@ loop_features() {
}

# Lint the workspace.
do_lint() {
do_lint_workspace() {
need_nightly

$cargo clippy --workspace --all-targets --all-features --keep-going -- -D warnings
$cargo clippy --workspace --all-targets --keep-going -- -D warnings
Copy link
Contributor

@nyonson nyonson Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll admit I am not sure on the driving reason for this change, so my bad if a dumb question, but what's the reasoning on dropping --all-features from the default flag set? My gut says it's better to check all the code by default, and then have the crate-specific stuff search for weird feature combos.

Copy link
Contributor Author

@jamillambert jamillambert Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I forgot to link to the issue in rust-bitcoin that started this all. I have updated the description to mention it now.

The bit that was dropped is --workspace --no-default-features, I am not sure exactly what clippy does, but it does not lint the crates with no-default-features when you use it with workspace.

Dropping --all-features for a second run of clippy was always there, it was just, incorectly, under an if statement that skipped corepc for no-default-features.

Copy link
Contributor

@nyonson nyonson Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, thanks for stepping through that for me. So now is the second clippy run testing the default features while the first is all?

Copy link
Contributor Author

@jamillambert jamillambert Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we also thought we were doing a third run with no features. But as you can see in the related issue this is not the case. EDIT: I say "we" but really this is my first contribution to this repo 😊.

Copy link
Contributor

@nyonson nyonson Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea makes sense. Is there any benefits to running the default features one? Feels a little murky what that would actually resolve to for the workspace.

Copy link
Contributor Author

@jamillambert jamillambert Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so, only because when I was testing this change there was a lint error in https://github.com/rust-bitcoin/corepc that only comes up with the second clippy with default features. It didn't come up before because that line was not run for corepc.

}

# Ugly hack to skip `corpec` because `node` does not build with no default features.
if echo "$REPO_DIR" | grep -viq "corepc"; then
# Lint various feature combinations to try and catch mistakes in feature gating.
$cargo clippy --workspace --all-targets --keep-going -- -D warnings
$cargo clippy --workspace --all-targets --no-default-features --keep-going -- -D warnings
fi
# Run extra crate specific lints, e.g. clippy with no-default-features.
do_lint_crates() {
need_nightly
for crate in $CRATES; do
pushd "$REPO_DIR/$crate" > /dev/null
if [ -e ./contrib/extra_lints.sh ]; then
./contrib/extra_lints.sh
Copy link
Contributor

@nyonson nyonson Oct 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to add this new interface to the README. Might just be my naive/fresh eyes, but I found the README really helpful to understand the interface of this repo.

Copy link
Contributor Author

@jamillambert jamillambert Oct 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I agree. I just forgot.

Added a line to the README.

fi
popd > /dev/null
done
}

# We should not have any duplicate dependencies. This catches mistakes made upgrading dependencies
Expand Down Expand Up @@ -332,7 +338,7 @@ say_err() {

verbose_say() {
if [ "$flag_verbose" = true ]; then
say "$1"
say "$1"
fi
}

Expand Down