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

Honor runner configuration for sub-crates in workspaces #12147

Closed
d-e-s-o opened this issue May 17, 2023 · 3 comments
Closed

Honor runner configuration for sub-crates in workspaces #12147

d-e-s-o opened this issue May 17, 2023 · 3 comments
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage.

Comments

@d-e-s-o
Copy link
Contributor

d-e-s-o commented May 17, 2023

Problem

When I have a workspace and one of the crates defines a custom test runner, this test runner is not used when running cargo test from the workspace root, only when executing it from the sub-crate itself. E.g.,

mkdir workspace
cd workspace
cargo init --lib crate1
cargo init --lib crate2
mkdir crate2/.cargo
cat <<EOF > crate2/.cargo/config
[target.x86_64-unknown-linux-gnu]
runner = "false"
EOF
cat <<EOF > Cargo.toml
[workspace]
members = [
  "crate1",
  "crate2",
]
EOF
$ cargo test
>   Compiling crate2 v0.1.0 (workspace/crate2)
>   Compiling crate1 v0.1.0 (workspace/crate1)
>    Finished test [unoptimized + debuginfo] target(s) in 0.23s
>     Running unittests src/lib.rs (workspace/target/debug/deps/crate1-17df813c4096d5b4)
>
>running 1 test
>test tests::it_works ... ok
>
>test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
>
>     Running unittests src/lib.rs (workspace/target/debug/deps/crate2-adadd9a2fd7f0375)
>
>running 1 test
>test tests::it_works ... ok
>
>test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
>
>   Doc-tests crate1
>
>running 0 tests
>
>test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s
>
>   Doc-tests crate2
>
>running 0 tests
>
>test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.00s

$ cd crate2/
$ cargo test
>     Finished test [unoptimized + debuginfo] target(s) in 0.00s
>      Running unittests src/lib.rs (workspace/target/debug/deps/crate2-adadd9a2fd7f0375)
> error: test failed, to rerun pass `--lib`
>
> Caused by:
>   process didn't exit successfully: `false workspace/target/debug/deps/crate2-adadd9a2fd7f0375` (exit status: 1)

In some weird way this seems to be yet another instance of #8870 (which I am still battling...)

Proposed Solution

Please: honor the logical configuration for the respective crate for which tests are run.

In the example above, I expect cargo test to fail for crate2 even if run from the workspace root.

Notes

I classified this issue as a feature request because as per #8870 it can probably be argued that this is "by design" (and not a bug). Yet, this design is most certainly questionable, in my opinion.

@d-e-s-o d-e-s-o added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels May 17, 2023
danielocfb pushed a commit to danielocfb/libbpf-rs that referenced this issue May 17, 2023
We have some complicated and non-obvious logic for skipping certain
tests when running cargo test from the root of the crate in CI, but then
an explicit invocation from the libbpf-rs/ sub-directory attempting to
run those skipped tests (now under sudo).
Simplify the logic and hopefully make it somewhat more obvious by
prefixing all tests that require root with `test_root_`.

Note: this is a workaround for what I'd call a bug in Cargo. I opened
rust-lang/cargo#12147 in the hopes that this
will be addressed eventually (though I don't know how realistic that
is). If we are still getting confusion even with this more explicit
naming, we may want to consider moving (or copying) libbpf-rs's .cargo/
directory into the workspace root.

Signed-off-by: Daniel Müller <deso@posteo.net>
@ehuss
Copy link
Contributor

ehuss commented May 17, 2023

Thanks for the report! It is known that it can be confusing that cargo uses the current directory to determine which configuration files to load. Closing as a duplicate of #2930, #9769, and #10098 (and possibly https://internals.rust-lang.org/t/proposal-move-some-cargo-config-settings-to-cargo-toml/13336).

@ehuss ehuss closed this as not planned Won't fix, can't repro, duplicate, stale May 17, 2023
@epage
Copy link
Contributor

epage commented May 17, 2023

To add, .cargo/config files are environment configuration, rather than package configuration, and are dependent on your CWD and not the directory of the package being run/tested. Hence, we are identifying config that is really package related and moving it to Cargo.toml (the linked internals thread). rust-lang/rfcs#3389 is an example of that.

@d-e-s-o
Copy link
Contributor Author

d-e-s-o commented May 17, 2023

Thanks for the pointers. I am happy to hear that this is being recognized as an issue and looked at!

danielocfb pushed a commit to danielocfb/libbpf-rs that referenced this issue May 18, 2023
We have some complicated and non-obvious logic for skipping certain
tests when running cargo test from the root of the crate in CI, but then
an explicit invocation from the libbpf-rs/ sub-directory attempting to
run those skipped tests (now under sudo).
Simplify the logic and hopefully make it somewhat more obvious by
prefixing all tests that require root with `test_sudo_`.

Note: this is a workaround for what I'd call a bug in Cargo. I opened
rust-lang/cargo#12147 in the hopes that this
will be addressed eventually (though I don't know how realistic that
is). If we are still getting confusion even with this more explicit
naming, we may want to consider moving (or copying) libbpf-rs's .cargo/
directory into the workspace root.

Signed-off-by: Daniel Müller <deso@posteo.net>
danielocfb pushed a commit to libbpf/libbpf-rs that referenced this issue May 18, 2023
We have some complicated and non-obvious logic for skipping certain
tests when running cargo test from the root of the crate in CI, but then
an explicit invocation from the libbpf-rs/ sub-directory attempting to
run those skipped tests (now under sudo).
Simplify the logic and hopefully make it somewhat more obvious by
prefixing all tests that require root with `test_sudo_`.

Note: this is a workaround for what I'd call a bug in Cargo. I opened
rust-lang/cargo#12147 in the hopes that this
will be addressed eventually (though I don't know how realistic that
is). If we are still getting confusion even with this more explicit
naming, we may want to consider moving (or copying) libbpf-rs's .cargo/
directory into the workspace root.

Signed-off-by: Daniel Müller <deso@posteo.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

3 participants