npc: init at 1.0.0 - #543008
npc: init at 1.0.0#543008
Conversation
|
|
||
| env = { | ||
| NPC_REV = finalAttrs.src.rev; | ||
| GIT_BIN = lib.getExe git; |
There was a problem hiding this comment.
Unfortunately this is wrong; see samestep/npc#6. What do you think is the best solution here?
- Try sending another email on the Git mailing list to see if people respond this time?
- Add Jonathan Tan's one-line patch to the Git package definition in Nixpkgs, and target
staging. - Use that patch but only for the Git that gets fed to
npcitself. - Something else?
There was a problem hiding this comment.
I spotted that; I'd assumed (perhaps naively) that the bug was at least rare, so it'd be fine to just live with it until it gets fixed in the Git source code.
That said, I rather expect submitting Jonathan Tan's one-line patch as a properly formatted patch to the Git mailing list would get it accepted. And once there's a patch on the mailing list, regardless of whether it's accepted or not, I think it'd be reasonable to add it to the Nixpkgs Git package.
There was a problem hiding this comment.
I'd assumed (perhaps naively) that the bug was at least rare, so it'd be fine to just live with it until it gets fixed in the Git source code.
It's been several months since I've tried running npd with Git 2.48+, but if I remember correctly, I was hitting the bug pretty consistently before I pinned the older Git version. If it's not too much to ask, would you be able to try it out on your end to see if you hit it?
That said, I rather expect submitting Jonathan Tan's one-line patch as a properly formatted patch to the Git mailing list would get it accepted.
Yeah I think you're right 😅 to be honest, I've had this on my TODO list for months now. Obviously the patch itself is super easy, but then I'd also have to include a regression test, and I just haven't gotten around to understanding the Git codebase's testing setup well enough to feel confident in writing and sending it...
There was a problem hiding this comment.
FWIW, I just ran the following and didn't hit the problem in any of the ten test clones
( set -xeuo pipefail; for (( n=0; n<10; n++ )); do d="$(mktemp -d)"; git clone --mirror --filter=tree:0 https://github.com/NixOS/nixpkgs.git "$d"; rm -rf "$d"; done )
There was a problem hiding this comment.
Right, the initial clone works fine; it's the git fetch that gives the error. Although, I did also recently change how the fetching works in samestep/npc@8d5e25b, so maybe it avoids the bug now somehow? I'd be surprised though.
There was a problem hiding this comment.
Yep, that was 50 runs without reproducing the bug with git v2.54.0! I'll test with v2.49 now, just to be sure it's not an environmental issue...
There was a problem hiding this comment.
Oh wow! And I assume this is after the more recent change to how npc fetches? If you want, it may also be useful to try with an npc commit prior to that change.
I'll look into sending that patch on the Git mailing list later today.
There was a problem hiding this comment.
Okay, I'm not managing to reproduce the problem with git v2.49.0 either. Specifically, I've just run the following and didn't see any issues:
( set -euo pipefail; git () { /nix/store/lchj5sfm6yyhv5jhahh5xr2fg239218r-git-2.49.0/bin/git "$@"; }; d="$(mktemp -d)"; git clone --mirror --filter=tree:0 https://github.com/NixOS/nixpkgs.git "$d"; cd "$d"; for (( n=0; n<20; n++ )); do git fetch --no-show-forced-updates; done )
So I don't think we can conclude anything from my testing other than there's some environmental factor at play :(
There was a problem hiding this comment.
Thank you for testing! I'll do some more testing on my end later today.
There was a problem hiding this comment.
And as a slightly different datapoint: I just rebuilt a couple of my machines using npc from your flake, but with both nixpkgs and nixpkgs-git pinned to a recent nixpkgs-unstable. Twenty runs of npc fetch on two different systems and I didn't see anything untoward.
|
samestep
left a comment
There was a problem hiding this comment.
Welp. I tried a bunch yesterday and today to reproduce the git fetch issue, and I couldn't 🙃
So I'd say ship it! Thanks again for opening this PR 😄
|
Oh wait hang on I tried a bit more and I think I actually reproduced it this time; please hold... |
|
OK yep, turns out we're good 🥳 The bug was fixed by git/git@7a4bd1b, which was released in Git 2.53, so the Git 2.54 currently in Nixpkgs works great! I also did samestep/npc#44 upstream. |
|
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/3213 |
npc is an MIT-licensed tool for working with Nix channels and channel histories. In particular, it provides an
npc bisectcommand that provides similar function togit bisectbut only considering commits that have actually been pointed to by a relevant Nixpkgs release branch.@samestep I've added you as a maintainer, since you're the author, but you obviously get to opt out!
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.