Skip to content
This repository has been archived by the owner on Jan 22, 2025. It is now read-only.

Add special feature for inter-crate safe dev-related code reuse #32169

Merged
merged 18 commits into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 3 additions & 1 deletion ci/buildkite-pipeline.sh
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,9 @@ wait_step() {
}

all_test_steps() {
command_step checks ". ci/rust-version.sh; ci/docker-run.sh \$\$rust_nightly_docker_image ci/test-checks.sh" 20 check
command_step checks1 ". ci/rust-version.sh; ci/docker-run.sh \$\$rust_nightly_docker_image ci/test-checks.sh" 20 check
command_step checks2 ". ci/rust-version.sh; ci/docker-run.sh \$\$rust_nightly_docker_image ci/test-dev-utils.sh check-bins" 15 check
command_step checks3 ". ci/rust-version.sh; ci/docker-run.sh \$\$rust_nightly_docker_image ci/test-dev-utils.sh check-all-targets" 15 check
wait_step

# Full test suite
Expand Down
4 changes: 4 additions & 0 deletions ci/test-checks.sh
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ echo --- build environment
cargo clippy --version --verbose
$cargoNightly clippy --version --verbose

$cargoNightly hack --version --verbose

# audit is done only with "$cargo stable"
cargo audit --version

Expand Down Expand Up @@ -110,6 +112,8 @@ else
_ scripts/cargo-for-all-lock-files.sh -- "+${rust_nightly}" sort --workspace --check
fi

_ scripts/check-dev-utils.sh tree

_ scripts/cargo-for-all-lock-files.sh -- "+${rust_nightly}" fmt --all -- --check

_ ci/do-audit.sh
Expand Down
5 changes: 5 additions & 0 deletions ci/test-dev-utils.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
#!/usr/bin/env bash

set -eo pipefail

scripts/check-dev-utils.sh "$@"
84 changes: 84 additions & 0 deletions scripts/check-dev-utils.sh
Copy link
Contributor Author

Choose a reason for hiding this comment

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

unexpected from me? lol I've done some dense commenting work. :)

Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
#!/usr/bin/env bash

set -eo pipefail
cd "$(dirname "$0")/.."
source ci/_
# only nightly is used uniformly as we contain good amount of nightly-only code
# (benches, frozen abi...)
source ci/rust-version.sh nightly

# There's a special common feature called `dev-utils` to
Copy link
Contributor

Choose a reason for hiding this comment

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

can we call the feature test-utils instead? or something that implies that the feature gates test-specific logic. maybe sol{,ana}-prefixed

Copy link
Contributor

Choose a reason for hiding this comment

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

actually maybe a use- or export- would better self-describe as well 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

can we call the feature test-utils instead? or something that implies that the feature gates test-specific logic.

well i avoided test because of existence of benches. so, i followed cargo's dev-dependency (it isn't called test-dependency).

maybe sol{,ana}-prefixed

hmm, i don't feel strong for prefixing as other existing features aren't prefixed. also, features are usually namespaced to workspace. so, even if we use --features XXX, it won't enable features from other crates. say, --features serde on CLI doen't enable im's serde feature. so, I wonder what's justification of prefixing just for dev-utils in that regard too.

actually maybe a use- or export- would better self-describe as well 🤔

i think features themselves imply using / exporting as cargo's features are additive by design. this new feature (dev-utils) works in additive manner just like all other normal features. we're just special-casing it to prevent accidental overuse and misuse at CI layer.

Copy link
Contributor

@t-nelson t-nelson Jun 26, 2023

Choose a reason for hiding this comment

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

oh that dev- 😅

i think we need another word in the middle to disambiguate. will think on it some more

# overcome cargo's issue: https://github.com/rust-lang/cargo/issues/8379
# This feature is like `cfg(test)`, which works between crates.
#
# Unfortunately, this in turn needs some special checks to avoid common
# pitfalls of `dev-utils` itself.
#
# Firstly, detect any misuse of dev-utils as normal/build dependencies.
# Also, allow some exceptions for special purpose crates. This white-listing
# mechanism can be used for core-development-oriented crates like bench bins.
#
# Put differently, use of dev-utils is forbidden for non-dev dependencies in
# general. However, allow its use for non-dev dependencies only if its use
# is confined under a dep. subgraph with all nodes being marked as dev-utils.

# Add your troubled package which seems to want to use `dev-utils` as normal (not
# dev) dependencies, only if you're sure that there's good reason to bend
# dev-util's original intention and that listed package isn't part of released
# binaries.
# Note also that dev-utils-ci-marker feature must be added and all of its
# dependencies should be edited likewise if any.
declare dev_util_tainted_packages=(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently empty; i'll add some with a follow up pr.

)

mode=${1:-full}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the default full is intended for local development use.

t-nelson marked this conversation as resolved.
Show resolved Hide resolved

if [[ $mode = "tree" || $mode = "full" ]]; then
Copy link
Contributor Author

Choose a reason for hiding this comment

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

hehe, I added this set of new checks after i drafted this pr.

# Run against the entire workspace dep graph (sans $dev_util_tainted_packages)
dev_utils_excludes=$(for tainted in "${dev_util_tainted_packages[@]}"; do
echo "--exclude $tainted"
done)
# shellcheck disable=SC2086 # Don't want to double quote $dev_utils_excludes
_ cargo "+${rust_nightly}" tree --workspace -f "{p} {f}" --edges normal,build \
$dev_utils_excludes | (
if grep -E -C 3 -m 10 "[, ]dev-utils([, ]|$)"; then
echo "dev-utils must not be used as normal dependencies" > /dev/stderr
exit 1
fi
)

# Sanity-check that tainted packages has undergone the proper tedious rituals
# to be justified as such.
for tainted in "${dev_util_tainted_packages[@]}"; do
# dev-utils-ci-marker is special proxy feature needed only when using
# dev-utils code as part of normal dependency. dev-utils will be enabled
# indirectly via this feature only if prepared correctly
_ cargo "+${rust_nightly}" tree --workspace -f "{p} {f}" --edges normal,build \
--invert "$tainted" --features dev-utils-ci-marker | (
if grep -E -C 3 -m 10 -v "[, ]dev-utils([, ]|$)"; then
echo "$tainted: All inverted dependencies must be with dev-utils" \
> /dev/stderr
exit 1
fi
)
done
fi

# Detect possible compilation errors of problematic usage of `dev-utils`-gated code
# without being explicitly declared as such in respective workspace member
# `Cargo.toml`s. This cannot be detected with `--workspace --all-targets`, due
# to unintentional `dev-utils` feature activation by cargo's feature
# unification mechanism.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here

# So, we use `cargo hack` to exhaustively build each individual workspace
# members in isolation to work around.
#
# 1. Check implicit usage of `dev-utils`-gated code in non-dev (= production) code by
# building without dev dependencies (= tests/benches) for each crate
# 2. Check implicit usage of `dev-utils`-gated code in dev (= test/benches) code by
# building in isolation from other crates, which might happen to enable `dev-utils`
if [[ $mode = "check-bins" || $mode = "full" ]]; then
_ cargo "+${rust_nightly}" hack check --bins
fi
if [[ $mode = "check-all-targets" || $mode = "full" ]]; then
_ cargo "+${rust_nightly}" hack check --all-targets
fi