Skip to content

Add builtins.debug#4914

Merged
thufschmitt merged 2 commits intoNixOS:masterfrom
gytis-ivaskevicius:master
Jul 6, 2022
Merged

Add builtins.debug#4914
thufschmitt merged 2 commits intoNixOS:masterfrom
gytis-ivaskevicius:master

Conversation

@gytis-ivaskevicius
Copy link
Contributor

When working on tests I thought that it would be useful to have some sort of logger which would be enabled by increasing verbosity level. This PR adds builtins.debug which evaluates the first argument when -vv is specified.

Exact use case:
I and @blaggacao started working on these check utils https://github.com/numtide/flake-utils/blob/master/check-utils.nix and plan is to add this kind of logging to simplify the debugging process for the end-users

PS: Please be gentle, I don't do CPP :D

Copy link
Contributor

@blaggacao blaggacao left a comment

Choose a reason for hiding this comment

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

LGTM (no CPP)

@gytis-ivaskevicius
Copy link
Contributor Author

I'd appreciate a review.
\CC: @domenkozar @edolstra @regnat

@edolstra
Copy link
Member

LGTM. Only problem is that the name debug is a bit generic...

Could you add a test?

@gytis-ivaskevicius
Copy link
Contributor Author

Does anyone have any naming suggestions?

@HaoZeke
Copy link
Member

HaoZeke commented Aug 25, 2021

pesticide --> bugnixer ?

exprval spewlvl

@L-as
Copy link
Member

L-as commented Aug 25, 2021

builtins.traceVerbose?

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

I think @L-as's suggestion of builtins.traceVerbose is great. In addition to the below suggestions, I'm not entirely sure whether using -v for this is a good idea, though I don't have any good reasons against it.

Here's a branch where I implemented the below two suggestions (and also having a separate flag instead of -v, but not sure if we want that): infinisil@3cfde43

@balsoft
Copy link
Member

balsoft commented Dec 2, 2021

I like the idea, as long as you can't "lift" the verbosity level to the Nix language itself.

@gytis-ivaskevicius
Copy link
Contributor Author

Finally got around to this. I think its ready for review

@infinisil
Copy link
Member

Yeah, I think it's fine. Does not really matter.

I think this does matter, why shouldn't it? This isn't an implementation detail, this is the user interface. It's the difference of requiring the user to learn an extra flag, or being able to reuse -v. On one hand, I don't see any problems to reuse -v, but on the other hand it seems cleaner to have this separate --trace-verbose flag, especially because the -v flag currently doesn't involve evaluation at all.

So I guess I'm leaning towards having the new --trace-verbose, but I'd like someone else to think about this.

@gytis-ivaskevicius
Copy link
Contributor Author

Alright, I would prefer -v. Definitely not a deal-breaker for me. Does anyone else have any thoughts on this?

@roberth
Copy link
Member

roberth commented Jan 10, 2022

Regarding -v/--trace-verbose: I think this can be decomposed into a more general problem.
In any sufficiently complex system, it's nice to be able to set verbosity at a component level, similar to how verbose kernel logging doesn't add extra data to my web server access logs. It seems like a perfectly sensible request to only log verbosely in certain components, such as one of:

  • Nix expression trace calls
  • evaluator
  • store operations
  • substitution
  • sandbox
  • NIX_DEBUG in derivations
  • user code (evaluation + NIX_DEBUG)
  • Nix expression trace calls, restricted to certain flakes

If this feature was implemented, it'd subsume --trace-verbose and probably provide it as a more systematic flag.
So let's not get ahead of ourselves and pick -v for now.

@roberth
Copy link
Member

roberth commented Jan 10, 2022

This also reminds me of NixOS/nixpkgs#140763, which makes lib.warn abort. It'd be great to have this behavior as a builtins.warn, which then also works in pure mode.

(A --show-trace-on-warn would also be great but is just not feasible without having traces as first class objects. Stackless evaluator? Would be cool but not a priority.)

@infinisil
Copy link
Member

@roberth --show-trace-on-warn could be feasible if there was a builtins.warn

@roberth
Copy link
Member

roberth commented Feb 14, 2022

@roberth --show-trace-on-warn could be feasible if there was a builtins.warn

Do we have first-class traces yet? Afaik, they're hidden behind the implicit C++ exception stack. Otherwise, realistically, we can only do --error-on-warn.

@infinisil
Copy link
Member

This is related to #749, ping @Profpatsch

@Profpatsch
Copy link
Member

I don’t see how it’s related to displaying warnings.

Warnings should show even if the user does not use -v, cause otherwise they will miss them during normal use of nix.

@infinisil
Copy link
Member

I think it's related because of different verbosities of messages:

@Profpatsch
Copy link
Member

At this point I have lost hope that nix will ever have a sensible way of displaying warnings tbh. The hydra and nix-env misdesign won’t be changed according to upstream, so there’s just no real way to make it work.

@edolstra
Copy link
Member

edolstra commented Mar 3, 2022

@Profpatsch What misdesign are you referring to?

@thufschmitt
Copy link
Member

@edolstra @gytis-ivaskevicius : apart from the (small) merge-conflict, anything preventing this from being merged?

@thufschmitt
Copy link
Member

@gytis-ivaskevicius I’ve merged the branch with master, but now the build is failing. Can you have a look?

@thufschmitt
Copy link
Member

We discussed that a bit during the last UX meeting. There's a broad consensus on this being great, just needs to fix the build.

@gytis-ivaskevicius think you can take a bit of time to do that?

@gytis-ivaskevicius
Copy link
Contributor Author

Alright, will do! (Expect a push tomorrow/day after tomorrow)

Co-Authored-By: Silvan Mosberger <contact@infinisil.com>

Add builtins.traceVerbose tests
@gytis-ivaskevicius
Copy link
Contributor Author

Sorry, a little late. Took me 2 attempts to resolve the issue 😅

Copy link
Member

@thufschmitt thufschmitt left a comment

Choose a reason for hiding this comment

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

Thanks, let's roll this!

@thufschmitt
Copy link
Member

Thanks, let's roll this!

Once the CI agrees, that is

@thufschmitt thufschmitt merged commit b0e18df into NixOS:master Jul 6, 2022
@thufschmitt
Copy link
Member

Duh, forgot about the release notes. I'll add them in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.