nix icon indicating copy to clipboard operation
nix copied to clipboard

fix(nix/eval.cc): move call to `fs::create_directory` out of `assert`

Open xokdvium opened this issue 1 year ago • 1 comments

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.

xokdvium avatar Oct 18 '24 21:10 xokdvium

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?

xokdvium avatar Oct 18 '24 21:10 xokdvium

@mergify queue

Mic92 avatar Oct 21 '24 12:10 Mic92

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[bot] avatar Oct 21 '24 12:10 mergify[bot]

@mergify rebase

Mic92 avatar Oct 21 '24 12:10 Mic92

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]

mergify[bot] avatar Oct 21 '24 12:10 mergify[bot]

Thanks! I think we always do asserts, but this is still better. (Indeed, always doing asserts is a work-around for mistakes like this.)

Ericson2314 avatar Oct 21 '24 15:10 Ericson2314