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

Inline the usage of nix::readDirectory and remove it #10684

Merged
merged 3 commits into from
May 13, 2024

Conversation

siddhantk232
Copy link
Contributor

nix::readDirectory is removed. std::filesystem::directory_iterator is used directly in places that used this util.

Motivation

The nix::readDirectory uses std::filesystem::directory_iterator and pushes all the entries in a std::vector before returning it. This is redundant as we can directly use std::filesystem::directory_iterator to iterate through entries and count files (and dirs) in path.

Context

#9205

Priorities and Process

Add 👍 to pull requests you find important.

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

`nix::readDirectory` is removed. `std::filesystem::directory_iterator`
is used directly in places that used this util.
@github-actions github-actions bot added new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store repl The Read Eval Print Loop, "nix repl" command and debugger labels May 12, 2024
Copy link
Member

@edolstra edolstra left a comment

Choose a reason for hiding this comment

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

Looks good apart from the test failure.

src/libstore/unix/builtins/unpack-channel.cc Outdated Show resolved Hide resolved
@edolstra
Copy link
Member

Can you resolve the conflicts? Then we can merge this.

@edolstra edolstra changed the title Inline the usage of nix::readDirectory and remove it. Inline the usage of nix::readDirectory and remove it May 13, 2024
@edolstra edolstra enabled auto-merge May 13, 2024 13:40
@edolstra edolstra merged commit 1623249 into NixOS:master May 13, 2024
9 checks passed
@Ericson2314
Copy link
Member

@siddhantk232 can you make sure ever for loop callsite has a "check interrupt" like before? This will make sure we don't regress with control-C support and very wide directories.

@siddhantk232
Copy link
Contributor Author

@siddhantk232 can you make sure ever for loop callsite has a "check interrupt" like before? This will make sure we don't regress with control-C support and very wide directories.

Thanks for pointing this out! I'll open another PR that does this.

if (entries.size() == 1)
tmpFile = entries[0].path();
auto entries = std::filesystem::directory_iterator{unpacked};
auto file_count = std::distance(entries, std::filesystem::directory_iterator{});
Copy link
Member

Choose a reason for hiding this comment

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

This broke the prefetch command if unpack is used. Fixed in #11051

auto entries = readDirectory(out);
if (entries.size() != 1)
auto entries = std::filesystem::directory_iterator{out};
auto fileName = entries->path().string();
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this way around it actually works.

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 repl The Read Eval Print Loop, "nix repl" command and debugger store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants