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 cargo runner support in order to work with cross #193

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

glehmann
Copy link
Contributor

@glehmann glehmann commented Jan 24, 2024

allow to use assert_cmd with cross

cross requires to use a runner to launch the binary. It configures cargo to run the binary
with that runner, so cargo run --bin foo works just fine, but assert_cmd doesn't do that
so the execution fails.

to test it on a x86_64 computer, just run cross -v test --target aarch64-unknown-linux-gnu

Fixes #139

src/cargo.rs Outdated Show resolved Hide resolved
@epage
Copy link
Contributor

epage commented Jan 24, 2024

I'm not sure whether it's better to introduce a dependency on escargot or copy the escargot's method to get the target triplet as I did in this PR

For this, what you did is good enough

src/cargo.rs Fixed Show fixed Hide fixed
src/cargo.rs Fixed Show fixed Hide fixed
src/cargo.rs Fixed Show fixed Hide fixed
src/cargo.rs Fixed Show fixed Hide fixed
@epage
Copy link
Contributor

epage commented Jan 26, 2024

the test mod_example is failing but this test seems to only test escargot's features

That should have turned it into a assert_cmd::Command to verify we can integrate with escargot.

src/cargo.rs Show resolved Hide resolved
@glehmann
Copy link
Contributor Author

the test mod_example is failing but this test seems to only test escargot's features

That should have turned it into a assert_cmd::Command to verify we can integrate with escargot.

It seems to rather be a std::process::Command, but it doesn't matter much: when we have a command (from std or assert_cmd) we have lost the information that we're dealing with a binary generated by cargo. It seems dangerous to try to guess whether the runner should be used or not :-/ And in fact the Command we get is already started inside the escargot part — we don't even have a chance to update it before launching it.

The proper fix may be to add the runner support in escargot… not sure I'll have the time to do that soon.

Would you mind if I change the test to do nothing when running in cross? Something like:

diff --git a/tests/cargo.rs b/tests/cargo.rs
index 02ba3dc..35288ab 100644
--- a/tests/cargo.rs
+++ b/tests/cargo.rs
@@ -19,15 +19,19 @@ fn cargo_binary_with_empty_env() {
 
 #[test]
 fn mod_example() {
-    let bin_under_test = escargot::CargoBuild::new()
-        .bin("bin_fixture")
-        .current_release()
-        .current_target()
-        .run()
-        .unwrap();
-    let mut cmd = bin_under_test.command();
-    let output = cmd.unwrap();
-    println!("{:?}", output);
+    if std::env::var("CROSS_SYSROOT").is_ok() {
+        // not running this test on cross because escargot doesn't support the cargo target runner yet
+    } else {
+        let bin_under_test = escargot::CargoBuild::new()
+            .bin("bin_fixture")
+            .current_release()
+            .current_target()
+            .run()
+            .unwrap();
+        let mut cmd = bin_under_test.command();
+        let output = cmd.unwrap();
+        println!("{:?}", output);
+    }
 }
 
 #[test]

That way we can still make sure that the code is compiling for the chosen target, and roll back to the normal behavior once the runner support is added in escargot

@epage
Copy link
Contributor

epage commented Jan 29, 2024

Would you mind if I change the test to do nothing when running in cross? Something like:

Feel free

@epage epage changed the title WIP: add cargo runner support in order to work with cross Add cargo runner support in order to work with cross Jan 29, 2024
github-merge-queue bot pushed a commit to cross-rs/cross that referenced this pull request Jan 30, 2024
…1420)

I'm working on making assert_cmd working with cross.

There is one case a bit more difficult:
when using `clear_env()` on a process, or `env -i`, the binary is not
able to run anymore because the
`QEMU_LD_PREFIX` envvar is not set anymore.

A (relatively) easy fix might be to ensure in `linux-runner` — and in
any other runner — that the
necessary environment is configured before running the executable.
Something as proposed
in this PR for `aarch64-unknown-linux-gnu`.

Would that be an acceptable solution?

assert-rs/assert_cmd#193
@glehmann
Copy link
Contributor Author

glehmann commented Feb 3, 2024

The fix for cross has been merged. If it's ok for you I think we can do the same with this PR :)

src/cargo.rs Outdated Show resolved Hide resolved
@glehmann
Copy link
Contributor Author

@epage do you see any remaining things to fix in this PR?

@glehmann
Copy link
Contributor Author

I'm fixing the clippy warning.
I'm not sure what I can do about the Lint Commits failure however.

don't run the mod_example test when using cross

it depends on escargot also supporting cross, which it doesn't at this time

document the runner configuration usage

and how to not change anything to run with cross

close: assert-rs#139
@epage epage merged commit 82b99c1 into assert-rs:master Feb 19, 2024
12 of 13 checks passed
@glehmann
Copy link
Contributor Author

thanks :-)

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.

Document how to use assert_cmd with rust-cross
2 participants