Add `builtins.warn` by roberth · Pull Request #10592 · NixOS/nix

roberth · 2024-04-22T23:32:13Z

Motivation

A variation of trace, but

with the correct log level
no ANSI codes in expressions
can be debugged without stopping for actual trace calls (ie intentional "info level" or trace logging).
logs warnings without trace: in front of it
follows the Nix color scheme.

Context

I'd planned to add this a long time ago but forgot to eventually follow up on it.
This Nixpkgs PR reminded me to complete it, and I would like to avoid an unnecessary deprecation.
It proposes to rename the environment variable, which would make the current lib.warn not an (anachronistic) polyfill of the Nix feature anymore. That'd be unfortunate.

lib.trivial: rename NIX_ABORT_ON_WARN to NIXPKGS_ABORT_ON_WARN nixpkgs#306026

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

Ericson2314 · 2024-04-23T15:07:03Z

+    if (args[0]->type() == nString)
+        printMsg(lvlWarn, ANSI_WARNING "warning:" ANSI_NORMAL " %1%", args[0]->string_view());
+    else
+        printMsg(lvlWarn, ANSI_WARNING "warning:" ANSI_NORMAL " %1%", ValuePrinter(state, *args[0]));


Why do we need to redo the error formatting here, vs reused something from the loggers?

It now uses logWarning, which also sets us up to print an actual position, after #10597.

Ericson2314 · 2024-04-24T01:37:43Z

