stdenv: Validate meta.outputsToInstall #46834

grahamc · 2018-09-18T13:46:42Z

If meta.outputsToInstall is set to include absent outputs, various
tools break including channel updates and nix-env.

grahamc@Morbo> nix-env -i -f . -A elf-header-real
installing 'elf-header'
error: this derivation has bad 'meta.outputsToInstall'

This patch adds a mandatory verification phase to the stdenv's meta
check to ensure the list is correct:

grahamc@Morbo> nix-build . -A elf-header-real
error: Package ‘elf-header’ in /home/grahamc/projects/nixpkgs/pkgs/development/libraries/elf-header/default.nix:36 has invalid meta.outputsToInstall, refusing to evaluate.

The package elf-header has set meta.outputsToInstall to: bin

however elf-header only has the outputs: out

and is missing the following ouputs:

  - bin

(use '--show-trace' to show detailed location information)

Note, now the nix-env experience is decidedly worse:

grahamc@Morbo> nix-env -i -f . -A elf-header-real; echo $?
0

though since this is already an issue for unfree, broken, unsupported,
and insecure validity problems I'm not sure we should do something
different here.

One possible question is should this be moved to the checkMeta phase, and not part of every evaluation.

Motivation for this change

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- other Linux distributions
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

srhb · 2018-09-18T13:52:24Z

pkgs/stdenv/generic/check-meta.nix

@@ -125,6 +126,20 @@ let

      '';

+  remediate_outputs_to_unknown = attrs: let
+      expected_outputs = (attrs.meta or {}).outputsToInstall or [];


Suggest attrs.meta.outputsToInstall or [] instead. It will only fail if attrs isn't an attrset or undefined (I think?), and the existing version will as well.

nix-repl> {}.bar.baz.tux or [] [ ]

whoa!

