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 14 commits
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 "$@"
159 changes: 159 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,159 @@
#!/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.
declare -r dev_utils_feature="dev-utils"
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.

)

# convert to comma separeted (ref: https://stackoverflow.com/a/53839433)
printf -v allowed '"%s",' "${dev_util_tainted_packages[@]}"
ryoqun marked this conversation as resolved.
Show resolved Hide resolved
allowed="${allowed%,}"

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
case "$mode" in
tree | check-bins | check-all-targets | full)
;;
*)
echo "$0: unrecognized mode: $mode";
exit 1
;;
esac

# cargo metadata uses null for normal dep. while "dev", "build" are natually
# used by other dep. kinds.
declare -r normal_dependency="null";

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.

query=$(cat <<EOF
.packages
| map(.name as \$crate
| (.dependencies
| map(select(.kind == ${normal_dependency}))
t-nelson marked this conversation as resolved.
Show resolved Hide resolved
| map({
"crate" : \$crate,
"dependency" : .name,
"dependencyFeatures" : .features,
})
)
)
| flatten
| map(select(
(.dependencyFeatures
| index("${dev_utils_feature}")
) and (.crate as \$needle
| ([$allowed] | index(\$needle))
| not
)
))
| map([.crate, .dependency] | join(": "))
| join("\n ")
EOF
)

abusers="$(_ cargo "+${rust_nightly}" metadata --format-version=1 | jq -r "$query")"
if [[ -n "$abusers" ]]; then
# Fold message for heredoc while stripping white-spaces by echo
# shellcheck disable=SC2116
error="$(echo "${dev_utils_feature}" must not be used as normal dependencies, \
but is by "([crate]: [dependency])")"
cat <<EOF 1>&2
$error:
$abusers
EOF
t-nelson marked this conversation as resolved.
Show resolved Hide resolved
exit 1
fi

# Sanity-check that tainted packages has undergone the proper tedious rituals
# to be justified as such.
query=$(cat <<EOF
.packages
| map([.name, (.features | keys)] as [\$this_crate, \$this_feature]
| if .name as \$needle | ([$allowed] | index(\$needle))
t-nelson marked this conversation as resolved.
Show resolved Hide resolved
then
{
"crate": \$this_crate,
"crateFeatures": \$this_feature,
}
elif .dependencies | any(
.name as \$needle | ([$allowed] | index(\$needle))
)
then
.dependencies
| map({
"crate": \$this_crate,
"crateFeatures": \$this_feature,
})
else
[]
end)
| flatten
| map(select(
(.crateFeatures | index("${dev_utils_feature}")) | not
))
| map(.crate)
| join("\n ")
EOF
)

misconfigured_crates=$(
_ cargo "+${rust_nightly}" metadata \
--format-version=1 \
| jq -r "$query"
)
if [[ -n "$misconfigured_crates" ]]; then
# Fold message for heredoc while stripping white-spaces by echo
# shellcheck disable=SC2116
error="$(echo All crates marked \`tainted\', as well as their \
dependents, MUST declare the \`${dev_utils_feature}\`. The \
following crates are in violation)"
cat <<EOF 1>&2
$error:
$misconfigured_crates
t-nelson marked this conversation as resolved.
Show resolved Hide resolved
EOF
exit 1
fi
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