Skip to content
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

Add --include-build-refs to `nix path-info' command #3523

Closed
wants to merge 22 commits into from

Conversation

matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Apr 22, 2020

This print out all of the store paths referenced in the Nix
expression.

This is useful with tools like cachix, and replace the previous usages
of:

  $ nix-store -qR --include-outputs $(nix-instantiate '<nixpkgs>' -A hello)

which now become:

  $ nix path-info --recursive --include-build-refs nixpkgs.hello

which can be piped into cachix like:

  $ nix path-info --include-build-refs nixpkgs.hello | cachix push my-cache

It provides a flag for --include-build-refs, meant to parallel dependencies needed for nix build.

Related to cachix/cachix#52
and #3506

/cc @domenkozar @edolstra

This allows use you to get all drvs that your Nix expression
references. Output looks like this:

  $ nix-instantiate --include-ifd
  warning: you did not specify '--add-root'; the result might be removed by the garbage collector
  /nix/store/xvplw4i56ys3j1lk4bxnr8r4725rb6c8-bauer-1.5.0.drv
  /nix/store/nk7j41ldlq39lmmsg5v1mlwwwpnqnbgk-README.drv
  /nix/store/pp84r1z7j7aqqarbwkq55h5asy35rs15-package-list.drv

This can be piped put into nix-store to get all outputs needed:

  $ nix-store -qR --include-outputs $(nix-instantiate --include-ifd)
  /nix/store/00crwk6z71l36av0gspzi0z410h8fq1r-libXtst-1.2.3.tar.bz2
  /nix/store/9krlzvny65gdc8s7kpb6lkx8cd02c25b-default-builder.sh
  /nix/store/58y89v7rl254dc2cygcfd5wzhv0kjm4m-bash44-013.drv
  /nix/store/7c0yirypq720qgj2clyanqp3b18h1lj0-bison-3.4.2.tar.gz.drv
  /nix/store/b7irlwi2wjlx5aj1dghx4c8k3ax6m56q-busybox.drv
  /nix/store/c0sr4qdy8halrdrh5dpm7hj05c6hyssa-unpack-bootstrap-tools.sh
  /nix/store/drsdq2ca1q1dj1hd0r1w2hl4s0fak1vh-bootstrap-tools.tar.xz.drv
  /nix/store/bfil786fxmnjcwc7mqpm0mk4xnm2cphg-bootstrap-tools.drv
  ...
This print out all of the store paths referenced in the Nix
expression. Examples provided in --help.

This is useful with tools like cachix, and replace the previous usages
of:

  $ nix-store -qR --include-outputs $(nix-instantiate '<nixpkgs>' -A hello)

which now become:

  $ nix refs --build --run nixpkgs.hello

which can be piped into cachix like:

  $ nix refs --build --run nixpkgs.hello | cachix push my-cache

Related to cachix/cachix#52
and NixOS#3506
nix-instantiate is deprecated!
@edolstra
Copy link
Member

Maybe these could be flags to nix path-info? We already have nix path-info -r to show the runtime dependencies.

@matthewbauer
Copy link
Member Author

Maybe these could be flags to nix path-info? We already have nix path-info -r to show the runtime dependencies.

That might make sense, but right now nix path-info wants store paths. To calculate, build and eval-time dependencies, we need an expression.

For build-time dependencies, we actually could use the "deriver" field to get that derivation, but it's frequently missing. Also eval-time will always need an expression (unless we add something like a "deriver-expression" field)

@matthewbauer
Copy link
Member Author

Perhaps we could rename it to nix expr-info?

@gleber
Copy link
Contributor

gleber commented Apr 22, 2020 via email

@edolstra
Copy link
Member

nix path-info (like most nix commands) works on a lot more than store paths:

$ nix path-info nixpkgs#hello
/nix/store/y26qxcq1gg2hrqpxdc58b2fghv2bhxjg-hello-2.10

$ nix path-info -f ~/Dev/nixpkgs hello
/nix/store/7hz2ms6mxdbfwj8bl4fcd171lly4l9wx-hello-2.10

$ nix path-info --impure --expr '(import <nixpkgs> {}).hello'
/nix/store/rr3y0c6zyk7kjjl8y19s4lsrhn4aiq1z-hello-2.10

Both require --recursive. --build calculates all buildtime
dependencies of a path. --eval calculates all evaltime dependencies of
an expression.
@matthewbauer matthewbauer changed the title Add `nix refs' command Add --build and --eval to `nix path-info' command Apr 23, 2020
@matthewbauer
Copy link
Member Author

Updated to use nix path-info, instead of adding a new command.

src/nix/command.cc Outdated Show resolved Hide resolved
src/nix/command.cc Outdated Show resolved Hide resolved
src/nix/command.cc Outdated Show resolved Hide resolved
src/nix/command.cc Outdated Show resolved Hide resolved
Now used in nix/command.cc error
@domenkozar domenkozar added improvement new-cli Relating to the "nix" command labels Apr 30, 2020
@matthewbauer
Copy link
Member Author

@edolstra Let me know if any concerns remain. It should be ready for re-review.

If we still have the .drv, it will be available in our database under
deriver. We can use that instead of requiring we actually received a
drvPath.
@matthewbauer
Copy link
Member Author

Updated to support --build with plain output paths. Also refactored data types a bit.

