-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
nix shell: Handle output paths that are symlinks #10467
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,9 +39,9 @@ void SourceAccessor::readFile( | |
} | ||
|
||
Hash SourceAccessor::hashPath( | ||
const CanonPath & path, | ||
PathFilter & filter, | ||
HashAlgorithm ha) | ||
const CanonPath & path, | ||
PathFilter & filter, | ||
HashAlgorithm ha) | ||
{ | ||
HashSink sink(ha); | ||
dumpPath(path, sink, filter); | ||
|
@@ -67,4 +67,42 @@ std::string SourceAccessor::showPath(const CanonPath & path) | |
return displayPrefix + path.abs() + displaySuffix; | ||
} | ||
|
||
CanonPath SourceAccessor::resolveSymlinks( | ||
const CanonPath & path, | ||
SymlinkResolution mode) | ||
{ | ||
auto res = CanonPath::root; | ||
|
||
int linksAllowed = 1024; | ||
|
||
std::list<std::string> todo; | ||
for (auto & c : path) | ||
todo.push_back(std::string(c)); | ||
|
||
while (!todo.empty()) { | ||
auto c = *todo.begin(); | ||
todo.pop_front(); | ||
if (c == "" || c == ".") | ||
; | ||
else if (c == "..") | ||
res.pop(); | ||
else { | ||
res.push(c); | ||
if (mode == SymlinkResolution::Full || !todo.empty()) { | ||
if (auto st = maybeLstat(res); st && st->type == SourceAccessor::tSymlink) { | ||
if (!linksAllowed--) | ||
throw Error("infinite symlink recursion in path '%s'", showPath(path)); | ||
auto target = readLink(res); | ||
res.pop(); | ||
if (hasPrefix(target, "/")) | ||
res = CanonPath::root; | ||
todo.splice(todo.begin(), tokenizeString<std::list<std::string>>(target, "/")); | ||
} | ||
} | ||
} | ||
} | ||
|
||
return res; | ||
} | ||
Comment on lines
+70
to
+106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can be deduped There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the code in See the implementation of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's not the job of this PR though. It just moves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK fair |
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear why this is needed (calling it with a parent of the store feels quite wrong).
I guess it should at least honor
requireValidPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because when following a symlink from one store path to another,
resolveSymlinks()
will stat the parents of those paths, so it will do amaybeLstat("/nix")
andmaybeLstat("/nix/store")
. Those are not valid store paths so it failed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about whether stores' source accessors should prefix everything with the store directory or not, and leaning towards "no".
This would be something not doing that helps with: the path is just
xxxxxxxxxxxx-foo
, no parent directories etc.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makeMountedInputAccessor
can be used to put put things in store directories if it matters.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#10510 Broke into new issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that #10510 isn't fixed, I think this is actually a fine fix for now.