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

Add repl-overlays setting #10203

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

9999years
Copy link
Contributor

@9999years 9999years commented Mar 9, 2024

Motivation

Fixes #9940 by adding a repl-overlays setting that can add bindings to the nix repl.

~/.config/nix/repl.nix:

info: final: prev: let
  optionalAttrs = predicate: attrs:
    if predicate
    then attrs
    else {};
in
  optionalAttrs (prev ? legacyPackages && prev.legacyPackages ? ${info.currentSystem})
  {
    pkgs = prev.legacyPackages.${info.currentSystem};
  }

Then:

$ nix repl --repl-overlays ~/.config/nix/repl.nix nixpkgs
Nix 2.21.0pre20240309_4111bb6
Type :? for help.
Loading installable 'flake:nixpkgs#'...
Added 5 variables.
Loading 'repl-init-files'...
Added 6 variables.
nix-repl> pkgs.bash
«derivation /nix/store/g08b5vkwwh0j8ic9rkmd8mpj878rk62z-bash-5.2p26.drv»

Priorities and Process

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@github-actions github-actions bot added repl The Read Eval Print Loop, "nix repl" command and debugger fetching Networking with the outside (non-Nix) world labels Mar 9, 2024
@9999years 9999years force-pushed the repl-default-bindings branch 3 times, most recently from a7b5ef3 to 0e185d5 Compare March 10, 2024 00:15
@9999years 9999years marked this pull request as ready for review March 10, 2024 01:10
@9999years 9999years changed the title Add repl-init-file setting Add repl-init-files setting Mar 10, 2024
@9999years 9999years changed the title Add repl-init-files setting Add repl-overlays setting Mar 11, 2024
@roberth
Copy link
Member

roberth commented Mar 12, 2024

This could alternatively be solved by improving the composition that can be done on the CLI.
For instance, we currently have --apply, but in the team, we've discussed a variation that would take positional or named arguments. Unfortunately that discussion hasn't made it from the notes into an issue yet, as we haven't quite decided what it should be.

Only vaguely related as of now: #7076

@Qyriad
Copy link
Member

Qyriad commented Mar 12, 2024

Part of the whole point of this feature is that it doesn't require additional interaction to use — just a config file.

@roberth
Copy link
Member

roberth commented Mar 13, 2024

Configuration is a double edged sword. It lets "power users" be more comfortable, most of the time, but it requires everyone to invest time into that sort of thing, or accept an unpleasant product. Meanwhile even power users aren't unaffected by the bad defaults, when they help someone with a different config, install onto a new machine, or need to do a bit of admin work on a remote host they don't own.

The example fixes a pretty major annoyance, so I'm not sure if more configurability is the solution. Maybe it should just work better out of the box.
Hardcoding or setting a default would achieve that, but I prefer to keep Nix lean when possible. What if the repl could load the extra attributes from the flake itself? Then every user of a given flake gets the same experience, and the individual projects and/or flake libraries get to take advantage of the feature, so they can expose some things, like pkgs, but also internal values that aren't intended for other tools and flakes to consume.

While this doesn't completely solve the "configuration problem" I started with, it does solve it for popular flakes and flakes that use libraries like flake-utils and flake-parts. I don't think anyone should write flake outputs by hand.

@Lunaphied
Copy link
Contributor

Configuration is a double edged sword. It lets "power users" be more comfortable, most of the time, but it requires everyone to invest time into that sort of thing, or accept an unpleasant product.

I do not see how having an extra feature be supported optionally has any impact on users that don't wish to use it. What is Nix but a tool ultimately that is extremely focused on power users?

This proposal does not bind itself to flakes, such as all your examples do, which is a significant downside as not every workflow involves flakes. Furthermore if your desire is to keep Nix "lean" flexible configuration (possibly with sane defaults!) is the exact approach to addressing this issue.

