-
-
Notifications
You must be signed in to change notification settings - Fork 15.4k
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
Small refactoring in coqPackages #49456
Conversation
Closes rocq-prover#8227 by solving remaining differences. Sets doNotFilter in anticipation of NixOS/nixpkgs#49456.
f0d1ae6
to
32bf813
Compare
Closes rocq-prover#8227 by solving remaining differences. Sets doNotFilter in anticipation of NixOS/nixpkgs#49456.
Closes rocq-prover#8227 by solving remaining differences. Sets doNotFilter in anticipation of NixOS/nixpkgs#49456.
pkgs/top-level/coq-packages.nix
Outdated
@@ -54,7 +54,8 @@ in rec { | |||
|
|||
mkCoqPackages = coq: | |||
let self = mkCoqPackages' self coq; in | |||
filterCoqPackages coq self; | |||
if coq.doNotFilter or false then self |
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.
if coq.doNotFilter or false then self | |
if coq.doNotFilter then self |
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.
Alright. Let me add some documentation.
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.
Right! It's the a.b or c
syntax. I can't believe I fell for that.
Do we really need the negated name? Negated names require unnecessary extra thinking. What about "setting doFilter = false
" which has a default of true?
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.
Maybe Théo has an opinion. Ping @Zimmi48 .
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.
Ah I did not know about the syntax, will use ;-)
Regarding doFilter = false
vs doNotFilter = true
, it seems that the implicit convention in nixpkgs (https://nixos.org/nixpkgs/manual/#sec-stdenv-phases) is that when something is off by default, it is enabled with doStuff = true
but when something is on by default, it is disabled dontStuff = true
. Thus, it suggests to use dontFilter
here.
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.
Let’s go with dontFilter
.
32bf813
to
9014c5f
Compare
ea820d0
to
33face8
Compare
Closes rocq-prover#8227 by solving remaining differences. Sets dontFilter in anticipation of NixOS/nixpkgs#49456.
Closes rocq-prover#8227 by solving remaining differences. Sets dontFilter in anticipation of NixOS/nixpkgs#49456.
Motivation for this change
Make it easier to override Coq packages.
Things done
sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)nix path-info -S
before and after)