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

Log all IFD #8094

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Log all IFD #8094

wants to merge 2 commits into from

Conversation

cidkidnix
Copy link
Contributor

@cidkidnix cidkidnix commented Mar 22, 2023

Motivation

For external tools that want to know about all import from derivation.

Context

Continuation of #3494
Currently doesn't work completely due to ctx changes

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

  • agreed on idea
  • agreed on implementation strategy
  • tests, as appropriate
    • functional tests - tests/**.sh
    • unit tests - src/*/tests
    • integration tests - tests/nixos/*
  • documentation in the manual
  • code and comments are self-explanatory
  • commit message explains why the change was made
  • new feature or incompatible change: updated release notes

src/libexpr/primops.cc Outdated Show resolved Hide resolved
src/libexpr/primops.cc Outdated Show resolved Hide resolved
@@ -642,6 +642,9 @@ struct EvalSettings : Config

Setting<bool> traceVerbose{this, false, "trace-verbose",
"Whether `builtins.traceVerbose` should trace its first argument when evaluated."};

Setting<bool> logAllIFD{this, false, "log-all-ifd",
"Emit log messages for all imports from derivation at the 'info' log level"};
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Emit log messages for all imports from derivation at the 'info' log level"};
"Emit log messages for all imports from derivation at the 'info' log level."};

@Ericson2314 Ericson2314 mentioned this pull request May 13, 2023
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label May 13, 2023
Co-authored-by: Dylan Green <[email protected]>
@cidkidnix cidkidnix marked this pull request as ready for review May 18, 2023 12:56
@cidkidnix
Copy link
Contributor Author

Functionally it works now, they'll need to be some cleanup of logging messages. Either way it should be ready to go now

{
Logger::Field{store->printStorePath(drv.drvPath)},
Logger::Field{drv.output},
Logger::Field{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we want this split out or do we want to construct the Field in a way where it's the same as positions[pos]?

Copy link
Member

Choose a reason for hiding this comment

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

I got keep it this way, so maximum structure for the an external application. And pull out the AbstactPos -> Fields conversion to a separate function.

Comment on lines +4 to +5
# TODO: Now what
nix-build ./import-derivation.nix --option "log-import-from-derivation" true --no-out-link --log-format internal-json 2>&1 1>/dev/null | grep "^@nix" | sed 's/^@nix //g' | jq "select(.type == 112)"
Copy link
Member

Choose a reason for hiding this comment

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

Need to somehow check the resulting JSON here. Not sure now.

@@ -1607,7 +1607,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
if (countCalls) primOpCalls[name]++;

try {
vCur.primOp->fun(*this, noPos, args, vCur);
vCur.primOp->fun(*this, pos, args, vCur);
Copy link
Member

Choose a reason for hiding this comment

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

@layus I am curious on your feedback about these noPos -> pos changes, I think some of the noPos dates to your big overhaul.

Copy link
Member

Choose a reason for hiding this comment

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

This indeed defeats my intent, as I wanted to have a given pos printed exactly once, and attached to the most relevant trace item. Before we used to reuse the same pos all over the place, so that it ended up being displayed multiple times in the error trace, and associated with trace items that were irrelevant.

In this particular case, the pos we have is the one of the AST node calling a function. That is why I pass noPos to primOp->fun (because that is irrelevant for errors inside said primop) and wrap any error thrown by the primop with an error trace (see right below this message, addErrorTrace(e, pos, "while calling the '%1%' builtin", name);)

I understand the need to access this value when logging (you have no context to understand where the error comes from), but that value should not be used in error traces. I am fairly confident this change will only have a small impact, if any, as most builtins should already have been rewritten to not use their pos argument in most cases. And in any case it is only redundant printing of positions in error traces.

Be wary that the position might not be very accurate or useful. It may not even contain the builtin name that is referenced, as in:

let
  doSomething = import;
  aStringFromSomewhere = "${some derivation generating a nix file}/nix-file.nix";
in
  doSomething aStringFromSomewhere # <- pos is HERE, no import, no context, nothing directly useful.

Copy link
Member

Choose a reason for hiding this comment

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

What gets broken is the implicit invariant that when you use a pos, you then pass noPos to your children so that they know the pos has already been used in a more relevant context, and you only have to print your message.
With this change a pos that has already been used gets passed around, and might get printed some more times. Not ideal, but not a big deal, especially since I have little time to play with this pr and investigate.

Copy link
Member

Choose a reason for hiding this comment

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

From my point of view, the position is not that useful, and thus does not need to be logged at all. But I also understand that a position, even an approximate one, can be better than no position at all.

What is the point of printing positions here BTW ? Because external tools will not be able to handle it correctly in all cases. So maybe not printing the position is an option after all 🤔

Will stop the fuss and get some sleep :-)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I wouldn't want to undo your hard work making the trace sane. Fundamentally, the "nicest" logging would have the entire trace, but the trace is only built when the exceptions / stack is unwound. There is no passing in the context pre-made as an argument to the primop/realize context for constructing the Activity with the current architecture.

Perhaps we shouldn't log positions at all end, at least for the first version.

@@ -1655,7 +1655,7 @@ void EvalState::callFunction(Value & fun, size_t nrArgs, Value * * args, Value &
// 1. Unify this and above code. Heavily redundant.
// 2. Create a fake env (arg1, arg2, etc.) and a fake expr (arg1: arg2: etc: builtins.name arg1 arg2 etc)
// so the debugger allows to inspect the wrong parameters passed to the builtin.
primOp->primOp->fun(*this, noPos, vArgs, vCur);
primOp->primOp->fun(*this, pos, vArgs, vCur);
Copy link
Member

Choose a reason for hiding this comment

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

More noPos to pos.

{
NixStringContext context;

auto path = state.coerceToPath(noPos, v, context, "while realising the context of a path");
auto path = state.coerceToPath(pos, v, context, "while realising the context of a path");
Copy link
Member

Choose a reason for hiding this comment

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

More noPos to pos.

Copy link
Member

@Ericson2314 Ericson2314 left a comment

Choose a reason for hiding this comment

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

I have long wanted to get rid of the "import from derivation" name, and this is a a good time to start doing it.

The phrase is barely better than "Holy Roman Empire". I concede that "from" is correct, but the other two words are wrong:

  1. It is not only "import"ing, every reason other than "import" in the patch is something else that is just as valid.
  2. It is not from "derivation" from from a derivation output. You can, in recentish Nix, import a drv file directily; it works and does not require realizeContext.

I would just call it something like:

  • building during evaluation
  • building for evaluation
  • realising during evaluation
  • realising for evaluation
  • eval from built path
  • eval from derivation output
  • interleaved building and evaluation

More suggestions welcome. It is not hard to be better than "import from derivation".

@layus
Copy link
Member

layus commented May 19, 2023

From a build systems point of view, this is a case of dynamic dependencies. A case where you have to actually build some of your build graph to access the produced content, and use it to further build your build graph.

So either that, or something along the lines of dynamic realizations or dynamic evaluation seems precise enough, but maybe not easy to force upon the community, and less easy to grasp.

I kind of like IFD though, as "derivation" is also commonly used to speak about realizations, and "import" is by far the simplest way to make it happen. So "import from derivation" happens when you import from the content of a derivation. The name gives a quick idea of why this concept matters and what it encompasses, if not in its details. Plus it is kind of the norm now. Like with intentional store being the nix-world keyword for a content addressed store, which is technically an extensional store 🙃 . I cannot help but get fond of this community.

@Ericson2314
Copy link
Member

Well "CA derivations" has thankfully replaced "intentional store".

Derivation vs derivation output I am especially sensative to because with RFC 92 there is no longer an obvious disambiguation: Derivations can also be derivation outputs (of another derivation), and derivation outputs can also be derivations. There is no choice but to be precise about what one is talking about.

We thankfully already have DerivedPath now, which finally allows us to be precise about what we're talking about (fixing pre and post RFC 92 ambiguities). And the CLI now exposes it too with the ^ usable with store paths. So I want to do a big documentation push with that too.

Overall, yeah we've gotten by our bad wrong terminology and whatnot, but people also complain Nix is too hard and confusing. Maybe we should try using self-consistent terminology without gaps of unexpressable stuff, and see how people react? :D

@layus
Copy link
Member

layus commented May 20, 2023 via email

@Ericson2314
Copy link
Member

I think "dynamic dependencies" is too vague and could equally mean this or 92, but "dynamic evaluation" could work.

@cidkidnix
Copy link
Contributor Author

I have no problems with not passing pos around to log it since it's not always 100% accurate.

I still find knowing roughly what file that the IFD is coming from is in, but that can be implemented without passing pos.

For the first version of all of this logging it might be best just to drop the pos changes completely

I also have no preferences with wording regarding the log message @Ericson2314

@fricklerhandwerk fricklerhandwerk added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Jun 5, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-06-05-nix-team-meeting-minutes-60/28933/1

@andreabedini
Copy link
Contributor

If I may, since there's already --allow-import-from-derivation let's keep --log-import-from-derivation in this PR so this doesn't get stuck. Then we can make another PR that renames both.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. with-tests Issues related to testing. PRs with tests have some priority
Projects
Status: 🏁 Review
Development

Successfully merging this pull request may close these issues.

8 participants