I don't see the point ultimately about hypothesizing endlessly about potential other solutions. I've seen this behavior on several of your other PR responses and it has personally made me feel like contributing to the Nix ecosystem is unproductive. The perfect is the enemy of the good, and this feature is good.

What's happening here (and in Nix in general it seems) is endless bikeshedding while features people could start using today to improve their workflows languish, even when fully designed and ready with code. This is not the experience we want for people entering the Nix ecosystem, either as contributors or as users who desire these features.

I'm in favor of this getting merged. @roberth please respond y/n on your opinion about merging this PR as is with only style/code cleanups.

@roberth
Copy link
Member

roberth commented Mar 13, 2024

@Lunaphied please consider the point of view of the maintainers, who will be responsible for addressing issues that come up with this functionality, making sure that it's up to standards, responding to bug reports about why someone's repl doesn't have pkgs, complaints why the documentation doesn't mention pkgs in the repl, etc.
Nix already has a reputation for a steep learning curve. If you want bloatware, merging everything just because someone decide to write something is how you get bloatware.

@roberth please respond y/n on your opinion about merging this PR as is with only style/code cleanups.

Wow, I'd say you're more direct than the Dutch.
I've given my opinion, but I'm not the only Nix maintainer. I'll put this on the board for the team.

@roberth
Copy link
Member

roberth commented Mar 13, 2024

