1.52 regression? - patch no longer applied

[Mark-Simulacrum]

This is just a hunch, but since there's been some resolver work I thought I'd mention it:

• 1.51.0: https://crater-reports.s3.amazonaws.com/beta-1.52-1/1.51.0/gh/PvdBerg1998.fourier/log.txt
• 1.52.0 beta: https://crater-reports.s3.amazonaws.com/beta-1.52-1/beta-2021-03-27/gh/PvdBerg1998.fourier/log.txt

As you can see there's some build errors, which I suspect are due to the patch not getting applied on the beta version (as Cargo warns about). I don't know whether there's actually some bug here - haven't tried to reproduce - or if this is just some environment change as is not atypical on Crater.

[ehuss]

I believe this is caused by #9133.

I'm not entirely certain yet why patches aren't staying locked. Normal git dependencies work (although they do trigger an update to Cargo.lock). Here's a test to demonstrate:

#[cargo_test]
fn old_git_patch() {
    // Example where an old lockfile with an explicit branch="master" in Cargo.toml.
    Package::new("bar", "1.0.0").publish();
    let (bar, bar_repo) = git::new_repo("bar", |p| {
        p.file("Cargo.toml", &basic_manifest("bar", "1.0.0"))
            .file("src/lib.rs", "")
    });

    let bar_oid = bar_repo.head().unwrap().target().unwrap();

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

                    [dependencies]
                    bar = "1.0"

                    [patch.crates-io]
                    bar = {{ git = "{}", branch = "master" }}
                "#,
                bar.url()
            ),
        )
        .file(
            "Cargo.lock",
            &format!(
                r#"
# This file is automatically @generated by Cargo.
# It is not intended for manual editing.
[[package]]
name = "bar"
version = "1.0.0"
source = "git+{}#{}"

[[package]]
name = "foo"
version = "0.1.0"
dependencies = [
 "bar",
]
            "#,
                bar.url(),
                bar_oid
            ),
        )
        .file("src/lib.rs", "")
        .build();

    bar.change_file("Cargo.toml", &basic_manifest("bar", "2.0.0"));
    git::add(&bar_repo);
    git::commit(&bar_repo);

    // This *should* keep the old lock.
    p.cargo("tree")
        .with_stderr("")
        .with_stdout(
            "\
foo v0.1.0 [..]
└── bar v1.0.0 (file:///[..]branch=master[..])
",
        )
        .run();
}

[Mark-Simulacrum]

