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

all-packages.nix: Alias self to res, deprecating self #51401

Merged
merged 1 commit into from Dec 3, 2018

Conversation

roberth
Copy link
Member

@roberth roberth commented Dec 2, 2018

For historical reasons, self in all-packages.nix was ill-named. This removes its usages from all-packages.nix and provides a deprecation message for those who use a patched Nixpkgs.

Some packages seem to depend on the peculiarities of res, as can be seen by making res into an alias of pkgs (normally self). The super variable doesn't have all attributes that are needed.

Therefore the simple fix is not guaranteed to work and as such,
usages of res need to be changed to pkgs or super, case by case.

Motivation for this change

#34881

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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

For historical reasons, self was ill-named. This removes its usages
from all-packages.nix and provides a deprecation message for those
who use a patched Nixpkgs.

Some packages seem to depend on the peculiarities of res, as can
be seen by making res into an alias of pkgs (normally "self").
The super variable doesn't have all that is needed.

Therefore the simple fix is not guaranteed to work and as such,
usages of res need to be changed to pkgs or super, case by case.
@matthewbauer
Copy link
Member

Since we use rec here i think we could just get rid of res.

@roberth
Copy link
Member Author

roberth commented Dec 2, 2018

@matthewbauer I don't see a rec keyword. Did I miss something?

@matthewbauer
Copy link
Member

Oh maybe not then! with pkgs; here does a similar thing. It's used inconsistently though.

@roberth
Copy link
Member Author

roberth commented Dec 2, 2018

So we have with self, as it's usually named. That's like some class-based object oriented languages.
I don't think we should do that here, but that's a separate issue.

@Ericson2314
Copy link
Member

Can we just go straight to pkgs: super:? I can't imagine why the self/res to pkgs switch would cause problems in any of those cases.

@roberth
Copy link
Member Author

roberth commented Dec 3, 2018

@Ericson2314 When switching, some hashes change, so it's not guaranteed to work.
Even though I don't expect things to break, it's still good to separate the concerns.
This pr only fixes the naming. I suppose a follow-up pr/prs can improve the way these packages are structured.

@Ericson2314 Ericson2314 merged commit 805d32d into NixOS:master Dec 3, 2018
@Ericson2314
Copy link
Member

Fair enough. If you want to make the next one removing all the res for self, and ofborg can spit out the changed hashes, I'll happily triage them.

@matthewbauer
Copy link
Member

The warning here breaks tarball:

https://hydra.nixos.org/build/85466401/nixlog/1/tail

roberth added a commit to roberth/nixpkgs that referenced this pull request Dec 4, 2018
This reference was added to master while the deprecation PR NixOS#51401 was open.
NeQuissimus pushed a commit that referenced this pull request Dec 5, 2018
This reference was added to master while the deprecation PR #51401 was open.
@roberth roberth deleted the all-packages-res branch December 6, 2018 10:34
@edolstra
Copy link
Member

edolstra commented Feb 3, 2019

IMHO, res is even vaguer than self. It should be final or something like that.

@Ericson2314
Copy link
Member

It's now been removed!

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

5 participants