+    }
+
+    if (evalSettings.builtinsAbortOnWarn) {
+        state.error<Abort>("aborting to reveal stack trace of warning, as abort-on-warn is set").debugThrow();


Optional: we could also just throw our BaseError with this as extra context.

I prefer to treat them separately, because they don't have the same message, and I don't think it should be just a single message.

Totally fine! Though just to be clear, I was thinking the this message would become an addTrace call --- I too don't think it should be a single message.

Ericson2314 · 2024-04-24T16:06:17Z

+    }
+
+    if (evalSettings.builtinsAbortOnWarn) {
+        state.error<Abort>("aborting to reveal stack trace of warning, as abort-on-warn is set").debugThrow();


Totally fine! Though just to be clear, I was thinking the this message would become an addTrace call --- I too don't think it should be a single message.

Ericson2314 · 2024-04-24T16:06:59Z

+    if ((evalSettings.builtinsTraceDebugger || evalSettings.builtinsDebuggerOnWarn) && state.debugRepl && !state.debugTraces.empty()) {
+        const DebugTrace & last = state.debugTraces.front();
+        state.runDebugRepl(nullptr, last.env, last.expr);
+    }


A few of the call sites runDebugRepl look similar --- not just builtin.trace's even. Should we abstract over this a bit?

I think yes, by allowing any primop to be marked as a breakpoint.
Until we have that, let's just keep it simple.

Just to be clear, I mean not a new generalized feature, but something very mechanical:

if (state.debugRepl && !state.debugTraces.empty()) { const DebugTrace & last = state.debugTraces.front(); state.runDebugRepl(<some-expression>, last.env, last.expr); }

seems to be part of this EvalErrorBuilder<T>::debugThrow, primop_break, primop_trace, and now prim_warn. So I am wondering if a new function (maybe in header, taking an auto && lambda, to get the laziness right) is warranted.

lorenzleutgeb · 2024-05-02T18:47:21Z

+        msg.atPos(state.positions[pos]);
+        auto info = msg.info();
+        info.level = lvlWarn;
+        logWarning(info);


If I understand correctly, users will not be able to easily distinguish output produced by builtins.warn from output produced by other calls of logWarning. However, other callsites will not go ahead and trigger the "abort on warn" behaviour. It's a safe bet that you'll have some users set NIX_ABORT_ON_WARN, then hit one of the other codepaths, and be confused why Nix did not abort.

I don't think that it's a solution to have Nix abort on all these kinds of warnings. There are rather benign warnings too, like:

warning: Git tree '...' is dirty warning: unknown flake output 'homeModules'

The only proposal I have is to use some other logging prefix, like eval-warning: instead of warning: to make the distinction more clear.

roberth · 2024-05-06T16:53:27Z

Trivial rebase for CI 🤞

Constructing ErrorInfo is a little awkward for now, but this does produce a richer log entry.

... so that we may perhaps later extend the interface. Note that Nixpkgs' lib.warn already requires a string coercible argument, so this is reasonable. Also note that string coercible values aren't all strings, but in practice, for warn, they are.

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

nixos-discourse · 2024-06-06T08:12:24Z

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

https://discourse.nixos.org/t/2024-06-03-nix-team-meeting-minutes-149/46582/1

Added builtins.warn` which takes two arguments: a message that is displayed as a warning during evaluation which must be a string and a value that is returned from the expression. The next commits add new settings to control the behavior of the new builtin: `debugger-on-warn` allows the user to start the debugger and `abort-on-warn` aborts evaluation with an error. Unlike upstream, I chose not to mark evaluation warnings from `builtins.warn` as distinct from other warnings because that breaks the commonly expected logging format `level: message`. Co-authored-by: Qyriad <qyriad@qyriad.me> Upstream-PR: NixOS/nix#10592 Fixes: https://git.lix.systems/lix-project/lix/issues/579 Change-Id: I8658c88e5c27952b65e8b9f5525a572e0680cc1f

roberth added feature Feature request or proposal error-messages Confusing messages and better diagnostics language The Nix expression language; parser, interpreter, primops, evaluation, etc labels Apr 22, 2024

roberth requested a review from edolstra as a code owner April 22, 2024 23:32

github-actions Bot added the with-tests Issues related to testing. PRs with tests have some priority label Apr 22, 2024

roberth mentioned this pull request Apr 22, 2024

lib.trivial: rename NIX_ABORT_ON_WARN to NIXPKGS_ABORT_ON_WARN NixOS/nixpkgs#306026

Closed

13 tasks

This comment was marked as outdated.

Sign in to view

roberth force-pushed the builtins-warn branch from be20eb6 to 3e146f4 Compare April 23, 2024 10:25

Ericson2314 reviewed Apr 23, 2024

View reviewed changes

roberth mentioned this pull request Apr 23, 2024

[WIP] Add a non-noPos call position to primops #10597

Draft

1 task

Ericson2314 reviewed Apr 24, 2024

View reviewed changes

Ericson2314 approved these changes Apr 24, 2024

View reviewed changes

roberth mentioned this pull request Apr 24, 2024

lib.warn: Use or behave like builtins.warn NixOS/nixpkgs#306481

Merged

13 tasks

roberth force-pushed the builtins-warn branch from ad9f26d to 582de3b Compare April 24, 2024 09:24

roberth requested a review from Ericson2314 April 24, 2024 09:43

Ericson2314 reviewed Apr 24, 2024

View reviewed changes

roberth requested a review from Ericson2314 May 2, 2024 08:25

Ericson2314 reviewed May 2, 2024

View reviewed changes

Comment thread src/libexpr/primops.cc Outdated

roberth force-pushed the builtins-warn branch from b2f39fe to 91d9008 Compare May 2, 2024 16:56

lorenzleutgeb reviewed May 2, 2024

View reviewed changes

Ericson2314 approved these changes May 3, 2024

View reviewed changes

roberth force-pushed the builtins-warn branch from 91d9008 to 15b6cc5 Compare May 6, 2024 16:52

edolstra requested changes May 6, 2024

View reviewed changes

Comment thread src/libexpr/primops.cc Outdated

This comment was marked as resolved.

Sign in to view

roberth requested a review from fricklerhandwerk as a code owner June 3, 2024 12:32

github-actions Bot added the documentation label Jun 3, 2024

roberth force-pushed the builtins-warn branch 3 times, most recently from b9c5780 to 6ff8cd0 Compare June 3, 2024 13:19

edolstra approved these changes Jun 3, 2024

View reviewed changes

edolstra enabled auto-merge June 3, 2024 13:25

edolstra reviewed Jun 3, 2024

View reviewed changes

Comment thread doc/manual/rl-next/builtins-warn.md Outdated

roberth force-pushed the builtins-warn branch from 6ff8cd0 to 958643a Compare June 3, 2024 13:29

roberth and others added 7 commits June 3, 2024 16:24

Add builtins.warn

2d4c9d8

builtins.warn: Use logWarning

923cbea

Constructing ErrorInfo is a little awkward for now, but this does produce a richer log entry.

builtins.warn: Require string argument

da82d67

... so that we may perhaps later extend the interface. Note that Nixpkgs' lib.warn already requires a string coercible argument, so this is reasonable. Also note that string coercible values aren't all strings, but in practice, for warn, they are.

refactor: Extract EvalState::{runDebugRepl,canDebug}

c07500e

builtins.warn: Do not throw EvalError

831d96d

builtins.warn: Use new EvalBaseError + "evaluation warning"

70b1036

Fix typo in doc/manual/rl-next/builtins-warn.md

3a0b0af

Co-authored-by: Eelco Dolstra <edolstra@gmail.com>

roberth force-pushed the builtins-warn branch 2 times, most recently from b03c414 to 3a0b0af Compare June 3, 2024 14:43

edolstra merged commit da92ad7 into NixOS:master Jun 3, 2024

roberth mentioned this pull request Jun 29, 2024

Fix #10947; don't cache disallowed IFD #10992

Merged

roberth mentioned this pull request Jul 23, 2024

[feature request]: warning builtin #3028

Closed

Uh oh!

Conversation

roberth commented Apr 22, 2024

Motivation

Context

Priorities and Process

Uh oh!

This comment was marked as outdated.

Ericson2314 Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth Apr 23, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

roberth Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

roberth Apr 24, 2024

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Apr 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lorenzleutgeb May 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roberth commented May 6, 2024

Uh oh!

Uh oh!

This comment was marked as resolved.

Uh oh!

nixos-discourse commented Jun 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ericson2314 Apr 23, 2024 •

edited

Loading

Ericson2314 Apr 24, 2024 •

edited

Loading

lorenzleutgeb May 2, 2024 •

edited

Loading