src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/nix/command.cc Outdated Show resolved Hide resolved
@Ericson2314 Ericson2314 mentioned this pull request Jun 11, 2020
Co-authored-by: John Ericson <git@JohnEricson.me>
@Ericson2314
Copy link
Member

Hmm i actually think --eval alone should get all imported paths, so e.g. import (builtins.fetchurl ...) doesn't slip under the radar. The combination of --eval and --build can turn those paths into derivations where possible.

Even non-derivations are important to have in this output, so keep
them in our list.
This is more accurate since things like fileExists and readFile also
go through this.
Runtime deps aren’t always useful, especially in the case of creating
gc roots for eval deps. for instance:

  $ nix-store --add-result --indirect -r $(nix path-info --recursive --no-run --eval my-expression)
@@ -54,6 +54,7 @@ void EvalState::realiseContext(const PathSet & context)
auto ctx = store->parseStorePath(decoded.first);
if (!store->isValidPath(ctx))
throw InvalidPathError(store->printStorePath(ctx));
importedPaths.push_back(ctx.clone());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would want to push the path to the specific output rather than drv file in this case? for --build --eval would we get the original drv file again.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be useful to have more information, but this is all that's needed for the computeFSClosure. The original drv needs to be included to get dependencies for --build. It may end up getting in multiple times, but I think that's okay.

From counting the number included, I am fairly sure it is the right thing. For instance, here's the deps I get for each:

recursive evaltime buildtime runtime total
Y N N N 0
Y Y N N 218
Y N N Y 293
Y Y N N 511
Y N Y X 1003
Y Y Y X 1008

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What standard commands or flags would enumerate all of these, but it can be unclear how to get a report/listing of each row. How was this generated?

src/libexpr/primops.cc Outdated Show resolved Hide resolved
@matthewbauer
Copy link
Member Author

matthewbauer commented Jun 12, 2020

Does anyone have objections to the naming of the options here? --eval, --build, and --no-run make sense to me but I realized it is a little confusing because you're not actually doing those things - just recursing into them. Perhaps --recursive-eval, --recursive-build are better... I also thought about something like --closure-type=runtime/build/eval.

@edolstra
Copy link
Member

I'm hesitant about adding these flags:

  • Build dependencies expose the concept of store derivations, which the nix command tries to move away from (so that in the future, we could stop materializing them to disk, and instead just send an in-memory graph of build actions to the daemon).

  • Evaluation-time dependencies are unpredictable: if evaluation caching is enabled (the default), then nix path-info could output different results when you run it a second time.

@Ericson2314
Copy link
Member

  1. Sorry I must be missing something. Build-time dependencies still exists as a concept, whether or not we have the drv files. Is the issue using drv file paths to refer to derivations? Or just that the daemon might not remember how something was built?

  2. Could we put this info in the evaluation cache so it can be retrieved later? At the end of the day, we (and I assume others) will be happy with any approve that allows one to be sure they are saving enough dependencies to make importing derivations reproducible----this is a feature we really want. If there is a different approach you would prefer, we'd be happy to try that instead.

This could have some bad interactions with the eval cache. Once flakes
is merged, we can resolve those so that --eval will always reevaluate
the expression.
@matthewbauer matthewbauer changed the title Add --build and --eval to `nix path-info' command Add --build to `nix path-info' command Jun 26, 2020
@matthewbauer
Copy link
Member Author

Updated to just provide --build since --eval could have some bad interactions with the eval cache.

@domenkozar
Copy link
Member

@edolstra there's no way to do this currently with flakes, which really limits ways to inspect closures for build dependencies.

If we stop materializing to disk, we can still compute the build closure it just can't be cached?

@hyperfekt
Copy link

Can --eval not be made to run without the evaluation cache? I'm thinking it's still going to be very useful.

@matthewbauer matthewbauer changed the title Add --build to `nix path-info' command Add --include-build-refs to `nix path-info' command Jan 29, 2021
@matthewbauer
Copy link
Member Author

@domenkozar @edolstra Any chance this can get some more review? I've updated on to master and now renamed it to --include-build-refs. I'm hoping the longer, more specific name means it's a little less objectionable and more obvious how it fits into the --recursive flag flow.

matthewbauer added a commit to matthewbauer/nix that referenced this pull request Jan 30, 2021
Continuation of NixOS#3523, but with
evaluation-time refs this time. More discussion is available at that
PR.

This keeps track of imported paths when evaluating. When --recursive &
--include-eval-refs is passed, these paths & their references are
included in the resulting closure.
@domenkozar
Copy link
Member

@matthewbauer does it address concerns raised by @edolstra? I'm +1 for merging it otherwise.

matthewbauer added a commit to matthewbauer/nix that referenced this pull request Apr 21, 2021
This primop lets you get information on paths realised (pathExists,
import, readFile) by Nix. It tracks paths realised in Nix & puts the
results under the "paths" attr. For instance:

$ nix eval --impure --expr '(builtins.traceContext (import ./.).outPath)'
{ paths = [ "/nix/store/2mpgi4bvn8py4liv9w3mjxd2c5r7bvv8-source" "/nix/store/3k7i1rdqcgyzjxvqm37hapdidy4ls4s3-source" "/nix/store/kqxic0j6wpsaw2bb51hr1yc1nb1z2xw8-source" ]; value = "/nix/store/3k7i1rdqcgyzjxvqm37hapdidy4ls4s3-source"; }

This is an alternative to the --include-eval-refs from
NixOS#3523 & --include-ifd from
NixOS#3506.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants