Require a comment for each patch?

roberth · June 27, 2023, 4:58pm

Hi Nixpkgs maintainers,

I figured the following would be a de facto standard, because it helps other maintain a patch:

patches have a comment describing either the upstream URL or a reason why the patch wasn’t upstreamed

… but maybe it’s not a standard yet?
What would be a valid reason not to have such a comment?

See doc/reviewing-contributions: Add points about patches by roberth · Pull Request #240136 · NixOS/nixpkgs · GitHub

vcunat · June 27, 2023, 7:27pm

Sounds OK. I suppose you don’t mean trivial cases like:

patch is fetched from an upstream URL (so doesn’t need this comment)
it’s trivial and obviously Nix(OS)-specific (e.g. adjusting some paths)

abathur · June 27, 2023, 7:33pm

Hmm. On the first pass I read the below as meaning a comment on the intent/effect of the patch fetched from the upstream URL (which may not be evident from the URL or even the patch itself):

vcunat · June 27, 2023, 7:43pm

Well, either way the point needs reformulation if we can’t even agree on its meaning.

jtojnar · June 27, 2023, 8:07pm

Ideally, we each patch would include both a short explanation (so that people looking at the expression can know why it is needed at a glance), and a upstream status/URL (when more in depth analysis is being done).

This is the explanation nixpkgs-hammering links to when it encounters a patch without a comment:

github.com

jtojnar/nixpkgs-hammering/blob/243b81c687aac33d6716957c0cd2235c81631044/explanations/missing-patch-comment.md

Each patch in nixpkgs applied to the upstream source should be documented. Out-of-tree patches, fetched using `fetchpatch`, are preferred.

In each patch comment, please explain the purpose of the patch and link to the relevant upstream issue if possible. If the patch has been merged upstream but is not yet part of the released version, please note the version number or date in the comment such that a future maintainer updating the nix expression will know whether the patch has been incorporated upstream and can thus be removed from nixpkgs.

Furthermore, please use a _stable_ URL for the patch. Rather than, for example, linking to a GitHub pull request of the form `https://github.com/owner/repo/pull/pr_number.patch`, which would change every time a commit is added or the PR is force-pushed, link to a specific commit patch in the form `https://github.com/owner/repo/commit/sha.patch`.

Here are two good examples of patch comments:

```nix
mkDerivation {
  …

  patches = [
    # Ensure RStudio compiles against R 4.0.0.
    # Should be removed next 1.2.X RStudio update or possibly 1.3.X.
    (fetchpatch {
      url = "https://github.com/rstudio/rstudio/commit/3fb2397c2f208bb8ace0bbaf269481ccb96b5b20.patch";
      sha256 = "0qpgjy6aash0fc0xbns42cwpj3nsw49nkbzwyq8az01xwg81g0f3";
    })
  ];

This file has been truncated. show original

sternenseemann · June 28, 2023, 7:13pm

In most cases an explanatory comment is good. I think it is also important to pick a good patch name which I feel like which is often neglected when using fetchpatch. That does not only improve code readability, but also causes the build output to make more sense (maybe we should remove the default for name in fetchpatch…). In some cases, the patch name alone is enough for the entry in patches to make sense.