nix
nix copied to clipboard
fix(nix/eval.cc): move call to `fs::create_directory` out of `assert`
If the call is inside the assertion, then in non-assert builds the call would be stripped out. This is highly unexpected.
Motivation
I was looking through recently merged commits and ran across b5c88650c57ac39a1631fda29e25c7a457e2a503 and this really stood out to me. I left a comment in the original PR https://github.com/NixOS/nix/pull/11650#issuecomment-2408932294, but that did not get any traction. So I'm submitting this PR so this does not get lost.
Context
https://github.com/NixOS/nix/pull/11650
Priorities and Process
Add :+1: to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.
CC @Ericson2314. I'm not familiar with the code at all. But just looking at the diff b5c88650c57ac39a1631fda29e25c7a457e2a503 this does not look correct to me.
I'm also somewhat confused by this usage of assert. Is this directory not existing an invariant maintained by nix at all times and would never occur?
@mergify queue
queue
🛑 The pull request has been removed from the queue default
The merge conditions cannot be satisfied due to failing checks.
You can take a look at Queue: Embarked in merge queue check runs for more details.
In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.
@mergify rebase
rebase
☑️ Nothing to do
- [ ] any of:
- [ ]
#commits > 1[📌 rebase requirement] - [ ]
#commits-behind > 0[📌 rebase requirement] - [ ]
-linear-history[📌 rebase requirement]
- [ ]
- [X]
-closed[📌 rebase requirement] - [X]
-conflict[📌 rebase requirement] - [X]
queue-position = -1[📌 rebase requirement]
Thanks! I think we always do asserts, but this is still better. (Indeed, always doing asserts is a work-around for mistakes like this.)