Skip to content
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

Add support for target runners #61

Merged
merged 9 commits into from
Feb 22, 2022
Merged

Add support for target runners #61

merged 9 commits into from
Feb 22, 2022

Conversation

Jake-Shadle
Copy link
Contributor

@Jake-Shadle Jake-Shadle commented Feb 18, 2022

This adds support for target runners which allows nextest to run tests for binaries that can't execute natively on the host such in cross-compilation situations targeting x86_64-pc-windows-msvc from a Linux host, or wasm32-unknown-unknown, etc.

This PR supports both the CARGO_TARGET_{TRIPLE}_RUNNER environment variable, as well as reading configured runners from .cargo/config.toml. This currently respects the same (IMO broken) behavior in cargo for looking up the hierarchy of configs.

Example output running windows binaries via wine from linux

image

Resolves: #57

Copy link
Member

@sunshowers sunshowers left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

So in general I like the idea of supporting target runners, but I think trying to emulate Cargo's behavior is going to be really complicated, as this PR shows. Instead, I think we should support a NEXTEST_<TRIPLE>_RUNNER env var, and a config in .config/nextest.toml under the profiles section.

I also think this needs some tests to ensure that the correct runner's being run. An integration test could be done similar to the basic tests -- through a stub binary that just prints some output.

nextest-runner/src/errors.rs Outdated Show resolved Hide resolved
nextest-runner/src/target_runner.rs Outdated Show resolved Hide resolved
nextest-runner/src/runner.rs Outdated Show resolved Hide resolved
nextest-runner/src/target_runner.rs Outdated Show resolved Hide resolved
@Jake-Shadle
Copy link
Contributor Author

I started addressing your PR feedback but just to be sure...you're not saying to support a NEXTEST_RUNNER and nextest.toml configuration for the target runner instead of cargo, but in addition to them?

@Jake-Shadle
Copy link
Contributor Author

So I'm looking at the config code right now, are you sure you want me to add runner support there? There's no target specific keys in there yet, and it seems like adding them to profiles could be really confusing since it would have to match both the target and the profile, and, at least how it is setup now, would mean you'd have to duplicate the runner for each profile...

@sunshowers
Copy link
Member

sunshowers commented Feb 21, 2022

Thanks for working on this!

I started addressing your PR feedback but just to be sure...you're not saying to support a NEXTEST_RUNNER and nextest.toml configuration for the target runner instead of cargo, but in addition to them?

I think it makes sense to support them instead of cargo, so that we don't have to deal with the complexity of emulating cargo's config logic. Nextest has several major differences from cargo test (such as per-test process isolation), and this can be documented as another.

So I'm looking at the config code right now, are you sure you want me to add runner support there? There's no target specific keys in there yet, and it seems like adding them to profiles could be really confusing since it would have to match both the target and the profile, and, at least how it is setup now, would mean you'd have to duplicate the runner for each profile...

Most people I think will just set the default profile, which other profiles will inherit from. In general there aren't expected to be too many profiles -- the main use case I've seen is local vs CI builds.

@sunshowers
Copy link
Member

sunshowers commented Feb 21, 2022

So yeah I think profile.<profile-name>.target.<target-name>.runner would be the correct place for this. This also provides the flexibility for CI to use a runner with a different name, or at a different path if desired.

@sunshowers
Copy link
Member

sunshowers commented Feb 22, 2022

OK, so after having thought about this more, I think the current solution is actually fine. I'll land this and iterate on it locally (keeping the CARGO behavior, I just want to tidy up and add documentation). Thank you so much for your patience dealing with this!!!

@sunshowers
Copy link
Member

The tests are absolutely fantastic, by the way. I love them so much.

@sunshowers sunshowers merged commit e43ee0f into nextest-rs:main Feb 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list (maybe run?) doesn't respect CARGO_TARGET_{TRIPLE}_RUNNER
2 participants