Please use camelCase, i.e. remediateOutputsToUnknown, expectedOutputs, and brokenOutputs`.

GrahamcOfBorg · 2018-09-18T14:01:17Z

Success on x86_64-darwin (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/95bw3cdcgc8c0rrvqwvhvc805pdv7rmx-stdenv-darwin

GrahamcOfBorg · 2018-09-18T14:01:22Z

Success on aarch64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/pjq5v1xgljr4ygb1h1xfybdjydczcqiw-stdenv-linux

GrahamcOfBorg · 2018-09-18T14:01:25Z

Success on x86_64-linux (full log)

Attempted: stdenv

Partial log (click to expand)

/nix/store/i6vl5lwlz5jbkg4r6p340dwmj6fha3xq-stdenv-linux

edolstra · 2018-09-18T14:01:36Z

What is the impact on evaluation performance?

GrahamcOfBorg · 2018-09-18T14:02:25Z

Success on x86_64-linux (full log)

Attempted: elf-header-real, stdenv

Partial log (click to expand)

these paths will be fetched (0.03 MiB download, 0.16 MiB unpacked):
  /nix/store/d90nn8g9711j6xfb89a7wl0yc66l7gzj-elf-header
copying path '/nix/store/d90nn8g9711j6xfb89a7wl0yc66l7gzj-elf-header' from 'https://cache.nixos.org'...
/nix/store/d90nn8g9711j6xfb89a7wl0yc66l7gzj-elf-header
/nix/store/i6vl5lwlz5jbkg4r6p340dwmj6fha3xq-stdenv-linux

GrahamcOfBorg · 2018-09-18T14:02:45Z

Success on aarch64-linux (full log)

Attempted: elf-header-real, stdenv

Partial log (click to expand)

these paths will be fetched (0.03 MiB download, 0.16 MiB unpacked):
  /nix/store/9wx3gfqmqcr2w5s9gnp79zfjbvca8zi1-elf-header
copying path '/nix/store/9wx3gfqmqcr2w5s9gnp79zfjbvca8zi1-elf-header' from 'https://cache.nixos.org'...
/nix/store/9wx3gfqmqcr2w5s9gnp79zfjbvca8zi1-elf-header
/nix/store/pjq5v1xgljr4ygb1h1xfybdjydczcqiw-stdenv-linux

GrahamcOfBorg · 2018-09-18T14:02:48Z

Success on x86_64-darwin (full log)

Attempted: elf-header-real, stdenv

Partial log (click to expand)

these paths will be fetched (0.03 MiB download, 0.16 MiB unpacked):
  /nix/store/0psyvq9qnn2v0j2kv9zk6qkknpyf7wc0-elf-header
copying path '/nix/store/0psyvq9qnn2v0j2kv9zk6qkknpyf7wc0-elf-header' from 'https://cache.nixos.org'...
/nix/store/0psyvq9qnn2v0j2kv9zk6qkknpyf7wc0-elf-header
/nix/store/95bw3cdcgc8c0rrvqwvhvc805pdv7rmx-stdenv-darwin

grahamc · 2018-09-18T14:12:20Z

on master:

[1] grahamc@Morbo> time  nix-env -qa --json --file .  > /dev/null
nix-env -qa --json --file . > /dev/null  6.08s user 0.37s system 100% cpu 6.447 total

on this branch:

[141] grahamc@Morbo> time  nix-env -qa --json --file .  > /dev/null
nix-env -qa --json --file . > /dev/null  6.36s user 0.41s system 100% cpu 6.761 total

what do you think @edolstra?

grahamc · 2018-09-18T14:14:27Z

I think at +.3s it should be only run if checkMeta is set to true:

-    in builtins.length missingOutputs > 0;
+    in if shouldCheckMeta
+       then builtins.length missingOutputs > 0
+       else false;

but wouldn't mind leaving it on always if you think that is better.

If meta.outputsToInstall is set to include absent outputs, various tools break including channel updates and nix-env. grahamc@Morbo> nix-env -i -f . -A elf-header-real installing 'elf-header' error: this derivation has bad 'meta.outputsToInstall' This patch verifies each value in meta.outputsToInstall is a valid output. It validates this condition only if checkMeta is true. grahamc@Morbo> nix-build . -A elf-header-real error: Package ‘elf-header’ in /home/grahamc/projects/nixpkgs/pkgs/development/libraries/elf-header/default.nix:36 has invalid meta.outputsToInstall, refusing to evaluate. The package elf-header has set meta.outputsToInstall to: bin however elf-header only has the outputs: out and is missing the following ouputs: - bin (use '--show-trace' to show detailed location information) Note, now the nix-env experience is decidedly worse for users who have checkMeta set to true: grahamc@Morbo> nix-env -i -f . -A elf-header-real; echo $? 0 though since this is already an issue for unfree, broken, unsupported, and insecure validity problems I'm not sure we should do something different here.

grahamc · 2018-09-18T14:59:32Z

this branch:

grahamc@Morbo> for i in `seq 1 10`; do time  nix-env -qa --json --file .  > /dev/null; done
nix-env -qa --json --file . > /dev/null  6.14s user 0.38s system 100% cpu 6.510 total
nix-env -qa --json --file . > /dev/null  6.09s user 0.38s system 100% cpu 6.465 total
nix-env -qa --json --file . > /dev/null  6.08s user 0.36s system 100% cpu 6.446 total
nix-env -qa --json --file . > /dev/null  6.10s user 0.40s system 100% cpu 6.501 total
nix-env -qa --json --file . > /dev/null  6.17s user 0.41s system 100% cpu 6.579 total
nix-env -qa --json --file . > /dev/null  6.03s user 0.43s system 100% cpu 6.454 total
nix-env -qa --json --file . > /dev/null  6.05s user 0.39s system 100% cpu 6.426 total
nix-env -qa --json --file . > /dev/null  6.06s user 0.39s system 100% cpu 6.445 total
nix-env -qa --json --file . > /dev/null  6.12s user 0.36s system 100% cpu 6.471 total
nix-env -qa --json --file . > /dev/null  6.15s user 0.39s system 100% cpu 6.538 total

master:

grahamc@Morbo> for i in `seq 1 10`; do time  nix-env -qa --json --file .  > /dev/null; done
nix-env -qa --json --file . > /dev/null  7.16s user 0.46s system 99% cpu 7.618 total
nix-env -qa --json --file . > /dev/null  6.42s user 0.38s system 100% cpu 6.795 total
nix-env -qa --json --file . > /dev/null  6.14s user 0.37s system 100% cpu 6.507 total
nix-env -qa --json --file . > /dev/null  6.06s user 0.39s system 100% cpu 6.442 total
nix-env -qa --json --file . > /dev/null  6.11s user 0.38s system 100% cpu 6.489 total
nix-env -qa --json --file . > /dev/null  6.08s user 0.36s system 100% cpu 6.442 total
nix-env -qa --json --file . > /dev/null  6.12s user 0.39s system 100% cpu 6.508 total
nix-env -qa --json --file . > /dev/null  6.03s user 0.38s system 100% cpu 6.403 total
nix-env -qa --json --file . > /dev/null  6.07s user 0.39s system 100% cpu 6.456 total
nix-env -qa --json --file . > /dev/null  6.07s user 0.37s system 100% cpu 6.432 total

grahamc · 2018-09-18T15:10:04Z

Backport: #46838

Ericson2314 · 2018-09-18T17:19:50Z

Thanks!!

grahamc requested review from edolstra and vcunat September 18, 2018 13:46

grahamc requested a review from Ericson2314 as a code owner September 18, 2018 13:46

GrahamcOfBorg added the 6.topic: stdenv Standard environment label Sep 18, 2018

srhb reviewed Sep 18, 2018

View reviewed changes

GrahamcOfBorg added 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Sep 18, 2018

grahamc changed the title ~~stdenv: Validate meta.outputsToInstall~~ WIP: stdenv: Validate meta.outputsToInstall Sep 18, 2018

grahamc force-pushed the check-outputs-in-meta branch from 69aeaf5 to b80c9ce Compare September 18, 2018 14:40

elf-header-real: fixup outputsToInstall

301109a

grahamc changed the title ~~WIP: stdenv: Validate meta.outputsToInstall~~ stdenv: Validate meta.outputsToInstall Sep 18, 2018

grahamc merged commit 11c76c9 into NixOS:master Sep 18, 2018

grahamc deleted the check-outputs-in-meta branch September 18, 2018 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: Validate meta.outputsToInstall #46834

stdenv: Validate meta.outputsToInstall #46834

grahamc commented Sep 18, 2018 •

edited

srhb Sep 18, 2018

grahamc Sep 18, 2018

edolstra Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

edolstra commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

grahamc commented Sep 18, 2018

grahamc commented Sep 18, 2018

grahamc commented Sep 18, 2018 •

edited

grahamc commented Sep 18, 2018

Ericson2314 commented Sep 18, 2018

stdenv: Validate meta.outputsToInstall #46834

stdenv: Validate meta.outputsToInstall #46834

Conversation

grahamc commented Sep 18, 2018 • edited

Motivation for this change

Things done

srhb Sep 18, 2018

Choose a reason for hiding this comment

grahamc Sep 18, 2018

Choose a reason for hiding this comment

edolstra Sep 18, 2018

Choose a reason for hiding this comment

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

edolstra commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

GrahamcOfBorg commented Sep 18, 2018

grahamc commented Sep 18, 2018

grahamc commented Sep 18, 2018

grahamc commented Sep 18, 2018 • edited

grahamc commented Sep 18, 2018

Ericson2314 commented Sep 18, 2018

grahamc commented Sep 18, 2018 •

edited

grahamc commented Sep 18, 2018 •

edited