-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
libgit2: introduce libgit2_1_8 attribute and init at 1.8.1; cargo-generate: 0.21.1 -> 0.21.3 #333660
Conversation
This is going to cause issues for |
so apprently I don't have enough disk space for nixpkgs-review to finish up. I definitely expect this to cause issues. Sadly I don't know how many. Is there any consensus on solution here? I would maybe be for introduction of new attribute for new version so we can roll it out more incrementally. That would also unblock the work on rust. |
We can introduce another attribute if necessary, though of course we don’t want to let version proliferation go on for longer than necessary. If it’s blocking the Rust stuff, then that’s probably for the best, as the number of rebuilds here is high enough that this would need to go to My proposal:
|
Although having said that, the number of rebuilds here really isn’t that bad. I’m running a |
The number of rebuilds is not as big to be "insanely huge" but it's still significant enough plus I feel some packages impacted are relatively important. Plan you outlined sounds good to me. |
I also noticed there is an override for turbogit in top-level. Not a problem but it seems relevant so just for the future reference: |
Yes, the version flip will definitely have to go to
Nice |
7acc268
to
7ae7b28
Compare
pkgs-by-name check is failing but I feels wrong to me to follow it in this case. I'll be waiting for reviews. |
Yes. We can ignore it in this case. |
Just drop turbogit. The package is super unmaintained and broken. |
@@ -1,3 +1,6 @@ | |||
{ version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This skills be generic.nix
@@ -20372,7 +20372,13 @@ with pkgs; | |||
|
|||
icon-lang = callPackage ../development/interpreters/icon-lang { }; | |||
|
|||
libgit2 = callPackage ../development/libraries/libgit2 { | |||
# Should be flipped for version 1.8.x soon | |||
libgit2 = callPackage ../development/libraries/libgit2/1-7-2.nix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't flip back the default, just pin packages requiring an old version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would mean this has to go to staging, which would mean that more Rust stuff will be broken after the 1.80 merge in a few days. We should not flip the default immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuperSandro2000 Plan is to do exactly that but in next PR that will go to staging. This one is just first step to unblock some merges to master in meantime.
FWIW here are the unfiltered nixpkgs-review results from the simple bump: Result of 209 packages marked as broken and skipped:
146 packages failed to build:
2709 packages built:
|
And here’s an approximate list of true regressions vs.
And an approximate list of root cause regressions (i.e., regressions that don’t seem to be because of depending on another regression):
Looks quite manageable for the next staging cycle. |
I'm a bit confused — why don't we just pin old packages on staging-next, and direct master PRs that need this to staging-next as well? |
My impression was that staging-next was sufficiently far enough along that we shouldn’t cause another ~3k rebuilds and more potential breakage, so it’d have to go to the next cycle. If we think it’s okay to bump |
Okay so after discussing on Matrix, it uh… turns out that we already bumped libgit2 to 1.8 this staging cycle! I’m sorry for wasting your time by not having checked that at the start here. Those regressions are already issues that we’ll have to deal with separately. We should be able to just bump |
That's not a big deal at all. This was low effort on my part so far anyway. And I should have checked that myself anyway so. If that's the case we can just merge #333488 (comment) directly to staging-next. Regarding regressions - I'm less sure about what these will require. Maybe we will need attribute for the older version. Let me know if you need some help there. I can maybe help to make some of these annoying mechanical fixes. |
I’m hopeful we can avoid reintroducing 1.7. Hydra would be a big blocker there, but the failure looks like it might be general flakiness or otherwise unrelated to libgit2. Jujutsu would hurt me personally, but is already being worked on upstream. If you wanted to look at any regressions in particular, I’d suggest |
Hydra built fine on |
Going to close this out since we merged the other PR. |
Ah, |
likely but in statging-next right? We need to deal with these separetely... |
Yeah, there are still regressions (e.g. |
Introduce new attribute for new version of libgit2 (1.8.1) to allow for incremental update of downstream dependencies & Update cargo-generate.
Related to #332957 (comment)
Outdated
Blocks #333488
There will be a lot of packages depending on libgit2 so this will cause huge rebuilds. I'm currently running nixpkgs-review on my side to see the impact.
requires review from @SuperSandro2000
Another approach might be to introduce new version of libgit2 along side the existing one.
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.