Hm, FWIW, we also saw a lot of breakage this crater run due to updated git dependencies (i.e., the git version used in the 1.52 build was different from 1.51's git version). So maybe the underlying problem is similar?

I had chalked that up to an old crater bug - rust-lang/crater#535 - but it's not impossible that it's actually related to this issue. I don't think there were patches in all of those cases though.

[ehuss]

Looking at the logs, it seems like crater does cargo fetch --locked, and if that fails it regenerates the lockfile. For these packages, the lockfile changes look like this:

diff --git a/Cargo.lock b/Cargo.lock
index 05f00d0..70168f8 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1,5 +1,7 @@
 # This file is automatically @generated by Cargo.
 # It is not intended for manual editing.
+version = 3
+
 [[package]]
 name = "adler32"
 version = "1.2.0"
@@ -160,7 +162,7 @@ dependencies = [
 [[package]]
 name = "embedded-layout"
 version = "0.1.0"
-source = "git+https://github.com/bugadani/embedded-layout.git#52261542db06a77ad0a70024caeaa5128cae024c"
+source = "git+https://github.com/bugadani/embedded-layout.git?branch=master#52261542db06a77ad0a70024caeaa5128cae024c"
 dependencies = [
  "embedded-graphics",
 ]

This is part of what #9133 is doing. The two lockfiles are essentially the same, but 1.52 elaborates it with more detail.

I'm curious, why does crater use --locked if it is going to regenerate the lockfile anyway? Seems like it would be easier and more reliable to just run cargo fetch without --locked.

[Mark-Simulacrum]

I think the intent is to regenerate only if there's some problem fetching - e.g., yanked deps or missing deps? Not quite clear. But the problem seems to be that https://github.com/rust-lang/rustwide/blob/283aa76407eb5d1d5b9399d241e3ebc3ee812c00/src/prepare.rs#L131 is not set here - https://github.com/rust-lang/rustwide/blob/283aa76407eb5d1d5b9399d241e3ebc3ee812c00/src/prepare.rs#L94. I'm not sure why.

@pietroalbini do you perhaps have context here?

@ehuss can you clarify why fetch --locked is failing here? I'd expect us to not need to edit the lockfile for this case (and the user has asked us not to). I wonder if there's some way we can "pre-apply" the version edits in a safe way. Crater generally wants to make sure that despite using two different cargos, the crate graph resolves to the same things (i.e., so that the build fails due to actual problems in the crate, rather than different resolution).

[pietroalbini]

I'm not sure why Crater ends up respecting the lockfile when testing repositories, as that was implemented before I started contributing and there isn't much documentation around. A possible guess I have is that it reflects the behavior of cloning that repository and running a build.

On when Crater regenerates the lockfile anyway: that happens when Cargo outputs "Cargo.lock needs to be updated but --locked was passed to prevent this".

[Mark-Simulacrum]

On when Crater regenerates the lockfile anyway: that happens when Cargo outputs "Cargo.lock needs to be updated but --locked was passed to prevent this".

This seems untrue. The logic I can see is linked in my comment above:

I think the intent is to regenerate only if there's some problem fetching - e.g., yanked deps or missing deps? Not quite clear. But the problem seems to be that https://github.com/rust-lang/rustwide/blob/283aa76407eb5d1d5b9399d241e3ebc3ee812c00/src/prepare.rs#L131 is not set here - https://github.com/rust-lang/rustwide/blob/283aa76407eb5d1d5b9399d241e3ebc3ee812c00/src/prepare.rs#L94. I'm not sure why.

and doesn't look for that string at all? Maybe I'm missing something.

I would expect crater to give an error during the prepare step here, which would then be much easier to diagnose as a test-pass vs. build-failed crater log, rather than continuing on with a changed log file.

[pietroalbini]

The check for that error message happens when running cargo fetch --locked. If Cargo outputs that message, Rustwide then "forces" generating a new lockfile and tries fetching the dependencies again:

https://github.com/rust-lang/rustwide/blob/283aa76407eb5d1d5b9399d241e3ebc3ee812c00/src/prepare.rs#L141-L159

self.lockfile_captured = true is only used to avoid infinite recursion when Cargo continues to output that message forever.

[ehuss]

can you clarify why fetch --locked is failing here?

I'm uncertain why. Anything involving patches ends up being kinda complicated.

The underlying issue is that historically Cargo treated git dependencies without a branch as "master". Even if you explicitly listed "master", the Cargo.lock would strip the "master" part. In 1.52, this is changing so that if there is no branch specified, it defaults to HEAD, and if you explicitly list "master" it will add "master" to the lockfile.

This should be mostly backwards compatible (except for some edge cases that have been issuing warnings for a while). I don't think cargo should be rewriting the lockfile here, and that's a bug I think. Normally it doesn't rewrite the lockfile for regular git dependencies, so this is a patch-specific thing.

I'm not sure why Crater ends up respecting the lockfile when testing repositories,

I'm still a bit unclear why it is using --locked. The logic was added in rust-lang/crater#405 to rebuild the lock file. What kind of errors was that PR fixing that wouldn't be resolved by just removing --locked?

[alexcrichton]

I think that this is indeed a bug in Cargo. Cargo should automatically be reusing the old entry in the lock file for this dependency. Unfortunately even after some poking around I'm not able to come up with a good solution. I sort of see where this is happening but when applying one fix it's just generating more problems that I don't know exactly where are coming from. The patch/lock/etc regions of Cargo were always super complicated...

[Mark-Simulacrum]

Do we have a sense whether a fix is likely here in time for the next release? If not, it would be good to write up the expected actions users should take (ideally ones that could let them retain compatibility with older releases too, though if not possible that's OK), so we can make a note in the main release notes compat section.

Alternatively, I suppose we could consider reverting #9133 on beta, to give more time to try to find a solution, but that seems a pretty heavy hammer...