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

Switching a build script from new style to old style does not fingerprint correctly. #6779

Open
ehuss opened this issue Mar 23, 2019 · 0 comments
Labels
A-build-scripts Area: build.rs scripts A-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug S-triage Status: This issue is waiting on initial triage.

Comments

@ehuss
Copy link
Contributor

ehuss commented Mar 23, 2019

If a build script switches from new-style (with rerun-if statements) to old-style (no rerun-if statements), then the fingerprint does not get updated correctly. This means that running cargo build a second time will re-run the script when it should be fresh.

Fixing this is somewhat difficult. The closure in prepare_build_cmd would need to recompute the old-style FingerprintLocal values, but those require access to a Context which is not available.

Below is a demonstration.

#[test]
fn build_script_to_old_style_rebuild() {
    // Changing from new-style to old-style should track freshness properly.
    let p = project()
        .file("src/lib.rs", "")
        .file("build.rs",
            r#"
            fn main() {
                println!("cargo:rerun-if-changed=build.rs");
            }
            "#)
        .build();

    p.cargo("build").run();
    p.cargo("build -v").with_stderr("\
[FRESH] foo [..]
[FINISHED] [..]
").run();
    p.change_file("build.rs", "fn main() {}");
    p.cargo("build -v").with_stderr("\
[COMPILING] foo [..]
[RUNNING] [..]build.rs[..]
[RUNNING] [..]build-script-build[..]
[RUNNING] [..]lib.rs[..]
[FINISHED] [..]
").run();
    // This line is currently bugged and doesn't appear fresh.
    p.cargo("build -v").with_stderr("\
[FRESH] foo [..]
[FINISHED] [..]
").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
Centril added a commit to Centril/rust that referenced this issue Sep 16, 2019
…ichton

Fix build script sanitizer check.

rust-lang#64166 changed the way the sanitizer build scripts work. However, they were changed so that they switch between new-style to old-style cargo fingerprints. This trips up on rust-lang/cargo#6779.

It also causes rustbuild to panic.  If you build stage1 std (with sanitizers off), and then enable sanitizers, it panics.  (This is because the build scripts don't declare that they need to re-run.)

This PR will trip rust-lang/cargo#6779 again, unfortunately. I've been having way too many unexplained rebuilds in rust-lang/rust recently, but at least I'll know why this time.

This doesn't fix all problems with the build scripts, but arguably they should be fixed in cargo. For example, the build scripts change which rerun-if statements they declare between runs which triggers rust-lang/cargo#7362.

The test for this is:
1. Turn off sanitizers (which is the default)
2. `./x.py build --stage=1 src/libstd`
3. `./x.py build --stage=1 src/libstd` again should be a null build.
4. Enable sanitizers.
5. `./x.py build --stage=1 src/libstd` should rebuild with sanitizers enabled.
6. `./x.py build --stage=1 src/libstd` again should be a null build. This actually rebuilds due to rust-lang/cargo#7362 because the rerun-if directives changed between step 3 and 5. A 3rd attempt should be a null build.
@epage epage added the S-triage Status: This issue is waiting on initial triage. label Oct 31, 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-rebuild-detection Area: rebuild detection and fingerprinting C-bug Category: bug S-triage Status: This issue is waiting on initial triage.
Projects
None yet
Development

No branches or pull requests

2 participants