-
-
Notifications
You must be signed in to change notification settings - Fork 13.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
CONTRIBUTING.md: patch conventions #317314
Conversation
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 should go here instead
ccede5b
to
ca25afe
Compare
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.
LGTM. I made an attempt a while ago at #246766 but somehow got distracted by some other things and forgot about it.
Patches available online should be retrieved using `fetchpatch`. | ||
Sometimes, changes are needed to the source to allow building a derivation in nixpkgs, or to get earlier access to an upstream fix or improvement. When using the `patches` parameter to `mkDerivation`, make sure the patch name clearly describes the reason for the patch, or add a comment. | ||
|
||
Patches already merged upstream or published elsewhere should be retrieved using `fetchpatch`. |
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.
Patches already merged upstream or published elsewhere should be retrieved using `fetchpatch`. | |
Patches already merged upstream, reviewed or published elsewhere should be retrieved using `fetchpatch`. |
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.
Thanks for your suggestions! I have merged the others but not this one, as I don't think it makes sense to fetchpatch
patches that have been reviewed but not merged: they might still get rebased/squashed and thus disappear.
As discussed in the NixOS Security Discussion Matrix room at https://matrix.to/#/!NBBFPbiuttRgTqbrcY:nixos.org/$kRB7R9ksae-78H4GQOO7NjMurKh7hSHimYSGMVE3w8k?via=nixos.org&via=matrix.org&via=nixos.dev and NixOS#317169 Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
7e64be9
to
cbfd7ee
Compare
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.
Looking good! Let's just merge, we can make incremental improvements in future PRs if necessary :)
Description of changes
As discussed in the NixOS Security Discussion Matrix room at https://matrix.to/#/!NBBFPbiuttRgTqbrcY:nixos.org/$kRB7R9ksae-78H4GQOO7NjMurKh7hSHimYSGMVE3w8k?via=nixos.org&via=matrix.org&via=nixos.dev and #317169
Things done
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.