Skip to content

flakes: No longer compute revCount for local git repository by default #13260

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Mic92
Copy link
Member

@Mic92 Mic92 commented May 25, 2025

Computing a revCount is expensive and might not work if the local history is not complete.
This particular fixes a long standing crash bug in Nix: #6073

Motivation

Depends on #13265

Context


Add 👍 to pull requests you find important.

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

@Mic92 Mic92 requested a review from edolstra as a code owner May 25, 2025 14:43
@github-actions github-actions bot added the fetching Networking with the outside (non-Nix) world, input locking label May 25, 2025
@Mic92 Mic92 added backport 2.24-maintenance Automatically creates a PR against the branch backport 2.28-maintenance Automatically creates a PR against the branch backport 2.29-maintenance Automatically creates a PR against the branch and removed backport 2.24-maintenance Automatically creates a PR against the branch labels May 25, 2025
@Mic92 Mic92 force-pushed the fix-shallowish-clones branch from 5b13707 to 8b5e637 Compare May 25, 2025 15:06
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 25, 2025
@Mic92 Mic92 force-pushed the fix-shallowish-clones branch 7 times, most recently from fc19348 to fb64d5b Compare May 26, 2025 07:29
@Mic92 Mic92 changed the title fix revCount in worktrees that are derived from shallow clones flakes: No longer compute revCount for local git repository by default May 26, 2025
@Mic92 Mic92 dismissed roberth’s stale review May 26, 2025 08:08

revCount still throws an error as before.

@Mic92
Copy link
Member Author

Mic92 commented May 26, 2025

Flake regression is unrelated (500er http github error).

@edolstra
Copy link
Member

edolstra commented May 26, 2025

This needs a release note since it's an incompatible change (and we probably shouldn't backport it for that reason).

BTW, maybe as an alternative we can make revCount computation lazy. I.e. it would only be computed if the flake actually uses the revCount attribute of the source tree.

@Mic92 Mic92 removed backport 2.28-maintenance Automatically creates a PR against the branch backport 2.29-maintenance Automatically creates a PR against the branch labels May 26, 2025
@Mic92
Copy link
Member Author

Mic92 commented May 26, 2025

This needs a release note since it's an incompatible change (and we probably shouldn't backport it for that reason).

Yeah. I had the label still assigned before I made this change, when it was still a fix.

BTW, maybe as an alternative we can make revCount computation lazy. I.e. it would only be computed if the flake actually uses the revCount attribute of the source tree.

I considered the alternative but decided against it because it would still than try to compute the revCount when we override flake inputs in other flakes in which case revCount will always be recorded and checked in the flake.lock.

@DavHau
Copy link
Member

DavHau commented May 26, 2025

If we'd just drop revCount, or make it opt-in, I wouldn't particularly mind it.

I fully agree. revCount should be remove from flakes and fetchTree.
(alternatively opt-in)

As expensive as computing revCount is, it doesn't justify the benefit. Its only use case seems to be generating a version string dynamically, but the shortRev can be used instead.

@grahamc
Copy link
Member

grahamc commented May 26, 2025

Flake regression is unrelated (500er http github error).

On that note, FlakeHub prohibits a hard revcount dependency because it loses that data once it is published. In other words, it is unlikely a primarily-flakehub backed corpus would fail if the feature went away.

We did find a fair number of our own repository depended on the revcount to evaluate at first, which we ended up fixing. I'd encourage some sort of graceful upgrade path here, like continuing to calculate the revCount for the next N releases paired with a strong evaluation-time admonition about it going away.

Shallow clones are faster to access because we don't have to compute the
revCount, which in sparse checkouts might not even exists.

This is especially useful in combination with lazy trees in mind on large
repository such as nixpkgs.
@Mic92 Mic92 force-pushed the fix-shallowish-clones branch from fb64d5b to 09a7ce9 Compare May 26, 2025 13:19
@Mic92 Mic92 marked this pull request as draft May 26, 2025 13:23
@Mic92
Copy link
Member Author

Mic92 commented May 26, 2025

Flake regression is unrelated (500er http github error).

On that note, FlakeHub prohibits a hard revcount dependency because it loses that data once it is published. In other words, it is unlikely a primarily-flakehub backed corpus would fail if the feature went away.

We did find a fair number of our own repository depended on the revcount to evaluate at first, which we ended up fixing. I'd encourage some sort of graceful upgrade path here, like continuing to calculate the revCount for the next N releases paired with a strong evaluation-time admonition about it going away.

Ok. Than we need to make the revCount attribute lazy so we can issue a warning on first access.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants