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.

Sign up for GitHub

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

Jump to bottom

Fix nix eval nixpkgs#bash segfault #10200

Merged
merged 2 commits into from
Mar 11, 2024
Merged

Fix nix eval nixpkgs#bash segfault #10200

merged 2 commits into from
Mar 11, 2024

Conversation

9999years
Copy link
Contributor

nix eval forces values and prints derivations as attribute sets, so commands that print derivations (e.g. nix eval nixpkgs#bash) will infinitely loop and segfault.

Printing derivations as .drv paths makes nix eval complete as expected. Further work is needed, but this is better than a segfault.

@9999years
Print derivation paths in nix eval
4910d74 
`nix eval` forces values and prints derivations as attribute sets, so
commands that print derivations (e.g. `nix eval nixpkgs#bash`) will
infinitely loop and segfault.

Printing derivations as `.drv` paths makes `nix eval` complete as
expected. Further work is needed, but this is better than a segfault.
@9999years 9999years requested a review from edolstra as a code owner March 9, 2024 05:48
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Mar 9, 2024
@tomberek tomberek added the idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. label Mar 11, 2024
tomberek
tomberek requested changes Mar 11, 2024
View reviewed changes
Copy link
Contributor

@tomberek tomberek left a comment

Choose a reason for hiding this comment

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

Looks good. Please add a release note to notify people.

During team discussion we also considered additional behavior, but this seems to be a net-positive regardless of future changes.

@9999years
Copy link
Contributor Author

Added a release note, PTAL.

@9999years 9999years requested a review from tomberek March 11, 2024 16:21
@9999years 9999years force-pushed the fix-nix-eval-for-derivations branch from 02289e8 to 7f45b1c Compare March 11, 2024 17:03
@tomberek tomberek merged commit 25bf671 into NixOS:master Mar 11, 2024
8 checks passed
@9999years 9999years deleted the fix-nix-eval-for-derivations branch March 11, 2024 17:59
@kjeremy
Copy link

kjeremy commented Mar 12, 2024

Can we get a backport? I think I'm hitting this.

@thufschmitt thufschmitt added backport 2.18-maintenance Automatically creates a PR against the branch backport 2.19-maintenance Automatically creates a PR against the branch backport 2.20-maintenance Automatically creates a PR against the branch backport 2.21-maintenance Automatically creates a PR against the branch labels Mar 12, 2024
@github-actions GitHub Actions
Copy link

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-10200-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-10200-to-2.18-maintenance
git switch --create backport-10200-to-2.18-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions GitHub Actions
Copy link

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-10200-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-10200-to-2.19-maintenance
git switch --create backport-10200-to-2.19-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions GitHub Actions
Copy link

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-10200-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-10200-to-2.18-maintenance
git switch --create backport-10200-to-2.18-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

1 similar comment
@github-actions GitHub Actions
Copy link

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-10200-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-10200-to-2.18-maintenance
git switch --create backport-10200-to-2.18-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions GitHub Actions
Copy link

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-10200-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-10200-to-2.19-maintenance
git switch --create backport-10200-to-2.19-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

1 similar comment
@github-actions GitHub Actions
Copy link

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-10200-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-10200-to-2.19-maintenance
git switch --create backport-10200-to-2.19-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions github-actions bot mentioned this pull request Mar 12, 2024
Closed
@github-actions GitHub Actions
Copy link

Successfully created backport PR for 2.20-maintenance:

@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.20-maintenance with exitcode 1

1 similar comment
@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.20-maintenance with exitcode 1

@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.21-maintenance with exitcode 1

@github-actions GitHub Actions
Copy link

Backport failed for 2.18-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.18-maintenance
git worktree add -d .worktree/backport-10200-to-2.18-maintenance origin/2.18-maintenance
cd .worktree/backport-10200-to-2.18-maintenance
git switch --create backport-10200-to-2.18-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions GitHub Actions
Copy link

Successfully created backport PR for 2.21-maintenance:

@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.21-maintenance with exitcode 1

@github-actions github-actions bot mentioned this pull request Mar 12, 2024
Closed
@github-actions GitHub Actions
Copy link

Backport failed for 2.19-maintenance, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin 2.19-maintenance
git worktree add -d .worktree/backport-10200-to-2.19-maintenance origin/2.19-maintenance
cd .worktree/backport-10200-to-2.19-maintenance
git switch --create backport-10200-to-2.19-maintenance
git cherry-pick -x 4910d74086a85876e093136a0e8ebc547b467af7 7f45b1c8d8caf4beeb68c981ae813d6251a7ee63

@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.20-maintenance with exitcode 1

@github-actions GitHub Actions
Copy link

Git push to origin failed for 2.21-maintenance with exitcode 1

@thufschmitt
Copy link
Member

The bot just opened backport PRs for 2.20 and 2.21 (the other ones didn't apply cleanly), but I actually don't think we can as this is a breaking change (not the fact that it doesn't segfault any more, but nix eval --expr '{ type = "derivation"; drvPath = "blah"; }' now behaves differently)

@9999years
Copy link
Contributor Author

@thufschmitt Not a breaking change, nix-command is an experimental feature with no stability guarantees.

@thufschmitt
Copy link
Member

@thufschmitt Not a breaking change, nix-command is an experimental feature with no stability guarantees.

I know (that's why it has been changed here), but it's still a trade-off (this potentially breaks people, and doing that on a patch release is uncool, even for experimental features).

Not married to it though, if you think it's worth backporting, feel free to reopen the PRs (I think you have the right, if not ping me).

@thufschmitt thufschmitt mentioned this pull request Mar 20, 2024
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-03-11-nix-team-meeting-132/42960/1

@pnmadelaine pnmadelaine mentioned this pull request Jun 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tomberek tomberek tomberek approved these changes

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

Assignees

@tomberek tomberek

Labels
backport 2.18-maintenance Automatically creates a PR against the branch backport 2.19-maintenance Automatically creates a PR against the branch backport 2.20-maintenance Automatically creates a PR against the branch backport 2.21-maintenance Automatically creates a PR against the branch idea approved The given proposal has been discussed and approved by the Nix team. An implementation is welcome. new-cli Relating to the "nix" command
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@9999years @kjeremy @thufschmitt @nixos-discourse @tomberek