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

cargo:rustc-link-arg-tests not available to unit/module tests #10937

Open
metasim opened this issue Aug 4, 2022 · 6 comments
Open

cargo:rustc-link-arg-tests not available to unit/module tests #10937

metasim opened this issue Aug 4, 2022 · 6 comments
Labels
A-build-scripts Area: build.rs scripts A-linkage Area: linker issues, dylib, cdylib, shared libraries, so C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@metasim
Copy link

metasim commented Aug 4, 2022

Problem

According to The Cargo Book, the build.rs file can emit the cargo:rustc-link-arg-tests=FLAG setting to have it applied passed to rustc as a -C link-arg=FLAG but only when building a "tests target". From the documentation it's not immediately clear that this only works with integration-style tests in tests/. Furthermore, there's no indication of how special link args might be specified for unit tests (this is particularly relevant for running tests for PyO3 extension modules on MacOS).

Also, if there are no tests tests/, the cargo test comands fails with the message:

error: invalid instruction `cargo:rustc-link-arg-tests` from build script of ...
The package xyz ... does not have a test target

Steps

1. Create a project with the following files:

Cargo.toml:

[package]
name = "build-script-link-args-for-tests"
version = "0.1.0"
edition = "2021"
[dependencies]

build.rs:

fn main() {
    println!("cargo:rustc-link-arg-tests=-Wl,-rpath,/usr/local/lib") // Arbitrary flag.
}

src/lib.rs:

pub struct Foo;

#[cfg(test)]
mod tests {
    #[test]
    fn can_haz_foo() {
        let _ = crate::Foo;
    }
}

2. Run tests

cargo test

(observe cargo "invalid instruction" error)

3. Add an integration test

tests/test_foo.rs:

use build_script_link_args_for_tests::Foo;

#[test]
fn i_haz_foo() {
    let _ = Foo;
}

4. Run tests

cargo test

(observe no cargo error)

5. Observe application of linker arguments

RUSTFLAGS='--print link-args' cargo test | grep '\-Wl,\-rpath,/usr/local/lib'

(observe linker arg only applied to .../build-script-link-args-for-tests/target/debug/deps/test_foo-...)

Possible Solution(s)

a. Allow cargo:rustc-link-arg-tests to apply to unit (in-module) tests, or create a new flag specific to unit tests. 😄
b. Update documentation to indicate unit tests aren't supported by this feature. 😞

Notes

May also apply to #[bench] functions.

Version

cargo 1.62.1 (a748cf5a3 2022-06-08)
release: 1.62.1
commit-hash: a748cf5a3e666bc2dcdf54f37adef8ef22196452
commit-date: 2022-06-08
host: aarch64-apple-darwin
libgit2: 1.4.2 (sys:0.14.2 vendored)
libcurl: 7.79.1 (sys:0.4.51+curl-7.80.0 system ssl:(SecureTransport) LibreSSL/3.3.6)
os: Mac OS 12.4.0 [64-bit]
@metasim metasim added the C-bug Category: bug label Aug 4, 2022
@ehuss
Copy link
Contributor

ehuss commented Aug 13, 2022

I'm not sure, but I think this was just an oversight. My original intent mentioned in #8441 (comment) was that it applied to unittests as well:

cargo:rustc-link-arg-tests=…         # equivalent to --tests (including unittests)

However, I think in #10274 that was an oversight.

Implementation-wise, I think the check applies_to needs to also consider the CompileMode. I'm not sure if it should check is_rustc_test or is_any_test; one would need to determine if it makes sense to also pass flags to rustdoc for doctests (I think maybe "yes"?).

@ehuss ehuss added A-linkage Area: linker issues, dylib, cdylib, shared libraries, so A-build-scripts Area: build.rs scripts labels Aug 13, 2022
@weihanglo
Copy link
Member

equivalent to --tests (including unittests)

If the "equivalent to --tests" means the equivalence of target selection, it implies

  • lib and bins as unit tests
  • no doctests

I am not sure if that's really what we want.

Also, what should cargo do when both cargo:rustc-link-arg-bins and cargo:rustc-link-arg-tests are specified?

  • Compile mode build
    • Only apply rustc-link-arg-bins. No problem in this case.
  • Compile mode test
    • Should we only apply rustc-link-arg-tests, or apply both tests and bins linker args?

Another thing I am uncertain is if a user invoke cargo t --example exp, do we want add link arg from cargo:rustc-link-arg-tests for that example's unit tests?

We've got some small details to determine. I feel like the most important question is "what is counted as a unit test?" in this feature. A simple solution could be adding link arg from cargo:rustc-link-arg-tests for every rustc invocation with --test flag. Though it feels inconsistent from others extra link arg.

@metasim
Copy link
Author

metasim commented Aug 31, 2022

Here are my thoughts on the points you brought up, but I must admit to knowing very little of the cargo internals and the various link stages it works with, so what I have to say may be of limited value.

If the "equivalent to --tests" means the equivalence of target selection, it implies

  • lib and bins as unit tests
  • no doctests

I am not sure if that's really what we want.

