Skip to content

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

Merged
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/move-create-directory-out-of-assert
Oct 21, 2024
Merged

fix(nix/eval.cc): move call to fs::create_directory out of assert#11718
Mic92 merged 1 commit intoNixOS:masterfrom
xokdvium:dev/move-create-directory-out-of-assert

Conversation

@xokdvium
Copy link
Copy Markdown
Contributor

@xokdvium xokdvium commented Oct 18, 2024

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 b5c8865 and this really stood out to me. I left a comment in the original PR #11650 (comment), but that did not get any traction. So I'm submitting this PR so this does not get lost.

Context

#11650

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@xokdvium
fix(nix/eval.cc): move call to fs::create_directory out of assert
8277b50 
If the call is inside the assertion, then in non-assert builds
the call would be stripped out. This is highly unexpected.
@xokdvium xokdvium requested a review from edolstra as a code owner October 18, 2024 21:47
@github-actions github-actions bot added the new-cli Relating to the "nix" command label Oct 18, 2024
@xokdvium
Copy link
Copy Markdown
Contributor Author

xokdvium commented Oct 18, 2024

CC @Ericson2314. I'm not familiar with the code at all. But just looking at the diff b5c8865 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?

Mic92
Mic92 approved these changes Oct 21, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

good catch

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Oct 21, 2024

@mergify queue

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 21, 2024 •
edited
Loading

queue

🛑 The pull request has been removed from the queue default

Details

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.

@Mic92
Copy link
Copy Markdown
Member

Mic92 commented Oct 21, 2024

@mergify rebase

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Oct 21, 2024

rebase

☑️ Nothing to do

Details
  • any of:
    • #commits > 1 [📌 rebase requirement]
    • #commits-behind > 0 [📌 rebase requirement]
    • -linear-history [📌 rebase requirement]
  • -closed [📌 rebase requirement]
  • -conflict [📌 rebase requirement]
  • queue-position = -1 [📌 rebase requirement]

@Mic92 Mic92 merged commit b93b910 into NixOS:master Oct 21, 2024
@Ericson2314
Copy link
Copy Markdown
Member

Ericson2314 commented Oct 21, 2024

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

@xokdvium xokdvium deleted the dev/move-create-directory-out-of-assert branch October 21, 2024 19:49
This was referenced Nov 7, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mic92 Mic92 Mic92 approved these changes

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

+1 more reviewer

@bryanhonof bryanhonof bryanhonof approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

new-cli Relating to the "nix" command

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@xokdvium @Mic92 @Ericson2314 @bryanhonof