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

Add recurseOnHydra function #43670

Closed
wants to merge 4 commits into from

Conversation

matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Jul 17, 2018

Motivation for this change

This has similar behavior to recurseIntoAttrs. The main difference is
that nix-env -qa will not find the attributes. These attribute sets will be built
on Hydra but not listed on nix-env -qa. Most of the time this is what
is actually wanted. On the downside they become less discoverable
in nix-env -qa output.

This will significantly improve times to run ‘nix-env -qa’.

$ time nix-env -qa -f before >/dev/null
8.319 secs

$ time nix-env -qa -f after >/dev/null
3.805 secs

This has similar behavior to recurseIntoAttrs. The main difference is
that nix-env -qa is not affected. These attribute sets will be built
on Hydra but not listed on nix-env -qa. Most of the time this is what
is wanted.
This will significantly improve times to run ‘nix-env -qa’. Instead of
using recurseIntoAttrs we use recurseOnHydra. This is usually what is
desired.

$ time nix-env -qa -f before >/dev/null
8.319 secs

$ time nix-env -qa -f after >/dev/null
3.805 secs
@matthewbauer matthewbauer changed the title Recurse on hydra Add recurseOnHydra function Jul 17, 2018
recurseIntoAttrs only affects nix-env so it should not break anything.
These recurseIntoAttrs uses were causing duplicates see (NixOS#39563).
@dezgeg
Copy link
Contributor

dezgeg commented Jul 17, 2018

But that also means those packages are all undiscoverable now?

@matthewbauer
Copy link
Member Author

But that also means those packages are all undiscoverable now?

Yes but you can always do nix-env -qa -f . -A pythonDocs if you know the attribute name. There's definitely a tradeoff. If this were accepted we would probably want to add more things to package.json like was done here: NixOS/nixos-homepage#201

These either had no effect or didn’t make sense in the context.
Copy link
Member

@LnL7 LnL7 left a comment

Choose a reason for hiding this comment

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

I'm not sure if all this belongs in all-packages or release.nix.

/cc @grahamc Does this impact the ofborg evaluation checker?

@@ -45,7 +49,7 @@ with pkgs;
by Hydra.
'';

tests = recurseIntoAttrs (callPackages ../test {});
tests = callPackages ../test {};
Copy link
Member

Choose a reason for hiding this comment

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

does release.nix recurse these? they are referenced by the constituent jobsets so this would cause evaluation errors if that's not the case

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry my bad

Copy link
Member

Choose a reason for hiding this comment

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

it definitively shouldn't be recursed for nix-env tho, nobody is going to want to install cc-wrapper-test. 😄

@matthewbauer
Copy link
Member Author

matthewbauer commented Jul 17, 2018

Yeah I kind of like just putting it in release.nix now that I think about it.

I think there is definitely a case to be made for including these into nix-env -qa - I just thought this would be worth proposing after seeing the dramatic savings in nix-env -qa.

@LnL7
Copy link
Member

LnL7 commented Jul 17, 2018

We already hide some stuff and 8s is definitively too slow IMHO. What about adding something like a config.recurseLanguages allowing users to enable querying for language specific package sets if they prefer that behaviour and don't mind the performance.

@grahamc
Copy link
Member

grahamc commented Jul 18, 2018

Unfortunately this has broken evaluation, it seems to never terminate.
With this PR, running the following command with the file at https://github.com/NixOS/ofborg/blob/released/ofborg/src/outpaths.nix fails to ever finish evaluating, maybe check it out?

NIX_PATH=nixpkgs=. nix-env --query --available --no-name --attr-path --out-path --show-trace --option restrict-eval true --option build-timeout 3600 --argstr system x86_64-linux -f .gc-of-borg-outpaths.nix --arg checkMeta false

@grahamc
Copy link
Member

grahamc commented Jul 18, 2018

Note: a second commit in this series also has been evaluting "forever" due to the same problem:

image

@grahamc
Copy link
Member

grahamc commented Jul 18, 2018

My concern (beyond it breaking nix-env / ofborg) is now it isn't an easy list of packages which are hidden (oh you want haskell? here. oh, you want r? here.), but a massive list of packages. We haven't suitably solved the discoverability problem with the easy to remember list, and I think we should try to solve that before expanding the list of secret, hidden, hard to find, hard for new users, package list.

@FRidh
Copy link
Member

FRidh commented Jul 18, 2018

Most of the time this is what is actually wanted.

Sometimes, perhaps, but in case of the package sets that's not true; users want to be able to find packages.

This will significantly improve times to run ‘nix-env -qa’

Search times for users are improved with the nix search command which uses a cache.

But that also means those packages are all undiscoverable now?

Correct

@grahamc
Copy link
Member

grahamc commented Jul 18, 2018

11:59 does "all of nixpkgs" for ofborg include haskellPackages?

11:59 That's why I'm wondering if we could split it up
11:59 Like, everything sans haskellPackages, then just haskellPackages

an example of how the current situation is very easy to remember

@matthewbauer
Copy link
Member Author

Closing for now. I think a better approach is to slowly move recurseIntoAttrs logic into an overlay. This kind of information just kind of gets lost in all-packages.nix.

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

6 participants