feat: builtins.readSymlink primop #15093
Conversation
github-actions
bot
added
the
with-tests
Ericson2314
left a comment
Ericson2314
left a comment
There was a problem hiding this comment.
I like it
(and I was not at the meeting where this was originally idea-approved)
JustAGuyTryingHisBest
force-pushed
the
read-symlink-primop
branch
from
99e4af7 to
69ba166
Compare
|
@Ericson2314 - Installer CI failed on an authentication issue unrelated to my change. I don't have the option to rerun. Is that only available to admins? |
|
Done |
Rad. Much appreciated! |
src/libexpr/primops.cc
Outdated
| static void prim_readSymlink(EvalState & state, const PosIdx pos, Value ** args, Value & v) | ||
| { | ||
| auto path = state.realisePath(pos, *args[0], SymlinkResolution::Ancestors); | ||
| v.mkString(path.readLink(), state.mem); |
There was a problem hiding this comment.
Oh hmm actually this part might need more thinking hmm. I would rather that it be a path.
There was a problem hiding this comment.
What about reading symlinks inside trees fetched by fetchTree or similar - those might very well be dangling or relative.
There was a problem hiding this comment.
That makes sense! I updated the implementation to return a path.
There was a problem hiding this comment.
What about reading symlinks inside trees fetched by fetchTree or similar - those might very well be dangling or relative.
See what I wrote in obsidiansystems@own-location-for-symlink, which @xokdvium and I have talked about before.
The issue, for example, that some source successors, like git repos for flakes for pure eval, shouldn't really support absolute paths at all, because the repo isn't necessary mounted at root or anywhere else. It should only support relative paths.
My suggestion that we should convert the raw target of the symlink into a path is a bit more complex than what you implemented if we take this concern into account, because we need to actually define this per-source-accessor policy of how symlinks should be interpreted / what should be allowed.
Sorry this is spiraling into something more complex than what you started with!
There was a problem hiding this comment.
I reread the original issue, and it appears the intended behavior is to match POSIX readlink returning a raw string.
Does it make sense to open a new ticket to differentiate this from the original ticket and POSIX realpath behavior?
Ericson2314
dismissed
their stale review
Noticed one thing that I want to think about more
| @@ -0,0 +1 @@ | |||
| error: filesystem error: read_symlink: not a symlink ["/pwd/lang/readDir/bar"] | |||
There was a problem hiding this comment.
Oh, this error really needs to be wrapped in the accessor IMO
There was a problem hiding this comment.
Is the suggestion here to catch the error in the primop and throw NotASymlink("file '%s' is not a symbolic link", showPath(path)); or add a cleaner error message in PosixSourceAccessor::readLink().
There was a problem hiding this comment.
To be clear, @xokdivium is just noticing a preexisting issue with PosixSourceAcccesor, it is not the fault of this PR.
src/libexpr/primops.cc
Outdated
| static void prim_readSymlink(EvalState & state, const PosIdx pos, Value ** args, Value & v) | ||
| { | ||
| auto path = state.realisePath(pos, *args[0], SymlinkResolution::Ancestors); | ||
| v.mkString(path.readLink(), state.mem); |
There was a problem hiding this comment.
What about reading symlinks inside trees fetched by fetchTree or similar - those might very well be dangling or relative.
| const auto path = state.realisePath(pos, *args[0], SymlinkResolution::Ancestors); | ||
| const auto target = path.readLink(); | ||
| const auto parent = path.parent(); | ||
| v.mkPath(SourcePath(path.accessor, CanonPath(target, parent.path)), state.mem); |
There was a problem hiding this comment.
It's a bit questionable whether this should return a path value or just the raw symlink as a string (without canonicalisation). If the former, maybe this function should be called builtins.canonPath or builtins.resolvePath or whatever (and accept non-symlink paths as well).
4 participants
Motivation
#13111 (comment)
Context
This is a stepping stone to - #13111
Add 👍 to pull requests you find important.
The Nix maintainer team uses a GitHub project board to schedule and track reviews.