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

Don't recommend 'nix log' unless experimental feature is enabled #8122

Conversation

bjornfor
Copy link
Contributor

Motivation

This fixes the issue that nix-build, without experimental feature 'nix-command' enabled, recommends the experimental CLI nix log to view build logs. Now it'll recommend the stable nix-store -l CLI instead.

Context

Fixes #8118.

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

Priorities

Add 👍 to pull requests you find important.

@bjornfor bjornfor marked this pull request as draft March 28, 2023 06:52
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, that makes a lot of sense, indeed.

Just a small style suggestion, feel free to ignore it if you want

src/libstore/build/derivation-goal.cc Outdated Show resolved Hide resolved
This fixes the issue that `nix-build`, without experimental feature
'nix-command' enabled, recommends the experimental CLI `nix log` to view
build logs. Now it'll recommend the stable `nix-store -l` CLI instead.

Fixes NixOS#8118
@bjornfor bjornfor force-pushed the use-nix-store-l-unless-experimental-enabled branch from a443639 to 74d94b2 Compare March 28, 2023 20:40
@bjornfor
Copy link
Contributor Author

I've marked the PR as a draft, because in my testing it keeps printing nix log instead of nix-store -l.

@bjornfor
Copy link
Contributor Author

I've marked the PR as a draft, because in my testing it keeps printing nix log instead of nix-store -l.

cd nix_from_this_pr && $(nix-build --no-out-link)/bin/nix-build -E '(import <nixpkgs> {}).runCommand "failing" {} "echo planned failure; false"' doesn't seem to use Nix from this PR, I don't understand why. Is it the daemon that executes the code in this PR?

@thufschmitt
Copy link
Member

Is it the daemon that executes the code in this PR?

Yes. Which indeed explains why it didn't work for you. You can use --store /tmp/some-path to directly use /tmp/some-path as the store (and not use the daemon). Very handy for this kind of testing

@bjornfor
Copy link
Contributor Author

Is it the daemon that executes the code in this PR?

Yes. Which indeed explains why it didn't work for you. You can use --store /tmp/some-path to directly use /tmp/some-path as the store (and not use the daemon). Very handy for this kind of testing

With --store /tmp/some-path the PR code doesn't seem to be run at all:

$ $(nix-build --no-out-link)/bin/nix-build -E '(import <nixpkgs> {}).runCommand "failing" {} "echo planned failure; false"' --builders '' --store /tmp/temp-nix-store-path
warning: Git tree '/home/bf/src/nix' is dirty
this derivation will be built:
  /nix/store/jjq9jdmi90a1iyb3np5i03lkj5d303z2-failing.drv
building '/nix/store/jjq9jdmi90a1iyb3np5i03lkj5d303z2-failing.drv'...
planned failure
error: builder for '/nix/store/jjq9jdmi90a1iyb3np5i03lkj5d303z2-failing.drv' failed with exit code 1

(It's missing the For full logs, run ... line.)

I'm not sure what to do now. I don't know how to test it and I'm starting to worry that this change isn't even correct, because the client and the daemon can have different/conflicting experimental-feature sets?

@bjornfor bjornfor marked this pull request as ready for review April 1, 2023 10:37
@bjornfor
Copy link
Contributor Author

bjornfor commented Apr 1, 2023

I'm not sure what to do now. I don't know how to test it and I'm starting to worry that this change isn't even correct, because the client and the daemon can have different/conflicting experimental-feature sets?

Un-drafted, because I don't think this change can make the situation any worse.

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.

This is good. Yeah it is a bug this is even showing up with nix-build at all.

The proper solution is to get this UI logic somewhere else rather than part of the building logic, but this will do for now. As you say, it is strictly better than before.

@Ericson2314 Ericson2314 dismissed thufschmitt’s stale review April 1, 2023 18:50

Code was changed as requested

@Ericson2314 Ericson2314 merged commit 2ef99cd into NixOS:master Apr 1, 2023
@bjornfor bjornfor deleted the use-nix-store-l-unless-experimental-enabled branch April 2, 2023 09:21
@fricklerhandwerk fricklerhandwerk added UX The way in which users interact with Nix. Higher level than UI. new-cli Relating to the "nix" command error-messages Confusing messages and better diagnostics cli The old and/or new command line interface labels Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli The old and/or new command line interface error-messages Confusing messages and better diagnostics new-cli Relating to the "nix" command UX The way in which users interact with Nix. Higher level than UI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nix-build suggests nix log (experimental CLI) to view logs
4 participants