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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos-rebuild: Fix regression from #277642 #280842

Merged
merged 3 commits into from
Jan 14, 2024

Conversation

roberth
Copy link
Member

@roberth roberth commented Jan 14, 2024

Description of changes

This should fix the regression from #277642 (comment).

See commit messages for details.

Tests run

  • nix-build -A nixosTests.nixos-rebuild-target-host catches the revert (ie without the fix)
  • nix-build -A nixosTests.nixos-rebuild-target-host on the second commit shows no regression
  • NO REGRESSION TEST FOR --build-host YET
    • merge as hotfix nonetheless?

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 馃憤 reaction to pull requests you find important.

This reverts commit 09fd207.

It caused a regression when using `--build-host` and flakes.
See NixOS#277642 (comment)
This solves again the problem solved by 09fd207.

To quote:

> We always want to use `ssh -t` to force PTY allocation as there may be
> interactive SSH prompts like trusting unknown hosts.

However, the creation of a pseudoterminal causes the remote stdout and stderr
to point to the same tty, resulting in a single stream in the ssh client,
which breaks other usages of ssh, such as `--build-host`.

Hence, this commit only sets the flag for invocations that need it -
or would need it if sudo were disabled. That should help with development
and gives a somewhat more consistent user experience.
@roberth roberth marked this pull request as draft January 14, 2024 02:41
@roberth roberth marked this pull request as ready for review January 14, 2024 03:03
"${a[@]}" => ok
"${foo:+a[@]}" => empty string when length is 0
@roberth roberth merged commit 48ffcd6 into NixOS:master Jan 14, 2024
4 of 5 checks passed
@roberth
Copy link
Member Author

roberth commented Jan 14, 2024

Self merged to solve critical bug.

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

Successfully merging this pull request may close these issues.

None yet

1 participant