Skip to content

nix key: no need for progressBar#9307

Merged
edolstra merged 1 commit intoNixOS:masterfrom
flox:tomberek.nix_key_newline
Nov 16, 2023
Merged

nix key: no need for progressBar#9307
edolstra merged 1 commit intoNixOS:masterfrom
flox:tomberek.nix_key_newline

Conversation

@tomberek
Copy link
Contributor

@tomberek tomberek commented Nov 6, 2023

otherwise the output will be invisible in common terminal configurations

Motivation

Alternative to: #8849

Context

Intended to make the output visible. Previously there was output, but terminals would often overwrite that output, making it look like the command failed.

Priorities

Add 👍 to pull requests you find important.

Fixes: #8844

@tomberek tomberek requested a review from edolstra as a code owner November 6, 2023 18:15
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Nov 6, 2023
@roberth
Copy link
Member

roberth commented Nov 6, 2023

It seems that all writeFull(STDOUT_FILENO, ...) will suffer from this. We have five of these an cat.cc seems to have the same problem. If you could turn this combination (stop and write) into a helper function, we'll avoid some future copy pasta bugs.

@roberth
Copy link
Member

roberth commented Nov 6, 2023

Usages:

$ git grep writeFull\(STDOUT_FILENO
src/libutil/logging.cc:    writeFull(STDOUT_FILENO, s);
src/libutil/logging.cc:    writeFull(STDOUT_FILENO, "\n");
src/nix/cat.cc:        writeFull(STDOUT_FILENO, accessor->readFile(CanonPath(path)));
src/nix/eval.cc:            writeFull(STDOUT_FILENO, *state->coerceToString(noPos, *v, context, "while generating the eval command output"));
src/nix/log.cc:            writeFull(STDOUT_FILENO, *log);
src/nix/sigs.cc:        writeFull(STDOUT_FILENO, SecretKey::generate(*keyName).to_string());
src/nix/sigs.cc:        writeFull(STDOUT_FILENO, secretKey.toPublicKey().to_string());

@tomberek
key and cat: no need for progressBar
0be84c8 
otherwise the output will be invisible in common terminal configurations
@tomberek tomberek force-pushed the tomberek.nix_key_newline branch from bf50240 to 0be84c8 Compare November 13, 2023 00:43
@Ericson2314
Copy link
Member

Ericson2314 commented Nov 13, 2023

Does this not use the logging stuff because we are worried about secrets being stored in a log?

@tomberek
Copy link
Contributor Author

tomberek commented Nov 14, 2023

Does this not use the logging stuff because we are worried about secrets being stored in a log?

Just keeping the changes minimal for now. With other progress bar or related things in-flight I didn't feel refactoring would be productive - don't always have to be DRY.

@tomberek
Copy link
Contributor Author

tomberek commented Nov 15, 2023

reproducer for cat:

printf "no eol" > no-eol.txt  && nix store cat $(nix store add-file ./no-eol.txt)
printf "eol\n" > eol.txt  && nix store cat $(nix store add-file ./eol.txt)

@edolstra edolstra merged commit 16c052e into NixOS:master Nov 16, 2023
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

Assignees

No one assigned

Labels

new-cli Relating to the "nix" command

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

nix key convert-secret-to-public is broken in 2.17

4 participants

@tomberek @roberth @Ericson2314 @edolstra