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

Lazy fetchTree outPath path values #10252

Closed
wants to merge 10 commits into from
9 changes: 5 additions & 4 deletions src/libcmd/common-eval-args.cc
Original file line number Diff line number Diff line change
Expand Up @@ -197,15 +197,16 @@ SourcePath lookupFileArg(EvalState & state, std::string_view s, const Path * bas
if (EvalSettings::isPseudoUrl(s)) {
auto accessor = fetchers::downloadTarball(
EvalSettings::resolvePseudoUrl(s)).accessor;
auto storePath = fetchToStore(*state.store, SourcePath(accessor), FetchMode::Copy);
return state.rootPath(CanonPath(state.store->toRealPath(storePath)));
state.registerAccessor(accessor);
return SourcePath(accessor);
}

else if (hasPrefix(s, "flake:")) {
experimentalFeatureSettings.require(Xp::Flakes);
auto flakeRef = parseFlakeRef(std::string(s.substr(6)), {}, true, false);
auto storePath = flakeRef.resolve(state.store).fetchTree(state.store).first;
return state.rootPath(CanonPath(state.store->toRealPath(storePath)));
auto storePath = flakeRef.resolve(state.store).lazyFetch(state.store).first;
auto [accessor, _] = flakeRef.resolve(state.store).lazyFetch(state.store);
return SourcePath(accessor);
}

else if (s.size() > 2 && s.at(0) == '<' && s.at(s.size() - 1) == '>') {
Expand Down
67 changes: 49 additions & 18 deletions src/libexpr/eval.cc
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,7 @@ EvalState::EvalState(
{
corepkgsFS->setPathDisplay("<nix", ">");
internalFS->setPathDisplay("«nix-internal»", "");
rootFS->setPathDisplay("/", "");

countCalls = getEnv("NIX_COUNT_CALLS").value_or("0") != "0";

Expand Down Expand Up @@ -902,6 +903,11 @@ void Value::mkPath(const SourcePath & path)
mkPath(&*path.accessor, makeImmutableString(path.path.abs()));
}

void EvalState::registerAccessor(const ref<InputAccessor> accessor)
{
inputAccessors.push_back(accessor);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is like lazy-trees, except we only do it so that we don't destroy them when we put a non-smart pointer in Value, which has no finalizer because of GC and performance.



inline Value * EvalState::lookupVar(Env * env, const ExprVar & var, bool noEval)
{
Expand Down Expand Up @@ -1985,10 +1991,22 @@ void EvalState::concatLists(Value & v, size_t nrLists, Value * const * lists, co
v.mkList(list);
}

// FIXME limit recursion
Copy link
Member Author

Choose a reason for hiding this comment

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

Known issue, solve later with #10240

Value * resolveOutPath(EvalState & state, Value * v, const PosIdx pos)
{
state.forceValue(*v, pos);
if (v->type() != nAttrs)
return v;
auto found = v->attrs->find(state.sOutPath);
if (found != v->attrs->end())
return resolveOutPath(state, found->value, pos);
return v;
}

void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
{
NixStringContext context;
std::shared_ptr<InputAccessor> accessor;
std::vector<BackedStringView> s;
size_t sSize = 0;
NixInt n = 0;
Expand Down Expand Up @@ -2022,15 +2040,19 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
Value * vTmpP = values.data();

for (auto & [i_pos, i] : *es) {
Value & vTmp = *vTmpP++;
i->eval(state, env, vTmp);
Value & vTmp0 = *vTmpP++;
i->eval(state, env, vTmp0);
Value & vTmp = *resolveOutPath(state, &vTmp0, i_pos);

/* If the first element is a path, then the result will also
be a path, we don't copy anything (yet - that's done later,
since paths are copied when they are used in a derivation),
and none of the strings are allowed to have contexts. */
if (first) {
firstType = vTmp.type();
if (firstType == nPath) {
accessor = vTmp.path().accessor;
}
Comment on lines +2043 to +2055
Copy link
Member Author

Choose a reason for hiding this comment

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

Similar but not equal to lazy-trees.

  • I've kept the diff smaller by keeping vTmp as a reference
  • Not adding the first name to the path, because it is added again later. Probably a bug in lazy-trees.

}

if (firstType == nInt) {
Expand Down Expand Up @@ -2072,7 +2094,7 @@ void ExprConcatStrings::eval(EvalState & state, Env & env, Value & v)
else if (firstType == nPath) {
if (!context.empty())
state.error<EvalError>("a string that refers to a store path cannot be appended to a path").atPos(pos).withFrame(env, *this).debugThrow();
v.mkPath(state.rootPath(CanonPath(canonPath(str()))));
v.mkPath({ref(accessor), CanonPath(str())});
} else
v.mkStringMove(c_str(), context);
}
Expand Down Expand Up @@ -2322,6 +2344,8 @@ BackedStringView EvalState::coerceToString(
v._path.path
: copyToStore
? store->printStorePath(copyPathToStore(context, v.path()))
: v.path().accessor->toStringReturnsStorePath()
? store->printStorePath(copyPathToStore(context, SourcePath(v.path().accessor, CanonPath::root))) + v.path().path.absOrEmpty()
Comment on lines +2347 to +2348
Copy link
Member Author

Choose a reason for hiding this comment

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

This is new, in order for readFile "${toString ./.}/.." to work, just as it did before.
Would not recommend to write that, but similar usages of paths exist in the wild.

: std::string(v.path().path.abs());
}

Expand Down Expand Up @@ -2434,10 +2458,15 @@ SourcePath EvalState::coerceToPath(const PosIdx pos, Value & v, NixStringContext
if (v.type() == nPath)
return v.path();

/* Similarly, handle __toString where the result may be a path
/* Similarly, handle outPath and __toString where the result may be a path
value. */
if (v.type() == nAttrs) {
auto i = v.attrs->find(sToString);
auto i = v.attrs->find(sOutPath);
if (i != v.attrs->end()) {
return coerceToPath(pos, *i->value, context, errorCtx);
}
Comment on lines +2464 to +2467
Copy link
Member Author

Choose a reason for hiding this comment

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

This rule is not new. Previously, it would have worked by falling through to the coerceToString + rootPath code down below.


i = v.attrs->find(sToString);
if (i != v.attrs->end()) {
Value v1;
callFunction(*i->value, v, v1, pos);
Expand Down Expand Up @@ -2835,8 +2864,8 @@ SourcePath EvalState::findFile(const SearchPath & searchPath, const std::string_
if (!rOpt) continue;
auto r = *rOpt;

Path res = suffix == "" ? r : concatStrings(r, "/", suffix);
if (pathExists(res)) return rootPath(CanonPath(canonPath(res)));
auto res = r / suffix;
if (res.pathExists()) return res;
}

if (hasPrefix(path, "nix/"))
Expand All @@ -2851,20 +2880,22 @@ SourcePath EvalState::findFile(const SearchPath & searchPath, const std::string_
}


std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Path & value0, bool initAccessControl)
std::optional<SourcePath> EvalState::resolveSearchPathPath(const SearchPath::Path & value0, bool initAccessControl)
{
auto & value = value0.s;
auto i = searchPathResolved.find(value);
if (i != searchPathResolved.end()) return i->second;

std::optional<std::string> res;
std::optional<SourcePath> res;

if (EvalSettings::isPseudoUrl(value)) {
try {
auto accessor = fetchers::downloadTarball(
EvalSettings::resolvePseudoUrl(value)).accessor;
// Traditional search path lookups use the absolute path space for
// historical consistency.
auto storePath = fetchToStore(*store, SourcePath(accessor), FetchMode::Copy);
res = { store->toRealPath(storePath) };
res.emplace(rootPath(CanonPath(store->toRealPath(storePath))));
} catch (Error & e) {
logWarning({
.msg = HintFmt("Nix search path entry '%1%' cannot be downloaded, ignoring", value)
Expand All @@ -2876,28 +2907,28 @@ std::optional<std::string> EvalState::resolveSearchPathPath(const SearchPath::Pa
experimentalFeatureSettings.require(Xp::Flakes);
auto flakeRef = parseFlakeRef(value.substr(6), {}, true, false);
debug("fetching flake search path element '%s''", value);
auto storePath = flakeRef.resolve(store).fetchTree(store).first;
res = { store->toRealPath(storePath) };
auto accessor = flakeRef.resolve(store).lazyFetch(store).first;
res.emplace(SourcePath(accessor));
}

else {
auto path = absPath(value);
auto path = rootPath(value);

/* Allow access to paths in the search path. */
if (initAccessControl) {
allowPath(path);
if (store->isInStore(path)) {
allowPath(path.path.abs());
if (store->isInStore(path.path.abs())) {
try {
StorePathSet closure;
store->computeFSClosure(store->toStorePath(path).first, closure);
store->computeFSClosure(store->toStorePath(path.path.abs()).first, closure);
for (auto & p : closure)
allowPath(p);
} catch (InvalidPath &) { }
}
}

if (pathExists(path))
res = { path };
if (path.pathExists())
res.emplace(path);
else {
logWarning({
.msg = HintFmt("Nix search path entry '%1%' does not exist, ignoring", value)
Expand Down
9 changes: 7 additions & 2 deletions src/libexpr/eval.hh
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ public:

const SourcePath callFlakeInternal;

/* A collection of InputAccessors, just to keep them alive. */
std::list<ref<InputAccessor>> inputAccessors;

/**
* Store used to materialise .drv files.
*/
Expand Down Expand Up @@ -311,7 +314,7 @@ private:

SearchPath searchPath;

std::map<std::string, std::optional<std::string>> searchPathResolved;
std::map<std::string, std::optional<SourcePath>> searchPathResolved;

/**
* Cache used by prim_match().
Expand Down Expand Up @@ -351,6 +354,8 @@ public:
*/
SourcePath rootPath(PathView path);

void registerAccessor(ref<InputAccessor> accessor);

/**
* Allow access to a path.
*/
Expand Down Expand Up @@ -416,7 +421,7 @@ public:
*
* If it is not found, return `std::nullopt`
*/
std::optional<std::string> resolveSearchPathPath(
std::optional<SourcePath> resolveSearchPathPath(
const SearchPath::Path & elem,
bool initAccessControl = false);

Expand Down
34 changes: 29 additions & 5 deletions src/libexpr/flake/call-flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -37,18 +37,42 @@ let
builtins.mapAttrs
(key: node:
let

sourceInfo =
# FIXME: remove obsolete node.info.
tree0 =
if overrides ? ${key}
then
overrides.${key}.sourceInfo
else
# FIXME: remove obsolete node.info.
fetchTree (node.info or {} // removeAttrs node.locked ["dir"]);
fetchTree (node.info or {} // removeAttrs node.locked [
# attributes that are applied after fetching
"dir"
"outPathIsString"
]);

# TODO: split this logic into stand-alone functions
getSourceInfo = coerceOutPathToString:
tree0 // {
# TODO: return the path value without fetching to the store?
# removing this will improve performance, but may break
# one or two flakes, that rely on
# `builtins.typeOf outPath` for some reason, or perhaps
# something more subtle than that, despite our conservative
# choice of lazy path semantics.
outPath = if coerceOutPathToString then "${tree0.outPath}" else tree0.outPath;
};

subdir = overrides.${key}.dir or node.locked.dir or "";

outPath = sourceInfo + ((if subdir == "" then "" else "/") + subdir);
getOutPath = coerceOutPathToString:
getSourceInfo coerceOutPathToString + ((if subdir == "" then "" else "/") + subdir);

outPath = getOutPath outPathIsString;
sourceInfo = getSourceInfo outPathIsString;

flake0 = import (getOutPath false + "/flake.nix");

# Users can opt in to the legacy behavior of outPath being a string.
outPathIsString = flake0.inputs.self.outPathIsString or false;

flake = import (outPath + "/flake.nix");

Expand Down
Loading