Nix repl flakes by tomberek · Pull Request #6233 · NixOS/nix

tomberek · 2022-03-11T18:55:11Z

Based on: #5824

Add ability to nix repl to utilize installables syntax.

TODO: more tests

Fixes #5824

src/nix/repl.cc

doc/manual/src/release-notes/rl-next.md

src/nix/repl.md

Ericson2314 · 2022-03-17T22:48:33Z

n.b. nix repl fooDir barDir bazDir might also have a different interpretation with this? That does make it a bigger breaking change.

I think we should consider this:

Deprecated features, the opposite of experimental features
an "old nix repl syntax" deprecated feature
Two different CmdRepl, nix v1-repl, nix v2-repl and we vary which one nix repl is using the deprecated feature.

nix v2-repl would require the "nix command" unstable feature so we are free to refine it.

roberth · 2022-03-31T13:42:16Z

src/nix/repl.cc

+
+    for (auto & [i,what] : getValues()) {
+        notice("Loading Installable '%1%'...", what);
+        addAttrsToScope(*i);


So this effectively merges the attributes of multiple installables?

I don't like that "loadables" are loaded into the global scope anyway. It makes discovering which attributes exist harder than it should.

Perhaps flakes and installables could be put in a binding that's derived from the installable name?

# nix repl nixpkgs Loading Installable 'flake:nixpkgs#'... Added 1 variable. nix-repl> nixpkgs.legacyPackages.x86_64-linux.emacs.name "emacs-27.1" nix-repl> nixpkgs.<TAB> <UX delight ensues as all top-level flake attrs are listed>

Also interesting

# nix repl . .#inputs.nixpkgs Added 2 variables nix-repl> foo.<TAB> nix-repl> nixpkgs.lib.concatStringsSep ":" ["testing" "it"]

The only problem with this is that flakes don't have an obvious name, so it's not entirely straightforward what the top-level attribute name (e.g. nixpkgs) should be.

Right. I pulled foo out of thin air.
Most flake refs have a somewhat straightforward candidate for a name.
Using dirname $PWD for . has produced mixed success in the past though.

Maybe the attribute name should be provided by the user.

nix repl --bind foo . --bind nixpkgs .#inputs.nixpkgs

or

nix repl foo=. nixpkgs=.#inputs.nixpkgs

Adding a bare flake or installable to the top-level scope seems ok-ish as long as there's only one. Or maybe it should default to flake= for a flake and a name derived from the installable otherwise?

We do have the previous behavior of multiple --files to worry about.

I wouldn't mind if it just made an array called args, hah! Then don't have to worry about names, and :a can put whatever one likes at the top level.

tomberek · 2022-04-07T19:15:32Z

Suggestions from NixUX:

more docs
more tests
improve error message for legacy usage nix repl '<nixpkgs>' (or use --file?)

Fixes: #5824

thufschmitt

Left a couple of small stylistic comments, but nothing really serious.

I’m a bit more concerned about the unexpect{ed,able} backwards-compatibility breakage. Maybe that could be eased by

Gating the change behind an xp feature flag (if possible making the non-experimental behavior just a layer on top of the new one, otherwise by just brutally duplicating everything)
Adding a flag --file to the “legacy” codepath (that would essentially be a no-op but allows a common interface between both versions)
Warn about usages of nix repl foo.nix, suggest using --file
After one (or two) releases, switch the default to the new behavior and remove the old codepath

Also (somewhat related to the above), --file apparently doesn’t auto-call functions properly anymore, so nix repl --file '<nixpkgs>' fails with error: value is a function while a set was expected

src/nix/repl.cc

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.

repl: search installable with findAlongAttrPath repl: refactor handling of args repl: temp

Co-authored-by: Théophane Hufschmitt <[email protected]>

src/nix/repl.cc

@edolstra

Style fixes from @edolstra Co-authored-by: Eelco Dolstra <[email protected]>

Co-authored-by: Eelco Dolstra <[email protected]>

thufschmitt · 2022-05-20T06:39:23Z

@tomberek I’ve added some tests in thufschmitt@1b00dcb if you want to cherry-pick that

- Test that without the XP feature things work as before - Test that with or without the XP feature `--file file` works - Test that with XP feature passing a flakeref works - Test `:reload` with a flake

thufschmitt

Thanks for keeping through my annoying reviews. Except for the small test failure, that’s looking good now!

tests/repl.sh

Co-authored-by: Théophane Hufschmitt <[email protected]>

tomberek · 2022-06-02T20:58:44Z

@bburdette my rebase caused me to interact a bit with your refactor. Please let me know if I've incorrectly resolved something.

bburdette · 2022-06-02T21:41:13Z

@tomberek as far as I can tell its fine.

thufschmitt

Duh sorry, I though I had it already merged.

I have a couple suggestions for the non-code side, and there's a merge conflict, but looks good otherwise 👍

src/nix/repl.md

doc/manual/src/release-notes/rl-next.md

Co-authored-by: Théophane Hufschmitt <[email protected]>

thufschmitt · 2022-06-24T06:12:21Z

@tomberek think you can have a look at the failing test?

thufschmitt

Thanks :)

The hardcoded ca-derivations xp feature in the test is a bit annoying, but that'll do for now

tomberek requested a review from Ericson2314 March 11, 2022 18:55

Ericson2314 reviewed Mar 11, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

Ericson2314 reviewed Mar 11, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

Ericson2314 reviewed Mar 11, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

