Code changes look good but Darwin fails on ofborg.

It works on aarch64-darwin, but not x86_64-darwin.
sys/timex.h is missing, it should be there on after macOS 10.13, guessing the SDKs are too old?

Do you know how to change them to something newer if I'm not explicitly using any framework?

So, essentially, I have to do all the work from scratch?
Don't the commits usually get squashed on merge anyway?
What's the point?

@vifino The convention is to not squash, but I think it's good like this and I appreciate the effort to split it into independent commits! This notably aligns with the commit conventions:

Create a commit for each logical unit.

There's also this:

If you have commits pkg-name: oh, forgot to insert whitespace: squash commits in this case. Use git rebase -i.

But this doesn't apply here, and I'm pretty sure there's no other guidelines that would imply having to squash the commits in this PR more, so I'd like to ask @fpletz to reconsider blocking on this. If there's a good reason to squash more in certain cases, we should discuss it separately and then amend the guidelines so we can refer to it, as suggested in C4 (search for "value judgments").

Independently of the above though, we do have a new codeowner validator CI check, which is now detecting a problem with this PR:

    [err] line 229: "/pkgs/tools/networking/chrony" does not match any files in repository

You'll need to change this line to fix that:

This would fit well into the first commit (you can do git rebase -i to amend a change to the first commit, see here for a tutorial if you don't know about it already)

Hey @infinisil, thank you for elaborating on this!

Just so we're on the same page:
The initial issue with the PR was that I committed the move, reformat, update and fixup for darwin in a single commit.
To me, that seemed OK as I believe to have seen it done this way in the past.
My assumption of "logical unit" is a lot more coarse, think split by package/module/tests/docs.
It is not very defined and probably differs a lot, developer to developer.

My annoyance with the request to split it into multiple (fine-detailed) commits was that I essentially needed to redo the work in it's entirety for - to me - questionable benefits, since I had (incorrectly) assumed that it would be squashed - not rebased - just judging from a quick glance at the nixpkgs commit history.

In my personal experience, reviewing the changes requires to look at the entire diff - if not the whole file/derivation - as skipping a "run the formatter" commit in the review process could yield.. bad outcomes.
Hence I was a bit upset at the request, since doing that just for the review of (to me) trivial changes seemed a bit pointless, as the resulting file looks very readable.

@fpletz was actually chiming in to tell me that I don't need to as many individual commits as initially requested, trying to save me work, not squashing down fine-grained commits.

However, after I resigned to the fact that the only way I can get this approved is by redoing all the changes, I followed my interpretation of the initial demands as best as I could, yielding the current state (which took me about an hour).

You'll need to change this line to fix that:

nixpkgs/.github/CODEOWNERS

Line 229 in bf3905a

/pkgs/tools/networking/chrony @thoughtpolice

I assume I'd only change the path, correct?

@vifino Ah I see, thanks for the clarification, and sorry for the misinterpretation, @fpletz!

I do think there is a case to be made for granular commits to a degree, not just for the sake reviews, but also history, as it makes reverting, bisecting and blaming nicer (could use .git-blame-ignore-revs). So I think it's best to get into the habit of committing often, because it's easy to squash multiple commits together. Going the other way is hard, so I wouldn't want to impose that if it's not too beneficial.

Sounds like we need to clarify the guidelines on this to hint at the right balance :)

Edit: #347586

You'll need to change this line to fix that:

nixpkgs/.github/CODEOWNERS

Line 229 in bf3905a

/pkgs/tools/networking/chrony @thoughtpolice

I assume I'd only change the path, correct?

Yup!

Updated the ci/OWNERS file now, following #347610!

@fpletz was actually chiming in to tell me that I don't need to as many individual commits as initially requested, trying to save me work, not squashing down fine-grained commits.

Yup, that was my intention. Some degree of separation in the commits does makes sense but we don't have to be too pedantic about it. Generally, I don't like nitpicking in PRs and tend to just force push my nitpicks rather than put the burden on the PR author to fix minor issues.

My personal interpretation of the rules is that we should make it easy to revert or backport something that later turns out to break something. Changes like formatting or the by-name move most probably won't get reverted so that's fine. The version bump might if we find issues with the release that can't be trivially fixed.

Anyway, thanks everyone for chiming in here and @vifino for separating the commits. 🎉