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

Projects
None yet
5 participants
@roberth
Copy link
Contributor

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.

all-packages.nix: Alias self to res, deprecating self
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.

@roberth roberth requested review from Ericson2314 , matthewbauer and nbp as code owners Dec 2, 2018

@matthewbauer

This comment has been minimized.

Copy link
Member

matthewbauer commented Dec 2, 2018

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

@roberth

This comment has been minimized.

Copy link
Contributor Author

roberth commented Dec 2, 2018

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

@matthewbauer

This comment has been minimized.

Copy link
Member

matthewbauer commented Dec 2, 2018

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

@roberth

This comment has been minimized.

Copy link
Contributor 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

This comment has been minimized.

Copy link
Member

Ericson2314 commented Dec 3, 2018

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

This comment has been minimized.

Copy link
Contributor 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

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Dec 3, 2018

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

This comment has been minimized.

Copy link
Member

matthewbauer commented Dec 4, 2018

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

all-packages.nix: Fix reference to self warning
This reference was added to master while the deprecation PR NixOS#51401 was open.

@roberth roberth referenced this pull request Dec 4, 2018

Merged

all-packages.nix: Fix reference to self warning #51525

0 of 10 tasks complete

NeQuissimus added a commit that referenced this pull request Dec 5, 2018

all-packages.nix: Fix reference to self warning
This reference was added to master while the deprecation PR #51401 was open.

@roberth roberth deleted the roberth:all-packages-res branch Dec 6, 2018

@edolstra

This comment has been minimized.

Copy link
Member

edolstra commented Feb 3, 2019

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

@Ericson2314

This comment has been minimized.

Copy link
Member

Ericson2314 commented Feb 4, 2019

It's now been removed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.