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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

xokdvium
Copy link
Contributor

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.

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
Contributor Author

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?

Copy link
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
Member

Mic92 commented Oct 21, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Oct 21, 2024

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.

@Mic92
Copy link
Member

Mic92 commented Oct 21, 2024

@mergify rebase

Copy link
Contributor

mergify bot commented Oct 21, 2024

rebase

☑️ Nothing to do

  • 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
11 of 12 checks passed
@Ericson2314
Copy link
Member

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-cli Relating to the "nix" command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants