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

Deprecate packageOverrides? #43266

Open
lheckemann opened this issue Jul 9, 2018 · 18 comments
Open

Deprecate packageOverrides? #43266

lheckemann opened this issue Jul 9, 2018 · 18 comments

Comments

@lheckemann
Copy link
Member

@lheckemann lheckemann commented Jul 9, 2018

packageOverrides is replaced by overlays as far as functionality goes, and has a number of disadvantages:

  • Learning/documentation overhead. We have 3 distinct things with the word override in them: packageOverrides, pkg.override introduced by callPackage, and pkg.overrideAttrs introduced by stdenv.mkDerivation. This is very confusing to beginners (it was to me as well, and we didn't have overlays at the time). Additionally, packageOverrides shares a space with overlays while being less powerful.
  • Unwieldy composition. I seem to recall there being some trick involving something like self: let super = self.pkgs; to get an approximation of what overlays provide, but it inrtoduces a fixpoint in each layer AFAIU. I personally don't know exactly how this works, but overlays are pretty clear in this respect.

For this reason, I think it would be nice to deprecate packageOverrides (remove it from docs in favour of overlays, add a warning when nixpkgs is evaluated with config.packageOverrides set) and eventually remove it completely, leaving perhaps only an explanatory error message. The warning/error messages should explain how to turn a packageOverrides function into an overlay to ease use of existing third-party documentation.

Any opinions on whether this is a good idea at all, further practical considerations, or any other relevant commentary welcome!

@grahamc
Copy link
Member

@grahamc grahamc commented Jul 10, 2018

cc @nbp

Loading

@nbp
Copy link
Member

@nbp nbp commented Jul 12, 2018

If I do not make any mistake this should almost be a backward compatible overlay for adding the feature back:

self: super:

(super.config.packageOverrides or (super: {})) super

The remaining issue is about modifying any bootstrap packages. This should be fixed by including all the stages of the bootstrap as attribute names within the main fix-point.

Loading

@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Jul 15, 2018

Given the current implementation of packageOverrides, yes, I think that should work :D

Loading

@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Jul 15, 2018

Opened PR #43560 to implement this.

Loading

@jcrben
Copy link
Contributor

@jcrben jcrben commented Jul 23, 2018

Overlays seem tricky to use correctly. Therefore I think deprecation at this point could on net cause more UX damage than non-deprecation. I'm thinking about the long-term user I met at a meetup a couple months ago who hadn't migrated yet. 🤔

Right now, they aren't showing up for me on my mac in nix-env -q (despite adding the directory to my NIX_PATH; originally was using single file approach) altho I can install them OK, including with nix-shell or nix-build (might be better on NixOS machine). (prolly just user error related to a package not having the proper attributes (more at https://gist.github.com/LnL7/570349866bb69467d0caf5cb175faa74#gistcomment-2655668))

Looks like #26487 (comment) ran into similar search problems altho I don't understand the fix. Admittedly search isn't absolutely necessary since I can read the overlay files. And #25264 has a long discussion about how to get overlays to be picked up by tools, but I don't see any sign of commits or docs to streamline the situation. Altho I guess https://nixos.wiki/wiki/Overlays has most of it?

Maybe hardening overlays and triaging / resolving the open issues on them (e.g., #24907 #28558 ) before deprecating?

And the existing docs on overlays could maybe be better as there are nuances that could to be called out a bit more strongly, e.g. single file needs to return a list while overlays in a directory are functions.

Loading

@ckauhaus
Copy link
Contributor

@ckauhaus ckauhaus commented Jul 23, 2018

I've converted an overlay to a packageOverrides construct lately because the overlay took too much memory to evaluate. It seems that a complete pkgs attrset mut be constructed on each fixpoint iteration. packageOverrides are more compact on memory usage exactly because they involve a local fixpoint.

This is by no way meant to stop overlays, but it should be taken into consideration before deprecating packageOverrides too early.

Loading

@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Jul 23, 2018

packageOverrides is implemented as a stage of nixpkgs which behaves just like an overlay, so I'm not sure I understand how this makes a difference.

Although at this point it almost seems to me like removing overlays again might be better 🙈

Loading

@ckauhaus
Copy link
Contributor

@ckauhaus ckauhaus commented Jul 24, 2018

I got random GC errors like "out of memory" or "too many heap sections" while using overlays (something similar to #40456). After converting overlays to packageOverrides, the errors disappeared. Actually no idea if this was just luck or a real solution.

Loading

@asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Dec 21, 2018

I currently use packageOverrides to declare a set of packages in my ~/.config/nixpkgs/config.nix (explained here).

How would I do that - declaratively - with overlays? i.e. I don't want to have to run nix-env for each package in the overlay, but rather have a "meta-package" of sorts that bundles all the packages I want.

Loading

@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Dec 21, 2018

@asymmetric As documented in the warning in my PR https://github.com/NixOS/nixpkgs/pull/43560/files#diff-69df24a05ec3d577cb638113e9837ae1R119 :
take only the value of packageOverrides, and replace pkgs: with pkgs; with self: super: with self;, then put it in a .nix file in ~/.config/nixpkgs/overlays.

Loading

@stale
Copy link

@stale stale bot commented Jun 3, 2020

Thank you for your contributions.

This has been automatically marked as stale because it has had no activity for 180 days.

If this is still important to you, we ask that you leave a comment below. Your comment can be as simple as "still important to me". This lets people see that at least one person still cares about this. Someone will have to do this at most twice a year if there is no other activity.

Here are suggestions that might help resolve this more quickly:

  1. Search for maintainers and people that previously touched the related code and @ mention them in a comment.
  2. Ask on the NixOS Discourse.
  3. Ask on the #nixos channel on irc.freenode.net.

Loading

@stale stale bot added the 2.status: stale label Jun 3, 2020
@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Jun 3, 2020

As mentioned on the issue, will do an RFC eventually

Loading

@stale stale bot removed the 2.status: stale label Jun 3, 2020
@stale
Copy link

@stale stale bot commented Nov 30, 2020

I marked this as stale due to inactivity. → More info

Loading

@ShamrockLee
Copy link
Contributor

@ShamrockLee ShamrockLee commented Feb 8, 2021

Is this still revalent?

Loading

@stale stale bot removed the 2.status: stale label Feb 8, 2021
@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Feb 10, 2021

Yes

Loading

@asymmetric
Copy link
Contributor

@asymmetric asymmetric commented Mar 8, 2021

@lheckemann if this is still relevant (which I think it is), then maybe it should be turned into an RFC. I don't think @edolstra's mild opposition to this should stop you from writing it, for the record :)

Otherwise, it should be left stale and closed, eventually.

Loading

@lheckemann
Copy link
Member Author

@lheckemann lheckemann commented Mar 8, 2021

As mentioned on the issue, will do an RFC eventually

for some value of eventually…

Loading

@stale
Copy link

@stale stale bot commented Sep 6, 2021

I marked this as stale due to inactivity. → More info

Loading

@stale stale bot added the 2.status: stale label Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
7 participants