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

remote-store: don't log raw stderr by default #3558

Merged
merged 2 commits into from May 4, 2020
Merged

Conversation

@LnL7
Copy link
Member

LnL7 commented May 2, 2020

For remote stores the log messages are already forwarded as structured
STDERR_RESULT messages so the old format is duplicate information. But
still included with -vvv since it could be useful for debugging
problems.

$ nix build -L /nix/store/nl71b2niws857ffiaggyrkjwgx9jjzc0-foo.drv --store ssh-ng://localhost
Hello World!
foo> Hello World!
[1/0/1 built] building foo

Fixes #3556

@LnL7
Copy link
Member Author

LnL7 commented May 2, 2020

Actually this breaks build output of nix-build when talking to the daemon. So it seems the daemon case handles this somewhere else.

LnL7 added 2 commits May 2, 2020
For remote stores the log messages are already forwarded as structured
STDERR_RESULT messages so the old format is duplicate information.  But
still included with -vvv since it could be useful for debugging
problems.

    $ nix build -L /nix/store/nl71b2niws857ffiaggyrkjwgx9jjzc0-foo.drv --store ssh-ng://localhost
    Hello World!
    foo> Hello World!
    [1/0/1 built] building foo

Fixes #3556
The raw stderr output isn't logged anymore so the build logs need to be
printed by the default logger in order for the old commands like
nix-build to still show build output.
@LnL7 LnL7 force-pushed the LnL7:ssh-ng-stderr branch from 7bcd39d to 4769eea May 2, 2020
@edolstra
Copy link
Member

edolstra commented May 4, 2020

@LnL7 Is this ready to be merged (given your last comment)?

@LnL7
Copy link
Member Author

LnL7 commented May 4, 2020

Yeah, that last commit resolves it.

Not entirely clear why the daemon doesn't currently suffer from this however, something related to the fact that ssh is going through the protocol twice since I'm not using root? (client -> ssh -> deamon)

@edolstra edolstra merged commit 3ebfbec into NixOS:master May 4, 2020
2 checks passed
2 checks passed
tests (ubuntu-18.04)
Details
tests (macos)
Details
@LnL7
Copy link
Member Author

LnL7 commented May 4, 2020

Thanks!

@LnL7 LnL7 deleted the LnL7:ssh-ng-stderr branch May 4, 2020
@Ma27
Copy link
Member

Ma27 commented May 5, 2020

Hmm, this seems to have the downside that e.g. hash-mismatch errors aren't shown @LnL7 @edolstra

@LnL7
Copy link
Member Author

LnL7 commented May 5, 2020

Oh indeed 😕, I didn't notice that yet.

@LnL7
Copy link
Member Author

LnL7 commented May 5, 2020

Only for fixed output drvs however builtins.fetchurl is handled correctly, I'll take a look.

@LnL7
Copy link
Member Author

LnL7 commented May 5, 2020

The following works for nix-build, but the new commands still won't show anything without -L so I'm not sure that's the way to go. Adding a new result type for this would be one option but that seems a bit adhoc.

diff --git a/src/libstore/build.cc b/src/libstore/build.cc
index 147093fa..a81d0101 100644
--- a/src/libstore/build.cc
+++ b/src/libstore/build.cc
@@ -3695,6 +3695,9 @@ void DerivationGoal::registerOutputs()
                 /* Throw an error after registering the path as
                    valid. */
                 worker.hashMismatch = true;
+                worker.act.result(resBuildLogLine, fmt("hash mismatch in fixed-output derivation '%s':", worker.store.printStorePath(dest)));
+                worker.act.result(resBuildLogLine, fmt("  wanted: %s", h.to_string(SRI)));
+                worker.act.result(resBuildLogLine, fmt("  got:    %s", h2.to_string(SRI)));
                 delayedException = std::make_exception_ptr(
                     BuildError("hash mismatch in fixed-output derivation '%s':\n  wanted: %s\n  got:    %s",
                         worker.store.printStorePath(dest), h.to_string(SRI), h2.to_string(SRI)));
@edolstra
Copy link
Member

edolstra commented May 6, 2020

I'll revert this for now until we figure out what to do.

edolstra added a commit that referenced this pull request May 6, 2020
This reverts commit 3ebfbec, reversing
changes made to c089c52.

#3558
@Ma27
Copy link
Member

Ma27 commented May 6, 2020

@edolstra makes sense, but can you please reopen #3556 then?

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

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.