Ericson2314 reviewed Mar 11, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

tomberek force-pushed the nix-repl-flakes branch 5 times, most recently from 8309301 to a9a5b85 Compare March 11, 2022 19:41

Ericson2314 mentioned this pull request Mar 11, 2022

nix repl demonstrates some new-cli CLI ideoms need generalization #6234

Open

Artturin suggested changes Mar 15, 2022

View reviewed changes

doc/manual/src/release-notes/rl-next.md Outdated Show resolved Hide resolved

src/nix/repl.md Outdated Show resolved Hide resolved

garbas assigned tomberek and edolstra Mar 17, 2022

tomberek force-pushed the nix-repl-flakes branch from d0145b8 to c60e350 Compare March 17, 2022 20:34

roberth reviewed Mar 31, 2022

View reviewed changes

thufschmitt suggested changes May 18, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

src/nix/repl.cc Outdated Show resolved Hide resolved

src/nix/repl.cc Outdated Show resolved Hide resolved

Timothy DeHerrera and others added 5 commits May 18, 2022 21:20

nix repl: load flakes from cli args

06d57ce

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.

repl: allow loading installables from CLI

81567a0

repl: search installable with findAlongAttrPath repl: refactor handling of args repl: temp

repl: use installables

5640b52

repl: update docs with installables

1ca3f60

Apply suggestions from code review

9f8c118

Co-authored-by: Théophane Hufschmitt <[email protected]>

tomberek force-pushed the nix-repl-flakes branch from fe13c30 to 9f8c118 Compare May 19, 2022 01:22

refactor: factor out getValue

7534798

tomberek force-pushed the nix-repl-flakes branch from bbf7f0f to 7534798 Compare May 19, 2022 01:36

tomberek added 2 commits May 18, 2022 22:28

repl: provide backward compat with legacy usage

e1f308a

repl: hide flake behavior behind flag and provide warning

f21dec5

tomberek requested a review from thufschmitt May 19, 2022 05:03

thufschmitt reviewed May 20, 2022

View reviewed changes

src/nix/repl.cc Outdated Show resolved Hide resolved

src/nix/repl.cc Outdated Show resolved Hide resolved

src/nix/repl.cc Outdated Show resolved Hide resolved

tomberek and others added 4 commits May 20, 2022 01:28

Apply suggestions from code review

7d7e002

Style fixes from @edolstra Co-authored-by: Eelco Dolstra <[email protected]>

repl: allow --file to always utilize autoargs

db613a8

Apply suggestions from code review

542e36c

Co-authored-by: Eelco Dolstra <[email protected]>

repl: clarify change and usage of <nixpkgs>

82c4af4

Théophane Hufschmitt and others added 3 commits May 20, 2022 07:42

Add some tests for the new REPL cli

9381504

- Test that without the XP feature things work as before - Test that with or without the XP feature `--file file` works - Test that with XP feature passing a flakeref works - Test `:reload` with a flake

repl: fix tests to run on any testing store

0053dab

repl: add repl-flake experimental feature for gating

7a04fb1

thufschmitt reviewed May 20, 2022

View reviewed changes

tests/repl.sh Outdated Show resolved Hide resolved

tests/repl.sh Outdated Show resolved Hide resolved

Apply suggestions from code review

8c3939a

Co-authored-by: Théophane Hufschmitt <[email protected]>

tomberek force-pushed the nix-repl-flakes branch 3 times, most recently from cbabcf7 to 36ce1fb Compare June 2, 2022 20:38

Merge branch 'master' into nix-repl-flakes

ffd41d1

tomberek force-pushed the nix-repl-flakes branch from 36ce1fb to ffd41d1 Compare June 2, 2022 20:58

tomberek requested a review from thufschmitt June 14, 2022 21:24

thufschmitt approved these changes Jun 15, 2022

View reviewed changes

src/nix/repl.md Outdated Show resolved Hide resolved

doc/manual/src/release-notes/rl-next.md Show resolved Hide resolved

tomberek and others added 2 commits June 15, 2022 09:02

Apply suggestions from code review

dae4a8a

Co-authored-by: Théophane Hufschmitt <[email protected]>

Merge branch 'master' into nix-repl-flakes

51268ce

tests: enable ca-derivations for simple.nix in repl tests

f801d70

tomberek requested a review from thufschmitt June 25, 2022 15:25

thufschmitt approved these changes Jun 29, 2022

View reviewed changes

thufschmitt merged commit 4c82100 into NixOS:master Jun 29, 2022

Ericson2314 mentioned this pull request Mar 11, 2023

repl: add :lh command #4392

Closed

Ericson2314 mentioned this pull request May 17, 2023

Move worker_proto defs out of remote-store.cc to own file #8360

Merged

8 tasks

thufschmitt mentioned this pull request Feb 28, 2024

Remove the repl-flake experimental feature #10103

Closed

Uh oh!

Comments

Conversation

tomberek commented Mar 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Ericson2314 commented Mar 17, 2022

Uh oh!

roberth Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

edolstra Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

roberth Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

Ericson2314 Mar 31, 2022

Choose a reason for hiding this comment

Uh oh!

tomberek commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

thufschmitt commented May 20, 2022

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tomberek commented Jun 2, 2022

Uh oh!

bburdette commented Jun 2, 2022

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thufschmitt commented Jun 24, 2022

Uh oh!

thufschmitt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

tomberek commented Mar 11, 2022 •

edited

Loading

tomberek commented Apr 7, 2022 •

edited

Loading