-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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 doesn't rebuild either main or test binaries in a mixed language project #4979
Comments
As there hasn't been any activity here in over 6 months I've marked this as stale and if no further activity happens for 7 days I will close it. I'm a bot so this may be in error! If this issue should remain open, could someone (the author, a team member, or any interested party) please comment to that effect? The team would be especially grateful if such a comment included details such as:
Thank you for contributing! (The cargo team is currently evaluating the use of Stale bot, and using #6035 as the tracking issue to gather feedback.) If you're reading this comment from the distant future, fear not if this was closed automatically. If you believe it's still an issue please leave a comment and a team member can reopen this issue. Opening a new issue is also acceptable! |
I have noticed this too in ring. It seems lately when I modify only a C source file, the build script gets skipped. This used to work fine until sometime late last year. I suspect something changed where Cargo doesn't run the build.rs script as often as previously. |
@alexcrichton It looks like the fingerprints don't track build script results at all (src), which is a bit surprising to me. I think ideally the fingerprint would track the mtime of all of the libraries being linked against, but Cargo does not necessarily know the exact paths. Perhaps it could include the mtime of the time when the build script was run? Thoughts? Here's a test that demonstrates this problem: #[test]
fn dirty_both_lib_and_test() {
// This tests that all artifacts that depend on the results of a build
// script will get rebuilt when the build script reruns. It does the
// following:
//
// 1. Project "foo" has a build script which will compile a small
// staticlib to link against.
// 2. Build the library.
// 3. Build the unit test. The staticlib intentionally has a bad value.
// 4. Rewrite the staticlib with the correct value.
// 5. Build the library again.
// 6. Build the unit test. <-- ERROR HERE. This does not get rebuilt!
let slib = |n| {
format!(
r#"
#include <stdint.h>
int32_t doit() {{
return {};
}}
"#,
n
)
};
let p = project()
.file("Cargo.toml", r#"
[project]
name = "foo"
version = "0.1.0"
[build-dependencies]
cc = { git = "https://github.com/alexcrichton/cc-rs.git" }
"#)
.file(
"src/lib.rs",
r#"
extern "C" {
fn doit() -> i32;
}
#[test]
fn t1() {
assert_eq!(unsafe { doit() }, 1, "doit assert failure");
}
"#,
)
.file(
"build.rs",
r#"
use std::env;
use std::path::PathBuf;
fn main() {
let out_dir = PathBuf::from(env::var("OUT_DIR").unwrap());
cc::Build::new()
.file("slib.c")
.out_dir(&out_dir)
.compile("slib");
println!("cargo:rustc-link-lib=static=slib");
println!("cargo:rustc-link-search={}", out_dir.display());
}
"#,
)
.file("slib.c", &slib(2))
.build();
p.cargo("build").run();
p.cargo("test --lib")
.with_status(101)
.with_stdout_contains("[..]doit assert failure[..]")
.run();
p.change_file("slib.c", &slib(1));
p.cargo("build").run();
p.cargo("test --lib").run(); // ERROR: This fails when it should pass!
} Note that this test won't be usable as a real Cargo test due to the |
Whelp that'd do it! @ehuss that exact test is why we started including transitive dependencies in the fingerprint calculations, and I think your example perfectly shows why we also need to do it for build scripts and why that comment is wrong. I don't really remember why that comment was there and what breaks (if anything) if it's removed, but it definitely looks like we need to remove it. I think the bit of information we can track is that the fingerprint of a build script execution is the As for writing a test, in theory a staticlib should work... If you want to game out a fix though using |
Here's the issue I had with static linking. Create the following two files: slib.rs: #![no_std]
#[no_mangle]
pub extern "C" fn doit() -> i32 {
return 2;
}
use core::panic::PanicInfo;
#[panic_handler]
fn panic(_info: &PanicInfo) -> ! {
loop {}
} foo.rs: extern "C" {
fn doit() -> i32;
}
fn main() {
assert_eq!(unsafe { doit() }, 1, "doit assert failure");
} Building with: rustc --crate-type=staticlib -C panic=abort slib.rs
rustc foo.rs -l slib -L . Here's the error that I get.
|
Well that's a bummer. For reasons totally unrelated to Cargo that doesn't work, and it definitely shows a weak point of Rust in that it's actually surprisingly difficult to link two Rust libraries into the same binary if they're built independently (not in tandem through Cargo). Believe it or not though if you remove |
What does cargo do here? Deduplicate the symbols? |
Include build script execution in the fingerprint. This adds information about the execution of a build script to the fingerprint. Previously, no information was included, and cargo relied on dirty propagation in `JobQueue` to trigger recompiles. However, if two separate targets are built via separate commands (such as `cargo build` then `cargo test`), the second command did not know that the build script was updated, and thus was incorrectly treated as "fresh". This works by including the timestamp of the last time the build script was ran in the fingerprint. For overridden build scripts, it includes the replaced output. Fixes #4979
Demo repo: https://github.com/yshui/cargo-bug
This project uses a build script to compile a C file.
Steps to reproduce
Clone the repo. Run
cargo build
, andcargo test
. Both commands should do some compiling, which is expected.Change the C file,
c/test.c
. You can simplytouch
it.Run
cargo {build, test}
again, in any order.Expected behaviour
Both
cargo build
andcargo test
rebuildsActual behaviour
Only the first run command rebuilds, the second one doesn't. Leaving either the test binary or the main binary out-of-date
The text was updated successfully, but these errors were encountered: