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

packages-config: refactor and simplification #113713

Merged
merged 1 commit into from
Aug 18, 2021

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Feb 19, 2021

cc @adisbladis @davidak

Motivation for this change

Fixes #110348

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Feb 19, 2021
@davidak
Copy link
Member

davidak commented Feb 21, 2021

I don't understand why this is needed at all.

Cant we just say: import any package from pkgs/

There is the thing that Nix don't know about packages. Would it help to add such a concept?

And even when there are good reasons why that don't work, we have all packages collected in pkgs/top-level/all-packages.nix, why do we have to list them here a second time?

It all feels like bad design. More complicated than needed, which causes errors like this over and over again.

Do you mind explaining it shortly for my understanding?

@Atemu
Copy link
Member Author

Atemu commented Feb 22, 2021

Nixpkgs is a tree, not a list; "import any package from pkgs/" isn't so simple. It's similar to listing all files in a FS; you have recurse into subfolders and aggregate a list of all the files you find there.
The problematic bit are these package subsets, not packages.

There is currently no way of knowing whether an attr of pkgs is a package or a subset of packages. (..or a function, broken or even evaluable, see #107539)

We need to tell Nix which attrs of pkgs are subsets and should be recursed into by setting set.recurseIntoAttrs on all of them which is precisely what this file does.
The problem we face is that we now have sub-subsets too which need to be declared somehow.

Now simply inherits attrsets from super directly instead of mapping getAttr over
a list of strings.

Filtering isn't really needed as non-existent subsets should just be removed
from this list when removing them from Nixpkgs (such an action warrants a
tree-wide search which should come across this file).

recurseIntoAttrs can simply be mapAttr'd now.

Some subsets have been removed because they no longer exist.

Sorted the list while I was at it.
@Atemu Atemu force-pushed the packages-config-cleanup branch from cb60727 to 9d8ac38 Compare July 17, 2021 03:49
@Atemu Atemu marked this pull request as ready for review July 17, 2021 03:50
@Atemu Atemu requested review from adisbladis and davidak July 17, 2021 03:51
@Atemu
Copy link
Member Author

Atemu commented Aug 9, 2021

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Aug 9, 2021

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot added marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2 needs_reviewer (old Marvin label, do not use) labels Aug 9, 2021
@marvin-mk2 marvin-mk2 bot requested a review from asymmetric August 9, 2021 12:23
@marvin-mk2 marvin-mk2 bot added awaiting_reviewer (old Marvin label, do not use) and removed needs_reviewer (old Marvin label, do not use) labels Aug 9, 2021
@marvin-mk2
Copy link

marvin-mk2 bot commented Aug 12, 2021

Reminder: Please review!

This Pull Request is awaiting review. If you are the assigned reviewer, please have a look. Try to find another reviewer if necessary. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be should be put back in the needs_reviewer queue in one day.


Note: This feature is currently broken. The bot will not actually change the status. If you see this message multiple times, please request a status change manually.

@davidak
Copy link
Member

davidak commented Aug 13, 2021

Result of nixpkgs-review pr 113713 run on x86_64-linux 1

@davidak
Copy link
Member

davidak commented Aug 13, 2021

I built packages.json using

[nix-shell:~/.cache/nixpkgs-review/pr-113713-1/nixpkgs]$ nix-build pkgs/top-level/release.nix -A tarball

and compared it to latest master https://hydra.nixos.org/build/149921732#tabs-summary

number of packages

PR 113713:

[davidak@gaming:~/Downloads]$ jq '.packages | length' packages.json
76881

master:

[davidak@gaming:~/Downloads]$ jq '.packages | length' packages.json
71674

diff

[davidak@gaming:~/Downloads]$ jq '.packages | keys' packages.json > packages.txt

[davidak@gaming:~/Downloads]$ jq '.packages | keys' packages-master.json > packages-master.txt

[davidak@gaming:~/Downloads]$ diff packages-master.txt packages.txt

https://gist.github.com/davidak/daca412ac5557e5fafa75726f241bbc9/edit

So this PR does not break the previous behavior and also adds emacs27Packages back. But that syntax is deprecated in #107152.

The previous versioned names emacsPackages have been moved to
aliases.nix and are now considered deprecated in favour of emacs
.pkgs.

cc @adisbladis

@Atemu
Copy link
Member Author

Atemu commented Aug 15, 2021

emacsPackages still exists as an alias.

@davidak
Copy link
Member

davidak commented Aug 15, 2021

emacsPackages have been moved to aliases.nix and are now considered deprecated in favour of emacs.pkgs.

so i think the packages should show up as emacs.pkgs.x in the search

see the documentation changes in https://github.com/NixOS/nixpkgs/pull/107152/files

@adisbladis maybe you can clarify?

@Atemu
Copy link
Member Author

Atemu commented Aug 17, 2021

I think we should merge this as-is until we find a better solution.

I tried recursing into emacs.pkgs but nix-env doesn't seem to want to recurse into a drv which seems sensible.

@davidak
Copy link
Member

davidak commented Aug 18, 2021

I agree. It looks cleaner (less code) and fixes an issue.

I can't evaluate if the code is good, but i tested it and it does what it should.

@davidak davidak merged commit b07140e into NixOS:master Aug 18, 2021
@Atemu Atemu deleted the packages-config-cleanup branch August 18, 2021 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux awaiting_reviewer (old Marvin label, do not use) marvin This PR was reviewed by Marvin, a discontinued bot: https://github.com/timokau/marvin-mk2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10081 packages disapeared from packages.json
3 participants