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

fix(nixos-generate-config): dont output carriage returns in output files #439

Merged
merged 2 commits into from
Dec 23, 2024

Conversation

sedlund
Copy link
Contributor

@sedlund sedlund commented Dec 18, 2024

ssh -t is being used by runSsh which causes new lines in output designed for a terminal. It could also output escape sequences.

https://unix.stackexchange.com/a/420438

this change fixes this problem (#438) by only outputting new lines. but it might be worth removing ssh password entry as an option and the tty allocation, and force the use of sshpass for passwords.

There is a FIXME note regarding the output of the nixos-facter with strange characters. This may resolve that as well.

Also updated the message about nixos-facter not being found to hopefully clarify and resolve #423 around the --generate-hardware-config option.

@sedlund sedlund changed the title fix(nixos-generate-config): dont ouput carriage returns in output files fix(nixos-generate-config): dont output carriage returns in output files Dec 18, 2024
@Mic92
Copy link
Member

Mic92 commented Dec 20, 2024

What version did you use for nixos-facter? We switched logging libraries at some point, which resolved an issue with ANSI escapes.

@sedlund
Copy link
Contributor Author

sedlund commented Dec 21, 2024

should have quoted:

step "Generating hardware-configuration.nix using nixos-facter"
# FIXME: if we take the output directly it adds some weird characters at the beginning
runSsh -o ConnectTimeout=10 ${maybeSudo} "nixos-facter" >"$hardwareConfigPath"

I was unsure what 'weird characters' referred to.

So maybe what you refer to fixed that, but CRLF in output persists with tty allocation with ssh.

I tested with:

ssh -t localhost nixos-generate-config --show-hardware-config --no-filesystems > ngc-old.out
ssh -t localhost "stty nl; nixos-generate-config --show-hardware-config --no-filesystems" > ngc-new.out
ssh -t localhost sudo nixos-facter > facter-old.out
ssh -t localhost "stty nl; sudo nixos-facter" > facter-new.out

fd -e out -x nix shell nixpkgs#tinyxxd -c xxd {} {.}.xxd
file *.out
facter-new.out: JSON text data
facter-old.out: JSON text data
ngc-new.out:    Unicode text, UTF-8 text
ngc-old.out:    Unicode text, UTF-8 text, with CRLF line terminators
delta --side-by-side ngc-old.xxd ngc-new.xxd
delta --side-by-side facter-old.xxd facter-new.xxd

this is qpqn0b91qbh879snbqpr04z8rdi7zkq8-nixos-facter-0.3.0.

diffs show CRLF in the old output

@Mic92
Copy link
Member

Mic92 commented Dec 21, 2024

We can remove the comment. It was caused by charm bracelet's logging library: numtide/nixos-facter#119
It wrote some ansi escape at the very beginning, which broke the json output. However this was removed.

@Mic92
Copy link
Member

Mic92 commented Dec 21, 2024

Are you running on macOS on the host by chance?

@Mic92
Copy link
Member

Mic92 commented Dec 21, 2024

But I think you are write, we shouldn't use -t for these commands. It's only needed for the installation because we need support for sudo and other password prompts.

@Mic92
Copy link
Member

Mic92 commented Dec 21, 2024

Could you try to remove it and see if this is fixing the issue?

@sedlund
Copy link
Contributor Author

sedlund commented Dec 23, 2024

Are you running on macOS on the host by chance?

my client is NixOS-wsl. dont know what reporter in #438 is running.

created a new runSshNoTty function that omits the tty request.

tested outputs of both nixos-facter and nixos-generate-config on a hetzner vm - no CRLF in this patched version.

@sedlund sedlund marked this pull request as ready for review December 23, 2024 05:57
@Mic92
Copy link
Member

Mic92 commented Dec 23, 2024

@Enzime this is also what we saw the other day. But with macOS line endings.

@Mic92
Copy link
Member

Mic92 commented Dec 23, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Dec 23, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 24f2d37

@mergify mergify bot merged commit 24f2d37 into nix-community:main Dec 23, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixos-facter not available
2 participants