My wording there wasn't very precise, but I'm not sure what it should be. What it probably comes down to is how many linker invocations are there per target "kind", and does cargo expose those different cases outside of its internals?

To illustrate, say we have a project with the files tests/test_foo.rs, src/bin/foo.rs, and src/lib.rs, each with one or more #[test] function in them. If we run cargo test, does that result in 3 different linker invocations? If the answer is "yes", and cargo:rustc-link-arg-tests is all we have to work with, then either it should apply to all three linker invocations, or we need a new feature to specify one or more of the others separately. After reading your points, I think the latter may be needed.

The problem is we have two dimensions we're trying to treat as one. We have (what I'm calling) the "target kind" (bins, lib, examples, benches, tests), and then two modes: build and test. build mode is understood and clear: create the artifacts for when #[cfg(not(test))] (except for tests, which is just skipped in build mode?). Each of the target kinds in this mode already have their own cargo:rustc-link-arg-XXX flag and is covered.

However, in test mode, when #[cfg(test)], we may need different linker flags depending on the target kind; that is, the location of where the #[test] annotation appears (a bin file, a lib file, a tests file, an example file, etc.). Right now cargo:rustc-link-arg-tests only applies to the tests kind, but we see that's not enough. But applying it to all the target kinds seems wrong too. So I'm not seeing anyway around having separate test mode flags for the different kinds. Something like this:

Target Kind Flag Note
tests cargo:rustc-link-arg-tests As it is now
bins cargo:rustc-link-arg-test-bins
lib cargo:rustc-link-arg-test-lib
examples cargo:rustc-link-arg-test-examples
benches cargo:rustc-link-arg-test-benches Is this needed?

I'm not sure where doctests fit in here. In this context, is it a "kind" that's only relevant in the test mode (like tests)?

Does this feel directionally correct?

@weihanglo
Copy link
Member

Thank your for providing a solution on this topic! Most of your understanding is correct. Each test target, either unit test or integration test results in a linker invocation. You proposal is also straightforward IMO, though just feels like a bit like feature bloat (not your fault, just cargo provides so many options).

This #8441 (comment) proposes

cargo:rustc-link-arg-test-LIBNAME=…  # for unittest of lib only?

which is similar to your solution, and somehow may resolve your original use case. There are also syntaxes to specify the name of a target, although they have yet been implemented so far. I couldn't think a way to avoid adding such an amount of new instructions 😞.

@metasim
Copy link
Author

metasim commented Aug 31, 2022

Another option to keep it simple (until proven to be insufficient in the wild):

Target Kind Flag Note
tests cargo:rustc-link-arg-tests As it is now
not tests cargo:rustc-link-arg-tests-unit #[test] anywhere but ./tests

If down the road we need to differentiate between bins and lib, etc. , then we could add an optional [KIND={bins|lib|doctest|...}] specifier, but hold off until that day arrives.

@hydra
Copy link

hydra commented Jun 23, 2023

I bumped into this today. I have some embedded 'bin' code which uses a custom linker script to collect some static items tagged with a linker section so they appear next to each other in the resulting binary, the linker script also exposes start/end addresses so that the entire set can be accessed from rust code. It all works fine, but then I went to write so unit tests and ran into the error below.

error: invalid instruction `cargo:rustc-link-arg-tests` from build script of `my-crate v0.1.0 (...)`

build.rs snippet:

    println!("cargo:rustc-link-arg-tests=-Tlink/registry.ld");

Here's my use-case so you can see why I need this:

linker script snippet:

.registry_items :
{
    . = ALIGN(4);
    KEEP(*(.registry.items))
    KEEP(*(.registry.items.*))
} > CODE

_registry_items_section_vma_start = ADDR(.registry_items);
_registry_items_section_vma_end   = ADDR(.registry_items) + SIZEOF(.registry_items);
extern "C" {
    pub static mut _registry_items_section_vma_start: u32;
    pub static mut _registry_items_section_vma_end: u32;
}

...

let first: *const RegistryItem = &_registry_items_section_vma_start as *const u32 as _;
let last: *const RegistryItem = &_registry_items_section_vma_end as *const u32 as _;

let item_count: usize = last.offset_from(first) as _;

Registry {
    items: slice::from_raw_parts(first, item_count),
    ...
}

and the codebase has many RegistryItems like this, scattered over many source files.

#[link_section = ".registry.items.THING1"]
pub static REGISTRY_ITEM: RegistryItem = RegistryItem {
    length: mem::size_of::<Thing1>() as u16,
    id: things::THING1,
};

The idea is that the list of things is built at link-time and there is no source dependency between the 'things', or the Registry implementation itself.

To recap - my goal is add to the linker script used by unit tests so that my '.registry.items.*' appear in a contiguous block of memory instead of where they would usually be placed.

@ehuss ehuss added the S-needs-team-input Status: Needs input from team on whether/how to proceed. label Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-build-scripts Area: build.rs scripts A-linkage Area: linker issues, dylib, cdylib, shared libraries, so C-bug Category: bug S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

4 participants