chore(vdev): add --features flag and FEATURES env var support to build/check/test commands#25688
chore(vdev): add --features flag and FEATURES env var support to build/check/test commands#25688pront wants to merge 11 commits into
Conversation
… in build and crate_versions
This comment has been minimized.
This comment has been minimized.
pront
added
the
no-changelog
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d0febc2b2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…t the Makefile default
…--all-features default
…tures "" on FEATURES=
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94bf42ceea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| clippy: bool, | ||
|
|
||
| #[arg(value_name = "FEATURE")] | ||
| #[arg(short = 'F', long, value_delimiter = ',')] |
There was a problem hiding this comment.
For direct cargo vdev check rust invocations, setting FEATURES=... is still ignored because this new --features field is the only one in the changed commands without env = "FEATURES" (checked check rust --help: it prints -F, --features <FEATURES> with no env marker, while build/test/crate-versions show [env: FEATURES=]). In that scenario FEATURES=component-validation-tests cargo vdev check rust falls through to the --all-features path instead of linting the requested feature set, so the advertised env-var support for check/clippy only works when callers happen to go through the Makefile.
Useful? React with 👍 / 👎.
| .PHONY: check-clippy | ||
| check-clippy: ## Check code with Clippy | ||
| $(VDEV) check rust | ||
| $(VDEV) check rust $(if $(filter-out default default-msvc,$(FEATURES)),--features $(FEATURES)) |
There was a problem hiding this comment.
When a developer uses the existing space-separated feature style, e.g. make check-clippy FEATURES="default rdkafka-dynamic", this recipe expands to vdev check rust --features default rdkafka-dynamic; I checked the new vdev check rust --help/parser behavior and --features accepts a single <FEATURES> value, so the second feature is treated as an unexpected argument and clippy never runs. The test target already quotes ${FEATURES}, and doing the same here would still let Cargo parse the space-separated feature list.
Useful? React with 👍 / 👎.
1 participant
Summary
All vdev commands that shell out to cargo with a feature set (
check rust,build,test,crate_versions) previously accepted features as positional arguments or used an inconsistent--feature(singular) flag. This makes them non-obvious and inconsistent with cargo's own interface.This PR:
featuresargument incheck rustwith a named--featuresflag (short:-F, comma-separated)--feature→--featuresinbuild vectorandcrate_versions--featuresflag totest(replacing the implicit passthrough hack)--no-default-featurestocheck rustandtestenv = "FEATURES"on all--featuresflags soFEATURES=x,y make check-clippynow worksBefore:
After:
cargo vdev check rust --features default,rdkafka-dynamic FEATURES=default,rdkafka-dynamic make check-clippy # worksVector configuration
N/A — vdev tooling change only.
How did you test this PR?
cargo build -p vdevcompiles cleanlycargo vdev check rust --all-featuressuccessfully--helpoutput oncheck rust,build,testChange Type
Is this a breaking change?
cargo vdev check rust <feature> <feature>(positional) no longer works. Replacement:cargo vdev check rust --features feat1,feat2.Does this PR include user facing changes?
no-changeloglabel to this PR.References