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

Throw error for removed nix-repl attribute #45636

Merged
merged 1 commit into from Aug 30, 2018
Merged

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Aug 25, 2018

Motivation for this change

Implementation of my suggestion in #44903

Ping @edolstra @LnL7 @vcunat @samueldr

Related: #45571

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 infinisil changed the title Throw warning for nix-repl attribute Throw error for removed nix-repl attribute Aug 26, 2018
@samueldr
Copy link
Member

The way the conditional is used makes for a weird sentence, with weird capitalization.

Ugrade your Nix installation to a newer version and Use nix repl instead.

In my opinion, nobody would fault your for using if then else with two complete sentences instead of lib.optionalString in the message, instead of shoehorning a sentence into another one.

@infinisil
Copy link
Member Author

Using a , instead of a . now which fixes the capitalization and makes the sentence sound better.

@xeji
Copy link
Contributor

xeji commented Aug 26, 2018

👎 lt was removed. It's no longer maintained. People will notice. Maybe mention it in the release notes.
As an entire project, I think we're really bad at getting rid of old stuff. Adding more code to give hints about things that were removed goes in the wrong direction IMO.

Btw, we should really and finally drop nix 1 support in 18.09...

@infinisil
Copy link
Member Author

Deprecation is not something we can just gloss over. Every time I update my system with the latest nixos-unstable I need to go around fixing stuff that is now throwing errors. The errors this time were attribute 'Gconf3' missing and attribute 'nix-repl' missing. This is not good user experience, at all. We can't expect people to go through git log to find the commit that broke it and fix the error appropriately (which is what I had to do). Especially with something as widely included in configurations as nix-repl.

I actually started writing an RFC today for adding proper deprecation guidelines, which should hopefully bring removal of old code forward much faster.

@samueldr
Copy link
Member

samueldr commented Aug 26, 2018

@xeji nix 1.11 deprecation won't happen for 18.09 #37693 and is scheduled for 19.03.


And I am 👍 for better user experience. Ideally, I would prefer something semantically better than throw, but there is none in nixpkgs. In my opinion, simply dropping an attribute is what at my work would be a "help desk call situation", an update that is bound to cause confusion for the end-user.

@7c6f434c
Copy link
Member

Also, saying «no longer maintained» about software that has less annoyances than some of the new Nix 2.0 commands (I am comparing with the nix command, not with the package as a whole that includes working inherited tools) and that is normally used in a non-security-sensitive way (your Nixpkgs checkout is untrusted → you have already lost) doesn't sound like a strong argument in favour of anything. Globally, software maintenance is a problem to eventually remove, not a goal in itself, and Nix allows some pretty cheap workarounds for this problem.

Copy link
Member

@vcunat vcunat left a comment

Choose a reason for hiding this comment

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

I can see no real down-side, and UX is a bit better.

@vcunat vcunat mentioned this pull request Aug 30, 2018
9 tasks
@vcunat vcunat merged commit 99e663d into NixOS:master Aug 30, 2018
vcunat added a commit that referenced this pull request Aug 30, 2018
@infinisil infinisil deleted the add/nix-repl branch August 30, 2018 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants