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

local-store: fix infinite loop on Windows #12251

Merged
merged 1 commit into from
Jan 27, 2025

Conversation

puffnfresh
Copy link
Member

Also switch to std::filesystem.

Motivation

Context


Add 👍 to pull requests you find important.

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

Also switch to std::filesystem.
@github-actions github-actions bot added the store Issues and pull requests concerning the Nix store label Jan 14, 2025
std::filesystem::path path = realStoreDir.get();
std::filesystem::path root = path.root_path();
while (path != root) {
if (std::filesystem::is_symlink(path))
Copy link
Member Author

Choose a reason for hiding this comment

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

Original comments on this were:

Would be nice to wrap is_symlink in an try-catch and re-throw a nix error instead the original exception that std::filesystem throws because those actually will have an error trace added when the exception is bubbled up.

And:

The function rethrowExceptionAsError() can be used for that. (Should be moved from local-derivation-goal.cc to libutil, probably.)

Copy link
Member Author

Choose a reason for hiding this comment

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

From reading the filesystem API docs and the source for GCC libstdc++, I can see that bad memory allocation should be the only exception. I don't think it makes sense to try/catch here.

@puffnfresh puffnfresh marked this pull request as draft January 14, 2025 05:18
@puffnfresh puffnfresh marked this pull request as ready for review January 26, 2025 23:16
@edolstra edolstra enabled auto-merge January 27, 2025 13:21
@edolstra edolstra merged commit 6a2198d into NixOS:master Jan 27, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants