Disable cache when a flake --update-input is explicitly specified. #4288
Conversation
If the user has supplied the explicit --update-input flag, then the input should be fetched from the remote regardless of whether it was previously fetched and cached (usually within the last hour). The caching can lead to user confusion and frustration when updates don't appear to be getting performed properly.
| } | ||
| }; | ||
|
|
||
| std::shared_ptr<disabler> disableCacheLookups(void) { |
There was a problem hiding this comment.
Any reason for making this an external function rather than just exporting the constructor?
There was a problem hiding this comment.
This is a fairly standard C++ idiom for providing a safe context region of operation. By using a shared pointer to a context object it ensures that the object's destructor is called to revert/shutdown the context when corresponding scope is no longer active, regardless of how that scope was exited (return, exception, etc.). In fact, the disableCacheLookups is the expected API point; the struct disabler is not intended to be used directly.
There was a problem hiding this comment.
Yes I know the idiom, I just don't see what the shared pointer gives, compared to directly calling the constructor (like it's done in other places in the repo (the Finally or Activity classes for example).
(I apologize in case I miss something obvious. I'm by no means a c++ expert, it just seems to me that these lines are not in line with how things are done in the rest of the codebase)
There was a problem hiding this comment.
The shared pointer immunizes the operation against caller mis-use such as copying/duplicating, passing by value, etc.
The Finally requires the caller to explicitly specify the cleanup activity at each usage point, whereas the shared pointer provides encapsulation and avoids repetition.
I also have concerns that the use of Finally without assignment might be subject to either outright removal or re-ordering of the destructor call due to compiler optimization passes because of the lack of any local reference in the calling location; if Finally were assigned to a local variable, that would mitigate my concern, but that's not the current usage pattern and is also outside the scope of this PR.
I'm not sure how Activity applies here since I'm not intending any associated Logger functionality.
I used the shared pointer idiom here because it is simple, safe, and a relatively common idiom for C++ developers.
| time (see 'nix show-config | grep tarball-ttl') | ||
| but if this was an explicit user request for an | ||
| update, ignore any recently cached fetch. */ | ||
| auto inputFlake = lockFlags.inputUpdates.count(inputPath) |
There was a problem hiding this comment.
Do we want this conditional? It looks like it would bring a strange behavior, in particular for --recreate-lock-file (which I'd expect to work like passing --update-input for every input)
There was a problem hiding this comment.
I'm not sure I understand your question: without this conditional the entire patch is irrelevant. This is the main place where the functionality of this PR is actually introduced: this checks that --update-input was specified for the corresponding inputPath and if so, ensures that the fetch cache is bypassed so that the input is actually retrieved from the remote rather than the cache.
The --recreate-lock-file has slightly different semantics as I understand it based on the existing flake code, and it does not necessarily imply an --update-input for each input. To change those semantics would need logic to be implemented elsewhere and presumably affect the population inputUpdates contents at that point, but that's beyond the scope of the intended effect of this PR and should probably be handled separately.
There was a problem hiding this comment.
Sorry if that was unclear. My understanding of the code is that we can reach this point either when passing --update-input or becase there's no previous lockfile (or --recreate-lock-file is passed which I think has the same effect). This PR only disables the cache in the first case, but maybe we want to disable it in every case. After all, this cache is mostly here so that you can do nix build -f channel:nixos-unstable hello without re-downloading the nixos-unstable channel each time, and it might not make sense at all with flakes.
There was a problem hiding this comment.
The interaction between --recreate-lock-file (or the lack of a lock file) and the --update-input argument is ill-defined and the subject of some dispute (c.f. issue number 3779). I'd prefer to keep this PR specifically targeted and not get caught up in unresolved design decisions.
|
I marked this as stale due to inactivity. → More info |
If the user has supplied the explicit --update-input flag, then the
input should be fetched from the remote regardless of whether it was
previously fetched and cached (usually within the last hour). The
caching can lead to user confusion and frustration when updates don't
appear to be getting performed properly.