Considering that you seem to prefer not to discuss, we have a set of issues labeled "idea approved" that have been discussed by the team and should be ready for implementation. Of course if those issues are not what you want to work on, that's ok, but then I would recommend to ask about it ahead of time.
@9999years did write an issue for it, which is great. Unfortunately it wasn't discussed yet. This could be indicative that we don't have enough capacity to respond to issues (and especially issues that aren't bug reports, which might tell you something about why I think we should be conservative about features).

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 13, 2024

Just to make sure I understand the feature correctly: Can this be hacked together along these lines (except that currentSystem won't work with pure-eval)?

# with-overlays.nix
{ overlays ? [] }:
let
  info = { inherit (builtins) currentSystem; };
  applyOverlays = # ...
in
applyOverlays info {
  # base attrset goes here
} paths
nix repl with-overlays.nix --arg overlays '[ ./overlay1.nix ./overlay2.nix ]'

@Lunaphied
Copy link
Contributor

Just to make sure I understand the feature correctly: Can this be hacked together along these lines (except that currentSystem won't work with pure-eval)?

# with-overlays.nix
{ overlays ? [] }:
let
  info = { inherit (builtins) currentSystem; };
  applyOverlays = # ...
in
applyOverlays info {
  # base attrset goes here
} paths
nix repl with-overlays.nix --arg overlays '[ ./overlay1.nix ./overlay2.nix ]'

No, because the whole point is for something like nix repl alone to just work. But that is along the same idea as what this is trying to do if you want to think about "what is this functionally achieving". It's just making the UX into a state where it's usable by default and we give users choice over it.

@Lunaphied
Copy link
Contributor

@Lunaphied please consider the point of view of the maintainers, who will be responsible for addressing issues that come up with this functionality, making sure that it's up to standards, responding to bug reports about why someone's repl doesn't have pkgs, complaints why the documentation doesn't mention pkgs in the repl, etc. Nix already has a reputation for a steep learning curve. If you want bloatware, merging everything just because someone decide to write something is how you get bloatware.

I am well familiar with the difficulties and long-term consequences of adding and maintaining features, as well as Nix's learning curves. The problems you point out are ones that should be addressed by documentation and guidance, not by declining ideas that would improve Nix's overall UX.

@infinisil
Copy link
Member

I'm not on the Nix team myself, but I really don't think Nix is in a good position to accept more features right now. There's already hundreds of existing bugs, half of what Nix does is poorly documented, and new regressions are seemingly being introduced with every release. Nix is in a bad spot and adding more features makes it harder to get out of it.

My recommendation is to think about ideas how the Nix team can reduce their responsibilities. Concretely, this could mean helping with the C bindings and stabilising them, such that perhaps nix repl doesn't need to live in the Nix codebase anymore.

@9999years
Copy link
Contributor Author

@infinisil: There's already hundreds of existing bugs, half of what Nix does is poorly documented, and new regressions are seemingly being introduced with every release. Nix is in a bad spot and adding more features makes it harder to get out of it.

This position is understandable, but challenging to empathize with when PRs to add tests like #10075 are also rejected.

@9999years
Copy link
Contributor Author

9999years commented Mar 14, 2024

Also, if Nix is in a feature freeze, or even a feature freeze for contributors outside the Nix team, that should be documented. But as-is, this feels like a post-hoc justification for rejecting my PR. (If bugs were a concern, your comment would have more likely read "can we add tests to this PR?" to which I would have replied: sure, if we merge the PR to add a decent test framework for nix repl!)

@fricklerhandwerk
Copy link
Contributor

fricklerhandwerk commented Mar 14, 2024

@9999years there is no formal feature freeze, and certainly there is no deliberate bias towards or against a certain group of contributors. But we clearly express priorities for what is actively triaged and thus picked up into the review pipeline. New features are decidedly not on that list.

`legacyPackages.${info.currentSystem}` (if that attribute is defined):

```nix
info: prev: final:
Copy link
Member

Choose a reason for hiding this comment

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

Having three positional arguments, of which one is an attrset with fields like currentSystem, seems unnecessarily complicated. I would prefer having a single attrset argument, e.g.

{ prev, final, currentSystem, ... }: ...

This would make it easier to add more fields in the future.

default bindings to [`nix
repl`](@docroot@/command-ref/new-cli/nix3-repl.md) sessions.

Each file is called with three arguments:
Copy link
Member

Choose a reason for hiding this comment

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

The help text documents what the arguments are, but not what the function is supposed to return.

@@ -137,6 +137,42 @@ struct EvalSettings : Config

This is useful for debugging warnings in third-party Nix code.
)"};

PathsSetting replOverlays{this, Paths(), "repl-overlays",
Copy link
Member

Choose a reason for hiding this comment

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

In the interest of convention over configuration, it would be good if this had a default value like ~/.nix/config/repl.nix.

@tomberek
Copy link
Contributor

tomberek commented Apr 8, 2024

The team has not yet reached a conclusion about this. There is a discussion about various options and paths forward.

Specified in the flake

  # nixpkgs/flake.nix'
  {
    outputs = { self ,... }: {
      legacyPackages = ...;

      # (extraReplScope?)
      # This is merged into the repl scope, not the flake
      replScope = info: {
        pkgs = self.legacyPackages.${info.currentSystem};
      };
    }
  }

Allows a flake to set attributes for convenience. Other consequence is that this is not a user-configuration, but an author-provided one.

rc-script

  • @tomberek: A gdbrc-like script would be a nice feature
    • @roberth: Agree, but not preferably not as the first solution we merge for the packages.${currentSystem}/convenience use case
    • @roberth: Serves a different (valid!) use case, which is to support a specific activity with a repl

repl

As convenience, the repl can load [legacy]Pacakges.SYSTEM to top-level by default, to address this common need.

No more config

Additional configuration reduces reproducibility, less teaching, discoverability about how to use the repl, obfuscation about data the repl is representing. If we have a default file/location, similar to overlays, one will have to ask users "do you have anything in your ~/.config/nix/nix.repl that might be changing your results or something in your settings?" On the other hand, without a default, one would have to keep specifying the config, not much better UX than today.

@9999years the invite to come discuss this at a Nix meeting still stands.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-04-08-nix-team-meeting-136/42963/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Feature request or proposal fetching Networking with the outside (non-Nix) world repl The Read Eval Print Loop, "nix repl" command and debugger
Projects
Status: ⚖ To discuss
Development

Successfully merging this pull request may close these issues.

nix repl configuration file
9 participants