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?
doc/reviewing-contributions: Add points about patches by roberth · Pull Request #240136 · NixOS/nixpkgs · GitHub
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)
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):
Well, either way the point needs reformulation if we can’t even agree on its meaning.
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:
This file has been truncated.
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:
patches = [
# Ensure RStudio compiles against R 4.0.0.
# Should be removed next 1.2.X RStudio update or possibly 1.3.X.
url = "https://github.com/rstudio/rstudio/commit/3fb2397c2f208bb8ace0bbaf269481ccb96b5b20.patch";
sha256 = "0qpgjy6aash0fc0xbns42cwpj3nsw49nkbzwyq8az01xwg81g0f3";
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
fetchpatch…). In some cases, the patch name alone is enough for the entry in
patches to make sense.