Skip to content

libmain: fix ignoring empty lines in the print-build-logs option#12133

Merged
mergify[bot] merged 1 commit intoNixOS:masterfrom
momeemt:#11991-logs_ignore_empty_lines
Jan 18, 2025
Merged

libmain: fix ignoring empty lines in the print-build-logs option#12133
mergify[bot] merged 1 commit intoNixOS:masterfrom
momeemt:#11991-logs_ignore_empty_lines

Conversation

@momeemt
Copy link
Copy Markdown
Member

@momeemt momeemt commented Jan 3, 2025

Closes: #11991

Motivation

The logger previously ignored empty lines in the print-build-logs option.

Running the following nix file with this option shows that empty lines are not printed in the log, even though the build script prints them.

# repro.nix

let buildscript = builtins.toFile "build.sh" ''
	#!/bin/sh
	echo "start"
	echo
	echo
	echo
	echo "end"
	: > $out
'';
in
builtins.derivation {
	name = "repro";
	builder = "/bin/sh";
	args = [ buildscript ];
	outputs = [ "out" ];
	system = builtins.currentSystem;
}
$ nix build -f ./repro.nix -L --rebuild
repro> start
repro> end
$ nix log -f ./repro.nix | cat
start



end

I fixed the problem by removing !lastLine.empty(), so it now outputs like this.

$ nix build -f ./repro.nix -L --rebuild
repro> start
repro>
repro>
repro>
repro> end
$nix log -f ./repro.nix | cat
start



end

Context

This problem was reported by #11991.

Add 👍 to pull requests you find important.

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

@momeemt momeemt requested a review from edolstra as a code owner January 3, 2025 04:27
@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 6, 2025

https://discourse.nixos.org/t/2024-12-04-nix-team-meeting-minutes-200/57005

In our nix meeting we decided that it should not print repeated empty lines. A single empty line is fine.

@momeemt
Copy link
Copy Markdown
Member Author

momeemt commented Jan 8, 2025

discourse.nixos.org/t/2024-12-04-nix-team-meeting-minutes-200/57005

In our nix meeting we decided that it should not print repeated empty lines. A single empty line is fine.

Please let me check just to be sure.
The current implementation does not output a blank line, is it correct to fix compressing to a single empty line?
Or is it to close this pull request?

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 8, 2025

@momeemt So just now it emits no empty newline and we would be ok with emitting one empty newline and than discarding the rest. Unless we have a real good use case for multiple new lines, we think just having one empty newline makes logs more readable.

roberth
roberth approved these changes Jan 15, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

Did we consider that silently omitting newlines could still be surprising to users?
It'd still be unexpected, but also a more complex behavior that's harder to "figure out by accident".
I think Nix should log something like <multiple newlines omitted> in ASCII faint color.

Nonetheless, this is already an improvement as is.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Jan 15, 2025

I guess you are right. Our proposal would be just an optimization to that.

@roberth
Copy link
Copy Markdown
Member

roberth commented Jan 18, 2025

@Mergifyio queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Jan 18, 2025 •
edited
Loading

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at bcb92a5

mergify bot added a commit that referenced this pull request Jan 18, 2025
@mergify
Merge of #12133
0049a6b
@mergify mergify bot mentioned this pull request Jan 18, 2025
Closed
37 tasks
@mergify mergify bot merged commit bcb92a5 into NixOS:master Jan 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@edolstra edolstra Awaiting requested review from edolstra edolstra is a code owner

1 more reviewer

@roberth roberth roberth approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

--print-build-logs|-L ignores empty lines

3 participants

@momeemt @Mic92 @roberth