Skip to content

Rework cargo-test-support & testsuite to use CARGO_BIN_EXE_* for Cargo #15692

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Jun 22, 2025

What does this PR try to resolve?

This PR reworks cargo-test-support and testsuite to use Snapbox's cargo_bin!() instead of cargo_bin() which makes assumptions about the structure of Cargo's build directory.
cargo_bin!() uses CARGO_BIN_EXE_* for locating the cargo binary which should be more resilient to directory/layout changes.

Linking a relevant Zulip discussion here#t-cargo > cargo_bin_exe and tests

As shown in that link, we could make these variables available at runtime and not need to do this. However, cargo-test-support, as an API, is a bit weird in that it is baking in support for one specific binary. This can be confusing for callers and makes it more annoying for callers provide their own fn cargo, e.g. see crate-ci/cargo-fixit#7

Implementation Notes

cargo_bin!() only works when being called from the testsuite as it's only set when executing integration tests and cargo-test-support is a regular crate.
To make this change, I introduced an extension trait CargoProjectExt in testsuite for running .cargo() and implemented it on Project.

In cargo-test-support other functionality relies on .cargo() so these also needed to be moved to testsuite

  • src/tools.rs
  • Parts src/cross_compile
    • I had to split this up unfortunately, as disabled() requires running Cargo to check if we should disable cross compile tests.
    • Other fns in cross_compile are used in cargo-test-support so moving everything to testsuite would have ended up requiring moving many things to test suite.

How to test and review this PR?

I'd definitely recommend reviewing commit by commit.
There are a lot of diffs due to the nature of reorganizing things.
I did my best to split things things into smaller PRs but they still contain a lot of use statement diffs.

r? @epage

@rustbot rustbot added A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 22, 2025
@@ -2,7 +2,7 @@
//!
//! # Example
//!
//! ```no_run
//! ```ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

Would prefer for these not to be ignore

Copy link
Contributor Author

@ranger-ross ranger-ross Jun 23, 2025

Choose a reason for hiding this comment

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

The issue is that these example use .cargo() which is no longer callable in cargo-test-support. So the example no longer compiles

Perhaps, we can keep it no_run if we comment (or remove) the .cargo() calls from the examples.

Maybe something like this for publish.rs

//! let registry = RegistryBuilder::new().http_api().http_index().build();
//!
//! let p = project()
//!     .file(...)
//!     .file("src/main.rs", "fn main() {}")
//!     .build();
//!
//!  // p.cargo("publish --no-verify")
//!  //   .replace_crates_io(registry.index_url())
//!  //   .run();
//!
//! validate_upload(
//!     r#"...."#,
//!     "foo-0.0.1.crate",
//!     &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
//! );

Copy link
Contributor

Choose a reason for hiding this comment

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

Either that or we could ust do:

//! validate_upload(
//!     r#"...."#,
//!     "foo-0.0.1.crate",
//!     &["Cargo.lock", "Cargo.toml", "Cargo.toml.orig", "src/main.rs"],
//! );

@@ -6,6 +6,8 @@ use cargo_test_support::registry::{self, Package};
use cargo_test_support::str;
use cargo_test_support::{basic_manifest, project, rustc_host};

use crate::utils::ext::CargoProjectExt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we put this in a prelude and maybe re-export cargo_test_support::prelude::* in it?

Copy link
Contributor

Choose a reason for hiding this comment

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

That could even be done as a refactor commit before any of the other changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, that was something I was considering but didn't end following through with.

I think its a good idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should probably check how https://doc.crates.io/contrib/tests/writing.html needs to be updated

@ranger-ross ranger-ross force-pushed the better-bin-handling branch 2 times, most recently from 71f7c4a to 843083d Compare June 24, 2025 14:44
@ranger-ross ranger-ross force-pushed the better-bin-handling branch from 843083d to b553378 Compare June 24, 2025 14:59
use cargo_test_support::{basic_bin_manifest, basic_lib_manifest, basic_manifest, project, str};


Copy link
Contributor

Choose a reason for hiding this comment

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

whats this blank line that got added to a lot of these files?

use crate::prelude::*;

pub mod prelude {
pub use crate::utils::ext::CargoProjectExt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you split out the creation of CargoProjectExt from the prelude, as in create the prelude in one commit and then CargoProjectExt after it?

This touches a lot of files and so being minimal with what is changed can make it a lot easier to review or come back later and see what was done

See also #15692 (comment)

Comment on lines 4 to 12

use cargo_test_support::compare::assert_e2e;
use crate::prelude::*;
use cargo_test_support::compare::assert_e2e;
use cargo_test_support::publish::validate_alt_upload;
use cargo_test_support::registry::{self, Package, RegistryBuilder};
use cargo_test_support::str;
use cargo_test_support::{basic_manifest, paths, project};


#[cargo_test]
Copy link
Contributor

Choose a reason for hiding this comment

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

This commit seems to have some unrelated changes in it

Comment on lines 1 to 2
use crate::prelude::*;
use crate::utils::ext::CargoCommandExt;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the use added when its in the prelude?

@ranger-ross ranger-ross force-pushed the better-bin-handling branch from b553378 to 1d064ab Compare June 25, 2025 14:08
@rustbot rustbot added the A-documenting-cargo-itself Area: Cargo's documentation label Jun 25, 2025
@ranger-ross ranger-ross force-pushed the better-bin-handling branch from 1d064ab to 02b56d4 Compare June 25, 2025 14:22
@ranger-ross
Copy link
Contributor Author

Okay sorry, I cleaned up the commits. (Not quiet sure what happened)

I split out the commits as you suggested in this comment and updated the testing documentation as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-testing-cargo-itself Area: cargo's tests S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants