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
nix-env.cc: make --query
evaluate .outPath
s
#1771
Conversation
2e634df
to
4bcd132
Compare
Well, that's possible, but due to the recursive nature, my previous tests suggested that forcing |
Except perhaps that |
4bcd132
to
f18f523
Compare
Okay, so I hacked it a little bit more and simplified the first patch considerably.
Well, that's possible, but due to the recursive nature, my previous tests suggested that forcing `.out` leads to a significant slowdown of `nix-env -qa` (roughly five-times in my year-old test). On the other hand, it improves semantics – even closure would get checked during `-qa`.
On my tests the first patch gives 7x slowdown (unless `--fast` is given).
However, now this patchset has the second patch, with it and the new patch in `nixpkgs` PR the slowdown is less than 2x.
I have to note, that 7x slowdown produced a nice effect: I found a couple of new bugs in expressions in `nixpkgs` on `-darwin` from `-linux` (because I could evaluate their `.outPath`s even though I couldn't build them) so I'm now seriously considering adding an option to skip setting `meta.skipme` in the other PR to get that 7x slowdown back.
Except perhaps that `queryOutPath` seems overkill, as it queries the binary cache if I understand it correctly.
I`m looking at `src/libexpr/get-drvs.cc:78` and see nothing that seems to be related to binary caches.
It seems that `.outPath` is just simply too expensive to compute.
|
f18f523
to
04dbea9
Compare
04dbea9
to
85cdf2e
Compare
After some more hacking it's now gives 4% slowdown on my machine with 99% of the end result (a small number of broken packages get listed).
Pedantic result does require 7x slowdown.
|
85cdf2e
to
dec8353
Compare
I think this and the sibling in `nixpkgs` are perfect now, with both
patches applied its now indistinguishable by performance (with default
options) and is much better w.r.t. semantics.
Will add more documentation to the manual if there's a consensus on
merging this.
|
This behaviour ( |
This behaviour (`-qa` not evaluating `outPath`) is intentional, see edd145d.
I don't understand. Is there any other motivation except speed there?
If not, then merging this and the sibling PR (NixOS/nixpkgs#33057) for `nixpkgs` makes things strictly not worse, and merging both and then setting `config.checkMetaRecursively` makes things much better (at the cost of 2x slowdown).
NixOS/nixpkgs#33057 can't be merged without this because it will make `nix-env -qa` list broken packages and you didn't want to have that in #22277.
Not merging NixOS/nixpkgs#33057 breaks NixOS/nixpkgs#32424 (see NixOS/nixpkgs#33006) and many people want to have that feature since NixOS/nixpkgs#9306.
|
Yes, the motivation is speed. |
This is a temporary workaround to make `nix-env -qa` and `nix search` ignore broken packages as they they did before this patchset. This patch should be reverted after `nix` gets a proper fix for this. See NixOS/nix#1771.
dec8353
to
e7f25a7
Compare
NixOS/nixpkgs#33057 delays `stdenv.mkDerivation` derivation checks (e.g. meta, brokennes, etc) to `.drvPath` and `.outPath` evaluation leaving other fields accessible. That has a side-effect of making `nix-env -qa` list broken packages. This patch allows to - evaluate all `.outPath`s in `-qa` output strictly (`--strict` option) at the cost of 5x-7x slowdown; - disable filtering completely with `--all` option at the cost of getting some junk in the output and some speedup; - filter based on `.meta.available` (the default, `--approximate` option) at the cost of getting a tiny amounts of junk in the output and 0.96x-1.04x slowdown. It is also possible to get near-perfect results with `--approximate` at the cost of 2x slowdown by setting `config.checkMetaRecursively` in nixpkgs.
`-i` has the same problem as `-qa` in previous patch.
e7f25a7
to
b6c847e
Compare
Updated and rebased onto master. Tested to work with 2.0.4 too. |
I tested this to fix both #1963 (with a new one-line change) and NixOS/nixpkgs#37350 as well my original issue. Other Now my conscience is clear. |
@edolstra (triage) If I read correctly, currently this PR is a 4% slowdown in exchange for fixing #1963 and NixOS/nixpkgs#37350. If so, this sounds like a net gain to me? @oxij Can you confirm this version is the 4% slowdown you mentioned in #1771 (comment) and not the 7x slowdown indeed? |
Yes, last time I checked it was 4% slowdown for `nix-env -qa`. This also adds some small undetermined amount of slowdown to `nix-env -i` in cases where it should fail, but the latter slowdown can be completely ignored, IMHO.
Those 4% come from evaluating `.available`. But `stdenv` also adds `assert`s to `.name` evaluation to work around the initial issue which doesn't work perfectly and breaks things like `.pos` handling. Moreover, I think that by applying this and then removing the `stdenv` `.name` hack we can get most, if not all, of those 4% back (not immediately, as `nixpkgs` will need to support both ways for a while, but eventually).
Since `.available` is now a fairly well-accepted part of nixpkgs I don't see any reason in not applying this.
SLNOS has this applied for like a year already.
|
The first patch here is no longer needed with |
NixOS/nixpkgs#33057 delays
stdenv.mkDerivation
derivation checks (e.g.meta, brokennes, etc) to
.drvPath
and.outPath
evaluation leavingother fields accessible. That has a side-effect of making
nix-env -qa
list broken packages.
This patch forces those checks.