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.

Sign up for GitHub

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

Jump to bottom

tarball: backport nixos-search's subsets for JSON generation #105857

Merged
merged 2 commits into from
Dec 16, 2020
Merged

tarball: backport nixos-search's subsets for JSON generation #105857

merged 2 commits into from
Dec 16, 2020

Conversation

Atemu
Copy link
Member

@Atemu Atemu commented Dec 4, 2020 •
edited
Loading

Motivation for this change

Progress towards #102508

More limited scope than #102509

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.

@Atemu Atemu requested review from edolstra and garbas December 4, 2020 09:01
@Atemu Atemu changed the title tarball: backport nixos search subsets for JSON generation tarball: backport nixos-search's subsets for JSON generation Dec 4, 2020
@Atemu Atemu added the 6.topic: repology https://repology.org/ label Dec 4, 2020
@Atemu
Copy link
Member Author

Atemu commented Dec 4, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 4, 2020

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 Dec 4, 2020
@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 Dec 4, 2020
@SuperSandro2000
Copy link
Member

Is this going to influence the speed of the bash-completion script when doing nix-build -A Tab?

@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 Dec 4, 2020
@edolstra
Copy link
Member

edolstra commented Dec 4, 2020

@SuperSandro2000 The evaluation cache in Nix master speeds up tab completion, e.g. nix build nixpkgs#firef<tab>.

@Atemu
Copy link
Member Author

Atemu commented Dec 4, 2020

I don't think this would affect bash completion for nix-build -A at all as that simply evaluates the specified Nix file itself.

AFAICT, this file is only used in the build script of pkgs/top-level/make-tarball.nix and the only user of this expression's output I know of is Repology.

nix search might be using it but it's pretty fast and the amount of packages in the JSON "only" increased by ~13%; if something were to use it, the slowdown would not be all that significant.

@Atemu
Copy link
Member Author

Atemu commented Dec 4, 2020

/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added needs_reviewer (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) labels Dec 4, 2020
@lukegb
Copy link
Contributor

lukegb commented Dec 5, 2020

A few questions:

  • How was the list generated - I'm assuming by just looking for dontRecurseIntoAttrs in top-level/all-packages.nix - and can we add a comment indicating how to regenerate it if need be? (I guess that new packagesets like this aren't added particularly frequently)
  • Does it matter if e.g. nodePackages and nodePackages_latest are both present (and emacs{26,27}Packages)? Do we need both (I assume yes because some packages might be disabled under older/newer versions of their respective runtime)?

@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 Dec 5, 2020
@Atemu
Copy link
Member Author

Atemu commented Dec 5, 2020

How was the list generated

I don't think it was generated. It looks like it was picked out manually.

If we add another larger subset of packages, we should just manually update the list. That's a rare enough of an occurance to not cause too much trouble I think.

Does it matter if e.g. nodePackages and nodePackages_latest are both present

For Repology, it doesn't matter. They take care of duplicates etc.

I don't know whether there are other users but I'm pretty sure there aren't. I the only thing I can think of that could possibly be using it is nix search but I'd have to check.

If there are users who can't deal with duplicates we could either just export the latest versions or export the longer list as a separate JSON (maybe even the nearly complete list).

I assume yes because some packages might be disabled under older/newer versions of their respective runtime

Yeah that's what I was thinking too. Although, the set of packages not in both sets probably isn't too large in most cases.

@lukegb
Copy link
Contributor

lukegb commented Dec 5, 2020

/status needs_merger

@marvin-mk2 marvin-mk2 bot added needs_merger (old Marvin label, do not use) and removed awaiting_reviewer (old Marvin label, do not use) labels Dec 5, 2020
@marvin-mk2 marvin-mk2 bot requested a review from timokau December 5, 2020 10:21
@marvin-mk2 marvin-mk2 bot removed the needs_merger (old Marvin label, do not use) label Dec 5, 2020
@marvin-mk2 marvin-mk2 bot added the awaiting_merger (old Marvin label, do not use) label Dec 5, 2020
@timokau
Copy link
Member

timokau commented Dec 5, 2020

@veprbl, you added a 😕 reaction to #102509. Is there a reason for that?

@veprbl veprbl mentioned this pull request Dec 6, 2020
Closed
10 tasks
@garbas
Copy link
Member

garbas commented Dec 7, 2020

I'm a bit -0 on this one, since it does not addresses the real problem. Somebody will have to keep this list manually up to date. It would be nicer if this list would not be even needed. But I understand that this solved the problem currently, therefore I'm not against it, but would like to challenge that the problem to be addressed at the root.

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 10, 2020

Reminder: Please review!

Reminder: This Pull Request is awaiting merger. If you are the assigned reviewer with commit permission, please have a look. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

@timokau
Copy link
Member

timokau commented Dec 12, 2020

Since this discussion has stalled a bit and it seems like nobody is working on a better solution, I think something is better than nothing for now. So I'm calling "FCP with predisposition to merge" now. Unless somebody disagrees, I'll merge on Wednesday.

@Atemu
Copy link
Member Author

Atemu commented Dec 13, 2020

The only thing I was worried about being affected would be nix search but it seems like that generates its own json.

@marvin-mk2
Copy link

marvin-mk2 bot commented Dec 16, 2020

Reminder: Please review!

Reminder: This Pull Request is awaiting merger. If you are the assigned reviewer with commit permission, please have a look. If you can't, please say so. If the status is not accurate, please change it. If nothing happens, this PR will be put back in the needs_reviewer queue in one day.

timokau
timokau approved these changes Dec 16, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @Atemu!

@timokau timokau merged commit 6dc8af7 into NixOS:master Dec 16, 2020
@Atemu Atemu deleted the tarball-json-backport-nixos-search-subsets branch December 16, 2020 13:21
@Atemu
Copy link
Member Author

Atemu commented Dec 24, 2020

:o

image

@Atemu
Copy link
Member Author

Atemu commented Dec 24, 2020

Issue on fixing this properly: #107539

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nixpkgs-has-been-the-largest-repository-for-months/10667/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukegb lukegb lukegb approved these changes

@timokau timokau timokau approved these changes

@edolstra edolstra Awaiting requested review from edolstra

@garbas garbas Awaiting requested review from garbas

Assignees
No one assigned
Labels
6.topic: repology https://repology.org/ 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_merger (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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Atemu @SuperSandro2000 @edolstra @lukegb @timokau @garbas @nixos-discourse