libfetchers: namespace input fingerprint by scheme

[stepbrobd]

Motivation

i've been getting hash mismatch errors almost daily when there's update on flake input from dependabot and that dependency have a local checkout

Context

see https://github.com/stepbrobd/nix-cache-hash-mismatch-mre

---

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

[xokdvium]

Hm, maybe #9480 is not working correctly? export-ignore should be included in the fingerprint?

[xokdvium]

Probably due to the fact that the github: inputs don't reflect that it honors export-ignore:

    std::optional<std::string> getFingerprint(Store & store, const Input & input) const override
    {
        if (auto rev = input.getRev())
            return rev->gitRev();
        else
            return std::nullopt;
    }

[stepbrobd]

In Jitsi, did you mean that this patch will cause different fetchers to not be able to share cache even if the fetched content is the same (due to the namespacing), and the correct solution is to fix github: specific fingerprinting logic so that the github fetcher behaves the same as git fetcher exportIgnore set to true when applicable?

[stepbrobd]

apparently export-ignore is not the only thing that might cause problem

NixOS/nixpkgs#84312

and it seems like export-subst is also problematic

[xokdvium]

apparently export-ignore is not the only thing that might cause problem

Right, there are lots of things that can go wrong.

and the correct solution is

I wouldn't really call that a correct solution. I guess the big question is whether we even want to continue playing along with the notion that fetchers can be interchangeable (as was intended, probably by Eelco). I'm not sure I'm the right person to make this call, but there's a whole unreproducivility mess to untangle. The whole caching scheme relies on the fetchers being reproducible (which they aren't).

[stepbrobd]

whether we even want to continue playing along with the notion that fetchers can be interchangeable

If not this PR can be merged as is no? (jk 😂)

If we want to minimize the blast radius for now, would adding a discriminator to getFingerprint be acceptable? e.g. from the code block above:

        if (auto rev = input.getRev())
            return std::string(schemeName()) + "-archive:" + rev->gitRev();
        else
            return std::nullopt;

[xokdvium]

would adding a discriminator to getFingerprint be acceptable? e.g. from the code block above:

I guess if we want to be really conservative we could use the same scheme that git uses and append ;e to reflect the fact that export-ignore is honored?

[stepbrobd]

I'm a bit reluctant on just adding ;e because while this would resolve my specific issue with export-ignore, trees with export-subst (or some other weird gitattribute flags I don't know about) can still cause hash mismatch

At least at the moment, github fingerprint and git (with export ignore set) are not interchangeable

[xokdvium]

trees with export-subst

Ideally that would also be supported. (That was also noted in the PR adding export-ignore support) and the hash mismatch does provide a useful canary, while being annoying to users. Either way, that's a bug in the fetchers. I'm afraid that if we were to make those not interchangeable this will bitrot forever...

Let's see what @roberth has to say.

[xokdvium]

I still think that this is quite sad, but realistically it seems that playing whack-a-mole isn't going to work out. In case anyone ever wants to reintroduce a more sound approach in future - that's easily done without breakage.

Let's try this for now. Just the fact that we are only running into this now might suggest that the benefit of sharing the cache isn't all that worth it.