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

Replace call to stat, lstat, and maybeLstat with appropriate functions from std::filesystem #10937

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

siddhantk232
Copy link
Contributor

@siddhantk232 siddhantk232 commented Jun 19, 2024

Motivation

The cpp std::filesystem has functions that we can use in place of stat and lstat. This makes the code more portable between windows and unix.

Some places where it required windows native implementation are still using unix::lstat (the old lstat just moved inside the new unix namespace). Alternative windows implementation needs to be written to tackle these situations.

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.

...with `std::filesystem` functions

The `stat` defined in `file-system.cc` is not used anywhere now. This
will be removed with the removal of the `STAT` macro defined in the same
file.
`lstat` is still used in some places so we cannot remove the `STAT`
macro.
the `maybeLstat` is removed in favor of the new `maybeSymlinkStat` which
uses the `std::filesystem` library. All the call sites have been updated
... `unix::lstat`

appropriate functions from `std::filesystem` is used in places where
`lstat` was called. Places that require platform specific implementation
are using `unix::lstat` for now.
- The `std::filesystem::status` returns `not_found` and does not throw
  any error.

- `std::filesystem::*` is shortened to `fs::*` for better readability
The result of this function cannot be `std::filesystem::file_status` yet
as we use it to acess `st_mtime`. The equivalent function in
`std::filesystem` to get the last write time does follow symlinks which
is not the behavior we want.
@github-actions github-actions bot added new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store fetching Networking with the outside (non-Nix) world, input locking labels Jun 19, 2024
A typo (`"src" -> "dst"`) caught by functional tests is fixed
The removed `nix::maybeLstat` was referenced here. It's replaced with
the new function that replaced it.
try {
fs::permissions(src, st.permissions() | fs::perms::owner_write, permOpts);
} catch (const fs::filesystem_error & e) {
throw SysError("setting permissions on '%s'", src);
Copy link
Member

Choose a reason for hiding this comment

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

Does SysError actually work correctly after a std::filesystem exception? I don't know if it's guaranteed to leave errno in a known state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The documentation says that fs::filesystem_error is only thrown on OS errors so I think it's safe to assume that the errno is appropriately set when we're in this catch block?

Copy link
Member

Choose a reason for hiding this comment

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

That's probably a safe assumption on Unix-like systems, but I'm not sure if it's the case on Windows. Maybe @Ericson2314 knows?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

I suppose on windows we than get these long error codes instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Ericson2314 I have changed the code to use the SysError with explicit errNo (see e0090fe)

@@ -196,11 +184,24 @@ std::optional<struct stat> maybeLstat(const Path & path)
}
return st;
}
#endif

std::optional<fs::file_status> maybeSymlinkStat(const Path & path)
Copy link
Member

Choose a reason for hiding this comment

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

Given that we're changing this function anyway, can we change path to be a std::filesystem::path?

Copy link
Member

Choose a reason for hiding this comment

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

Why is this function called maybeSymlinkStat and the other maybeLstat? Seems inconsistent.

Copy link
Contributor Author

@siddhantk232 siddhantk232 Jun 26, 2024

Choose a reason for hiding this comment

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

Why is this function called maybeSymlinkStat and the other maybeLstat? Seems inconsistent.

The unix::maybeLstat (and unix::lstat) will be removed very soon. The places where it's used currently demand specific functions (getDeviceIdOfFile and getLastWriteTime for example) which I'll soon implement.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2024-06-24-nix-team-meeting-minutes-155/47739/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fetching Networking with the outside (non-Nix) world, input locking new-cli Relating to the "nix" command store Issues and pull requests concerning the Nix store
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants