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

Build script fingerprints do not track dependencies to links build scripts. #6780

Closed
ehuss opened this issue Mar 23, 2019 · 0 comments · Fixed by #6832
Closed

Build script fingerprints do not track dependencies to links build scripts. #6780

ehuss opened this issue Mar 23, 2019 · 0 comments · Fixed by #6832
Labels
A-build-scripts Area: build.rs scripts A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2019

The fingerprint for running a build script does not include information about links dependency build scripts. This means that if the dependent build script runs, but the top-level one does not (such as if it was cancelled, or built separately), it will not re-run the top-level build script.

Fixing this may be quite difficult. Due to the way build script Fingerprint::locals are recomputed within the closure, the information for the links dependencies is not readily available. These Fingerprints could be shared with Arc in Context::fingerprints, but that would require making locals a Mutex (or changing the entire Fingerprint to be Arc<Mutex<Fingerprint>>). This is also complicated by the fact that build script fingerprints need access to Context::build_explicit_deps which is not pre-populated before fingerprints are computed.

Below is a demonstration of the problem.

#[test]
fn links_interrupted_can_restart() {
    // Test for a `links` dependent build script getting canceled and then
    // restarted. Steps:
    // 1. Build to establish fingerprints.
    // 2. Change something (an env var in this case) that triggers the
    //    dependent build script to run again. Kill the top-level build script
    //    while it is running (such as hitting Ctrl-C).
    // 3. Run the build again, it should re-run the build script.
    let bar = project()
        .at("bar")
        .file(
            "Cargo.toml",
            r#"
            [package]
            name = "bar"
            version = "0.5.0"
            authors = []
            links = "foo"
            build = "build.rs"
            "#,
        )
        .file("src/lib.rs", "")
        .file(
            "build.rs",
            r#"
            fn main() {
                println!("cargo:rerun-if-env-changed=SOMEVAR");
            }
            "#,
        )
        .build();

    let p = project()
        .file(
            "Cargo.toml",
            &format!(
                r#"
                [package]
                name = "foo"
                version = "0.5.0"
                authors = []
                build = "build.rs"

                [dependencies.bar]
                path = '{}'
                "#,
                bar.root().display()
            ),
        )
        .file("src/lib.rs", "")
        .file(
            "build.rs",
            r#"
            use std::env;
            fn main() {
                println!("cargo:rebuild-if-changed=build.rs");
                if std::path::Path::new("abort").exists() {
                    panic!("Crash!");
                }
            }
            "#,
        )
        .build();

    p.cargo("build").run();
    // Simulate the user hitting Ctrl-C during a build.
    p.change_file("abort", "");
    // Set SOMEVAR to trigger a rebuild.
    p.cargo("build")
        .env("SOMEVAR", "1")
        .with_stderr_contains("[..]Crash![..]")
        .with_status(101)
        .run();
    fs::remove_file(p.root().join("abort")).unwrap();
    // Try again without aborting the script.
    // ***This is currently broken, the script does not re-run.
    p.cargo("build -v")
        .env("SOMEVAR", "1")
        .with_stderr_contains("[RUNNING] [..]/foo-[..]/build-script-build[..]")
        .run();
}
@ehuss ehuss added C-bug Category: bug A-rebuild-detection Area: rebuild detection and fingerprinting A-build-scripts Area: build.rs scripts labels Mar 23, 2019
alexcrichton added a commit to alexcrichton/cargo that referenced this issue Apr 10, 2019
Ever since the inception of Cargo and the advent of incremental
compilation at the crate level via Cargo, Cargo has tracked whether it
needs to recompile something at a unit level in its "dependency queue"
which manages when items are ready for execution. Over time we've fixed
lots and lots of bugs related to incremental compilation, and perhaps
one of the most impactful realizations was that the model Cargo started
with fundamentally doesn't handle interrupting Cargo halfway through and
resuming the build later.

The previous model relied upon implicitly propagating "dirtiness" based
on whether the one of the dependencies of a build was rebuilt or not.
This information is not available, however, if Cargo is interrupted and
resumed (or performs a subset of steps and then later performs more).
We've fixed this in a number of places historically but the purpose of
this commit is to put a nail in this coffin once and for all.

Implicit propagation of whether a unit is fresh or dirty is no longer
present at all. Instead Cargo should always know, irrespective of it's
in-memory state, whether a unit needs to be recompiled or not. This
commit actually turns up a few bugs in the test suite, so later commits
will be targeted at fixing this.

Note that this required a good deal of work on the `fingerprint` module
to fix some longstanding bugs (like rust-lang#6780) and some serious hoops had to
be jumped through for others (like rust-lang#6779). While these were fallout from
this change they weren't necessarily the primary motivation, but rather
to help make `fingerprints` a bit more straightforward in what's an
already confusing system!

Closes rust-lang#6780
bors added a commit that referenced this issue Apr 11, 2019
Remove `Freshness` from `DependencyQueue`

Ever since the inception of Cargo and the advent of incremental
compilation at the crate level via Cargo, Cargo has tracked whether it
needs to recompile something at a unit level in its "dependency queue"
which manages when items are ready for execution. Over time we've fixed
lots and lots of bugs related to incremental compilation, and perhaps
one of the most impactful realizations was that the model Cargo started
with fundamentally doesn't handle interrupting Cargo halfway through and
resuming the build later.

The previous model relied upon implicitly propagating "dirtiness" based
on whether the one of the dependencies of a build was rebuilt or not.
This information is not available, however, if Cargo is interrupted and
resumed (or performs a subset of steps and then later performs more).
We've fixed this in a number of places historically but the purpose of
this commit is to put a nail in this coffin once and for all.

Implicit propagation of whether a unit is fresh or dirty is no longer
present at all. Instead Cargo should always know, irrespective of it's
in-memory state, whether a unit needs to be recompiled or not. This
commit actually turns up a few bugs in the test suite, so later commits
will be targeted at fixing this.

Note that this required a good deal of work on the `fingerprint` module
to fix some longstanding bugs (like #6780) and some serious hoops had to
be jumped through for others (like #6779). While these were fallout from
this change they weren't necessarily the primary motivation, but rather
to help make `fingerprints` a bit more straightforward in what's an
already confusing system!

Closes #6780
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-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant