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.

Sign up for GitHub

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

Jump to bottom

Revert "nix-repl: Remove" #44903

Closed
wants to merge 1 commit into from
Closed

Revert "nix-repl: Remove" #44903

wants to merge 1 commit into from

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 11, 2018 •
edited

This reverts commit 490ca6a.

The minimum required Nix version for nixpkgs is still only 1.11, which
means we can't expect people to have nix repl available. Removing
nix-repl therefore removes potentially the only nix-repl available to
some people.

We should at least wait until the minimum version is bumped to 2.0, which might only happen in 19.03, see #37693. And then it should be a warning in the form of

{
  # ...
  nix-repl = throw "nix-repl has been retired for nix repl";
  # ...
}

Ping @edolstra

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@infinisil
Revert "nix-repl: Remove"
5f5ae2b 
This reverts commit 490ca6a.

The minimum required Nix version for nixpkgs is still only 1.11, which
means we can't expect people to have `nix repl` available. Removing
nix-repl therefore removes potentially the only nix-repl available to
some people.
LnL7
LnL7 approved these changes Aug 11, 2018
View reviewed changes
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

I still use nix-repl from time to time to test 1.11 compatibility.

@vcunat
Copy link
Member

vcunat commented Aug 11, 2018

Maybe call it nix1-repl or something to decrease the possible confusion?

@infinisil
Copy link
Member Author

@vcunat This doesn't have much to do with this change though (and just renaming it will still break everybody's nix files that refer to nix-repl)

@edolstra Can you elaborate why you are against this revert?

@edolstra
Copy link
Member

Because nix-repl is no longer maintained.

@infinisil
Copy link
Member Author

Lots of nixpkgs packages are not maintained either and they too don't get removed in order to not breaks peoples workflows and configs. I don't see why nix-repl would be an exception to that.

Also see PR description for another reason not to remove it.

@samueldr
Copy link
Member

samueldr commented Aug 15, 2018 •
edited

Nix-pills and manual entriesgoogle search will need to be fixed too.

@dezgeg
Copy link
Contributor

dezgeg commented Aug 15, 2018

Maybe we can replace the nix-repl binary with a script that just warns about the deprecation and then runs nix repl.

@vcunat
Copy link
Member

vcunat commented Aug 15, 2018

It was my point to break those expressions. In default install nix-repl doesn't work, because you have nix2 daemon.

@infinisil
Copy link
Member Author

Can we merge this? This change is in unstable now, and I'm getting

error: undefined variable 'nix-repl' at /home/infinisil/cfg/config/new-modules/console.nix:35:7
(use '--show-trace' to show detailed location information)
error: unable to build all machine configurations

Now because I know what's going on and know that there's nix repl too, I know what to do, but I doubt that's the case for everybody.

I can also open a PR to change it to a throw if that's preferred

infinisil added a commit to infinisil/system that referenced this pull request Aug 24, 2018
@infinisil
nix-repl has been removed..
e5326f7 
See NixOS/nixpkgs#44903
@infinisil infinisil mentioned this pull request Aug 25, 2018
Merged
9 tasks
@vcunat
Copy link
Member

vcunat commented Aug 30, 2018

I consider this resolved by #45636.

@vcunat vcunat closed this Aug 30, 2018
@infinisil infinisil deleted the revert/nix-repl-removal branch August 30, 2018 13:21
@sordina sordina mentioned this pull request Aug 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LnL7 LnL7 LnL7 approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@infinisil @vcunat @edolstra @samueldr @dezgeg @LnL7 @GrahamcOfBorg