2023-10-23 Nix team meeting minutes #97

Attendees: @edolstra @roberth @ericson2314 @regnat @fricklerhandwerk @tomberek @infinisil
Notes: @infinisil @fricklerhandwerk


Funding Nix C++ development time

Feedback to the team from people at Ocean Sprint

The team appaers more responsive, and it feels like it’s possible to land changes. Yay!

String optimization effort by @tomberek

Some string operations are very frequent and more expensive than what they should be. @tomberek tried optimizing that, but harder than expected. @tomberek and @roberth will pair on that.

`nix profile`: Allow referring to elements by human-readable name by iFreilicht · Pull Request #8678 · NixOS/nix · GitHub

  • Idea already approved, previously assigned to @edolstra
  • Will look at it in the work part of the meeting


Merged 5 days after opening, without completed reviews, a lot of code, @roberth and @Ericson2314 not happy with the process.


  • Try it out: Wait for approval by at least one reviewer (not technically enforced to allow for quick trivial changes)
  • Establish a routine to split out tangential discussion into issues
  • Stay on the course towards working on smaller changes
  • Finish review on this particular change after the fact
Complete discussion
  • Merged 5 days after opening, without completed reviews, a lot of code
  • @edolstra wanted to make progress, didn’t think it was a big problem, reviews are very detailed
  • @Ericson2314: Should have the opportunity to discuss the design properly
  • @edolstra: Lazy tree backports create a lot of work if they stay open, needs a lot of rebasing
  • @fricklerhandwerk: Let’s break down changes more if they don’t fly immediately as discussed recently
    • @roberth/@Ericson2314: This PR looked well broken down, just needed more review time, still a lot of code
  • @tomberek: Communication issue, whether self-merging is okay, to wait for review approvals, talk more
  • @roberth: At least one approving review needed
    • @edolstra: Don’t agree, stops progress, don’t want that for trivial PR’s
      • @roberth: Trivial PR’s are still okay to self-merge
    • @fricklerhandwerk: It’s not about asking for permission if we agree on direction. Reviews are mainly for:
      • Knowledge sharing of code, ideas and vision, both ways author ↔ reviewer
      • Informing readers about the change
  • @infinisil: Review PR afterwards?
    • @roberth: Yeah let’s review afterwards, no need to revert
  • @edolstra: The problem is not reviews per se, but many reviews devolving into architecture astronautics

Split-up reviews


Awesome, thanks! Just a small remark: it wasnt clear to me that https://github.com/NixOS/nix/pull/8678 was previously already accepted. The previous meeting notes only mentioned assignment of review, does that imply an idea being accepted as well?

Yes, that is often implied. If review is assigned, there is nothing to discuss on the team, which means we want it merged eventually.

Thanks :+1: That’s good to know. I was a bit afraid that the idea of nix profile as a whole might not be accepted.