-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(test): Make CARGO_BIN_EXE_ available at runtime #16421
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
Conversation
Seeing as this is an implicit artifact At this new location, it will now run as part of `cargo rustdoc --test <name>` which technically fixes a bug but `--test` is not really supported (rust-lang#13427) so I didn't mark this as a `fix` nor did I add tests.
This now runs in build scripts and doctests *but* the unit check should prevent any side effects from happening.
This also makes artifact deps available for consistency purposes. I originally proposed this for when moving Cargo off of Cargo's own internals but went with a different solution. Re-visiting this because `assert_cmd` has 2300+ dependents and it seems like making `assert_cmd::cargo_bin` work would be a better use of time than migrating all of those users to `assert_cmd::cargo_bin!`. For example, within the top 130 dependents (100,000 downloads or more), I found 66 that used `assert_cmd::cargo_bin` (rust-lang/rust#149852). > The reason to make `CARGO_BIN_EXE` set only at build-time was to > address the concern of manually running the test executable. > It's not > too common for tests to look at any runtime environment variables > (that I know of), so it usually just works (like running > `gdb target/debug/.../test-xxx`). > The concern was that if it were set at > runtime it would move more down the path where directly running the test > executable doesn't "just work". See https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/cargo_bin_exe.20and.20tests/near/513638220 However, - This is user opt-in - Users can run with `-vv` to see the env variables that are set - Users can run `CARGO_TARGET_..._RUNNER=gdb` - We can notify the `cargo nextest`, the main third-party test runner, about this. It should be easy to support as they have their own version of this, see https://nexte.st/docs/configuration/env-vars/?h=nextest_bin_exe#environment-variables-nextest-sets
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.
This will need an FCP
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 was a bit confused. The "Make CARGO_BIN_EXE_ available at runtime" itself needs FCP, not this specific refactor commit, right?
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.
Yes
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
CC @sunshowers |
|
Thanks, excited about this. One thing I'd note is that many environments such as shells silently drop environment variables which have hyphens in them -- for those environments (e.g. under Cargo target runners or nextest wrapper scripts), I make a version of these variables available that replaces hyphens with underscores. Would recommend both variables also be made available in Cargo. |
|
Thanks for noting that. That seems orthogonal to this and would be good for an issue to be opened to discuss and track that. |
|
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.
This looks great! Thanks for the fix.
Unrelated but this reminds me that we might also want to print the working directory in verbose mode (or |
This comment has been minimized.
This comment has been minimized.
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.
Thanks! ![]()
|
To clarify: This is only set at runtime for integration tests and benchmarks, right? That is, not doctests, unit tests, etc.? Can the documentation be updated to the new behavior? |
This comment has been minimized.
This comment has been minimized.
I've extended the test coverage to ensure they are not available for doctests and unit tests |
|
Docs are now updated |
d1dc152 to
3b6bedf
Compare
This comment has been minimized.
This comment has been minimized.
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
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.
As this is more an addition and less controversial, merge it now so we can get feedback sooner before beta branch off.
Update cargo submodule 24 commits in 94c368ad2b9db0f0da5bdd8421cea13786ce4412..fc4c92b64d1e0046b66cbdc747cc1c17af8b35a0 2025-12-26 19:39:15 +0000 to 2026-01-08 06:54:56 +0000 - Fixed incorrect version comparision during build script dependency selection (rust-lang/cargo#16486) - refactor: new type for unit index (rust-lang/cargo#16485) - feat(test): Make CARGO_BIN_EXE_ available at runtime (rust-lang/cargo#16421) - fix(package): detect dirty files when run from workspace member (rust-lang/cargo#16479) - fix(timing)!: remove `--timings=<FMT>` optional format values (rust-lang/cargo#16420) - docs(unstable): expand docs for `-Zbuild-analysis` (rust-lang/cargo#16476) - test: add `-Zunstable-options` with custom targets (rust-lang/cargo#16467) - feat(report): add cargo report rebuilds (rust-lang/cargo#16456) - feat(test-support): Use test name for dir when running tests (rust-lang/cargo#16121) - refactor: Migrate some cases to expect/reason (rust-lang/cargo#16461) - docs(build-script): clarify OUT_DIR is not cleaned between builds (rust-lang/cargo#16437) - chore: Update dependencies (rust-lang/cargo#16460) - Update handlebars to 6.4.0 (rust-lang/cargo#16457) - chore(deps): update alpine docker tag to v3.23 (rust-lang/cargo#16454) - Any build scripts can now use cargo::metadata=KEY=VALUE (rust-lang/cargo#16436) - fix(log): add `dependencies` field to `UnitRegistered` (rust-lang/cargo#16448) - Implement fine grain locking for `build-dir` (rust-lang/cargo#16155) - feat(resolver): List features when no close match (rust-lang/cargo#16445) - feat(report): new command `cargo report sessions` (rust-lang/cargo#16428) - feat (patch): Display where the patch was defined in patch-related error messages (rust-lang/cargo#16407) - test(build-rs): Reduce from 'build' to 'check' where possible (rust-lang/cargo#16444) - feat(toml): TOML 1.1 parse support (rust-lang/cargo#16415) - feat(report): support --manifest-path in `cargo report timings` (rust-lang/cargo#16441) - fix(vendor): recursively filter git files in subdirectories (rust-lang/cargo#16439)
This also makes artifact deps available for consistency purposes.
What does this PR try to resolve?
I originally proposed this for when moving Cargo off of Cargo's own
internals but went with a different solution.
Re-visiting this because
assert_cmdhas 2300+ dependents and it seemslike making
assert_cmd::cargo_binwork would be a better use of timethan migrating all of those users to
assert_cmd::cargo_bin!.For example, within the top 130 dependents (100,000 downloads or more),
I found 66 that used
assert_cmd::cargo_bin(rust-lang/rust#149852).
See https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/cargo_bin_exe.20and.20tests/near/513638220
However,
-vvto see the env variables that are setCARGO_TARGET_..._RUNNER=gdbcargo nextest, the main third-party test runner,about this. It should be easy to support as they have their own
version of this, see https://nexte.st/docs/configuration/env-vars/?h=nextest_bin_exe#environment-variables-nextest-sets
How to test and review this PR?
The
CARGO_BIN_EXE_code now also runs duringcargo rustdoc --test <name>For build scripts and doctests, the unit filter means that nothing has changed.
For
cargo rustdoc --test <name>, this technically fixes a bug where running that on a test that callsenv!("CARGO_BIN_EXE_foo")would fail. However,--testis broken more generally and we've talked about removing it (#13427) so I didn't add tests to cover this.