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

stdenv: only log hooks if $NIX_DEBUG >= 4 #331560

Merged

Conversation

philiptaron
Copy link
Contributor

@philiptaron philiptaron commented Aug 1, 2024

Motivation for change

Make the logging output by default from the stdenv less verbose.

Description of changes

Due to feedback about the amount of logging output in the above issue, this PR moves the hook logging added in PRs #290081 and #310387 under $NIX_DEBUG, specifically at "talkative" level.

As a part of this commit, nixLog is no more, replaced with these eight available logging functions:

  1. nixErrorLog (logs at $NIX_DEBUG>=0 level, which is always)
  2. nixWarnLog (logs at $NIX_DEBUG>=1 level)
  3. nixNoticeLog (logs at $NIX_DEBUG>=2 level)
  4. nixInfoLog (logs at $NIX_DEBUG>=3 level)
  5. nixTalkativeLog (logs at $NIX_DEBUG>=4 level)
  6. nixChattyLog (logs at $NIX_DEBUG>=5 level)
  7. nixDebugLog (logs at $NIX_DEBUG>=6 level)
  8. nixVomitLog (logs at $NIX_DEBUG>=7 level)

All of these names are from the Nix source code.

Setting $NIX_DEBUG is left as an exercise for the reader.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@emilazy emilazy left a comment

Choose a reason for hiding this comment

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

A little nit. Don’t feel like building stdenv to test this out, but the basic temporary compromise seems sound to me, even if it will probably satisfy nobody.

I do think that this should probably be a higher value than 2 so that we have room to add things that are less noisy than this, but it doesn’t really matter that much.

pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
@philiptaron philiptaron marked this pull request as draft August 1, 2024 18:01
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from 0f6c60c to 1938735 Compare August 1, 2024 18:51
@philiptaron philiptaron changed the title stdenv: only log hooks if $NIX_DEBUG >= 2 stdenv: only log hooks if $NIX_DEBUG >= 3 Aug 1, 2024
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from 1938735 to c48d19f Compare August 1, 2024 18:53
pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from c48d19f to a7363f6 Compare August 2, 2024 23:15
@philiptaron philiptaron marked this pull request as ready for review August 2, 2024 23:17
pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from a7363f6 to 916aedb Compare August 3, 2024 17:39
@philiptaron philiptaron changed the title stdenv: only log hooks if $NIX_DEBUG >= 3 stdenv: only log hooks if $NIX_DEBUG >= 4 Aug 3, 2024
@philiptaron philiptaron requested review from emilazy and tie August 3, 2024 20:47
@philiptaron
Copy link
Contributor Author

To test this, btw, I use nix-build --arg pkgs 'import ./. {}' ./lib/tests/release.nix, and then again with a custom Nix that sets NIX_DEBUG to see the other side.

pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
pkgs/stdenv/generic/setup.sh Show resolved Hide resolved
pkgs/stdenv/generic/setup.sh Outdated Show resolved Hide resolved
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from 916aedb to 2c93603 Compare August 5, 2024 23:18
@philiptaron philiptaron force-pushed the issue-328229/stdenv-spam-less-only branch from 2c93603 to d8fbb16 Compare August 6, 2024 03:23
# $NIX_DEBUG must be a documented integer level, if set, so we can use it safely as an integer.
# See the `Verbosity` enum in the Nix source for these levels.
if ! [[ -z ${NIX_DEBUG-} || $NIX_DEBUG == [0-7] ]]; then
printf 'The `NIX_DEBUG` environment variable has an unexpected value: %s\n' "${NIX_DEBUG}"
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 it’s OK to use echo in this case since we know that the argument will not be interpreted as a flag.

Suggested change
printf 'The `NIX_DEBUG` environment variable has an unexpected value: %s\n' "${NIX_DEBUG}"
echo "The `NIX_DEBUG` environment variable has an unexpected value: ${NIX_DEBUG@Q}"

Note: ${parameter@Q} quotes the string, similar to printf %q. See https://www.gnu.org/software/bash/manual/html_node/Shell-Parameter-Expansion.html

Copy link
Contributor

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

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

This change looks good to me. Ran the tests over night and it seems to work well in practice as well, although I didn't test with debugging enabled. I agree with keeping it simple with the cost of some duplication. Would be very nice to get this merged as the current logging of hooks seems to the be causing some real world problems.

@philiptaron philiptaron merged commit 3f3dd53 into NixOS:staging Aug 6, 2024
25 of 27 checks passed
@philiptaron philiptaron deleted the issue-328229/stdenv-spam-less-only branch August 6, 2024 17:03
@Mindavi
Copy link
Contributor

Mindavi commented Aug 9, 2024

Not sure if this is an issue with the logging infra or nix, but I'm noticing some issues in nix develop mode:

mindavi@nixos-asus:~/nixpkgs$ nix develop -f default.nix xenPackages.xen_4_17
mindavi@nixos-asus:~/nixpkgs$ unpackPhase 
unpacking source archive /nix/store/hwh8vvj30w09w328hmpbkmmhjn034njn-xen-d530627
bash: "$NIX_LOG_FD": Bad file descriptor
source root is xen-d530627
bash: "$NIX_LOG_FD": Bad file descriptor

@tie
Copy link
Member

tie commented Aug 9, 2024

@Mindavi, this should have been fixed in #330830 👀

@Mindavi
Copy link
Contributor

Mindavi commented Aug 9, 2024

Ah ok, thanks. Staging is not on master yet and my working tree is normally not staging. So probably with a next staging merge it'll be good.

@Atemu
Copy link
Member

Atemu commented Aug 9, 2024

@Mindavi note that the PR is in staging-next already if you need it urgently.

@Mindavi
Copy link
Contributor

Mindavi commented Aug 9, 2024

No worries, not in a rush 🙂

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

Successfully merging this pull request may close these issues.

Sourcing setup hook *default hook* spam in writers/trivial builders
6 participants