Conversation
If experimental feature "flakes" is enabled, args passed to `nix repl` will now be considered flake refs and imported using the existing `:load-flake` machinery. In addition, `:load-flake` now supports loading flake fragments.
thufschmitt
left a comment
thufschmitt
left a comment
There was a problem hiding this comment.
As a small bonus, I've implemented the ability for the :load-flake command to parse flake ref fragments (i.e. the part after the #)
Yay! That’s cool 😎
when experimental feature "flakes" is enabled, it makes sense to consider the arguments as flake refs instead of files
That’s a tricky thing to decide. Because it means breaking (again) backwards-compatibility once flake aren’t experimental anymore.
Maybe a nicer path would be to
- Add a
--fileflag for loading a file - Add a
--flakeflag for loading a flake - Deprecate (but keep) the current behavior and recommend to use
--fileinstead - Eventually (after flakes are non-experimental anymore, and after at least 2 or 3 6w cycles) change the behavior
Possibly (dunno how much work that would be), the last step could be done by moving the cli of nix repl to use the same mechanism as the other commands (making it an instance of InstallableCommand) to make it fully consistent with the rest
It would also be nice to include the same search behavior as other flake ref parsing commands, e.g. nixpkgs#zlib becomes nixpkgs#legacyPackages.$current_system.zlib
Yes, that sounds like that would be great (and probably also for :lf)
| throw Error("cannot use ':load-flake' on mutable flake reference '%s' (use --impure to override)", flakeRefS); | ||
|
|
||
| Value v; | ||
| auto v = state->allocValue(); |
There was a problem hiding this comment.
What’s the reason for the change? Having v as a plain stack-allocated Value is nice since it has a lexical lifetime.
There was a problem hiding this comment.
Oh I think I copied that from somewhere else while try to debug a stack overflow. I'll change it back 👍
| addAttrsToScope(v); | ||
| *v); | ||
|
|
||
| auto f = v->attrs->get(state->symbols.create(fragment)); |
There was a problem hiding this comment.
I think this won’t work properly if fragment has more than one level (foo.bar). You probably need to split it first with parseAttrPath and recursively get the right value.
There was a problem hiding this comment.
I'll give that a shot. While we are at it, is there a function or method somewhere used to traverse the multiple outputs to find a match?
edolstra
left a comment
edolstra
left a comment
There was a problem hiding this comment.
Could you mention this in the release notes?
|
|
||
| auto f = v->attrs->get(state->symbols.create(fragment)); | ||
|
|
||
| if (f == 0) { |
There was a problem hiding this comment.
| if (f == 0) { | |
| if (!f) { |
| auto f = v->attrs->get(state->symbols.create(fragment)); | ||
|
|
||
| if (f == 0) { | ||
| warn("no attribute %s, nothing loaded", fragment); |
There was a problem hiding this comment.
| warn("no attribute %s, nothing loaded", fragment); | |
| warn("attribute '%s' does not exist, nothing loaded", fragment); |
| return; | ||
| }; | ||
|
|
||
| fragment != "" ? addAttrsToScope(*f->value) : addAttrsToScope(*v); |
There was a problem hiding this comment.
| fragment != "" ? addAttrsToScope(*f->value) : addAttrsToScope(*v); | |
| addAttrsToScope(fragment != "" ? *f->value : *v); |
| settings.isExperimentalFeatureEnabled(Xp::Flakes) | ||
| ? loadFlake(i) | ||
| : loadFile(i); |
There was a problem hiding this comment.
| settings.isExperimentalFeatureEnabled(Xp::Flakes) | |
| ? loadFlake(i) | |
| : loadFile(i); | |
| if (settings.isExperimentalFeatureEnabled(Xp::Flakes)) | |
| loadFlake(i); | |
| else | |
| loadFile(i); |
since it's not really idiomatic in C++ (IMHO) to have the conditional operator at statement level.
|
Added a bit of cleanup here: flox@da4dcb5 After this i wanted to start using InstallableCommand,InstallablesCommand, or SourceExprCommand so that we are using the Installable concept instead of having a conditional on an experimental flag. Directly using that works, but also brings some unwanted default behavior: should probably NOT use a default attrpath of |
thufschmitt
added
feature
|
Working on #6233 |
5 participants
I think when working with flakes it is quite common to load the flake into a repl for inspecting and debugging. Therefore, when experimental feature "flakes" is enabled, it makes sense to consider the arguments as flake refs instead of files.
As a small bonus, I've implemented the ability for the
:load-flakecommand to parse flake ref fragments (i.e. the part after the#).I still need to modify the documentation, and I'm debating on whether I should add new
--files, -fflag for when the old behavior is desired while using flakes, so I'm opening as draft for feedback. It would also be nice to include the same search behavior as other flake ref parsing commands, e.g.nixpkgs#zlibbecomesnixpkgs#legacyPackages.$current_system.zlib.