nixos: doc: implement related packages in the manual (again) #33898
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Well, it seems to work on my hacky cross-compiled aarch64 stdenv. Ping @grahamc again because you was involved in 3 out of 4 related PRs. |
grahamc
reviewed
Jan 19, 2018
| reverseList listDfs toposort sort take drop sublist last init | ||
| crossLists unique intersectLists subtractLists | ||
| reverseList listDfs toposort sort compareLists take drop sublist | ||
| last init crossLists unique intersectLists subtractLists |
There was a problem hiding this comment.
One of the intentions behind making the inherits explicit here is so we don't need to make all the lib functions available in the top level, but instead say lib.lists.compareLists. If this seems like a reasonable thing to do, would you like to not inherit them here? It could be we're just not ready for that yet, seeing as I don't think many functions aren't exposed at the top level here.
grahamc
reviewed
Jan 19, 2018
lib/trivial.nix
Outdated
| compare elements of the same subtype with `yes` and `no` | ||
| comparisons respectively. | ||
| */ | ||
| splitByAndCompare = p: yes: no: a: b: |
There was a problem hiding this comment.
Please provide a usage example for these functions.
grahamc
reviewed
Jan 19, 2018
nixos/doc/manual/default.nix
Outdated
| # Convert the list of options into an XML file. | ||
| optionsXML = builtins.toFile "options.xml" (builtins.toXML optionsList'); | ||
| optionsXML = builtins.toFile "options.xml" (builtins.toXML optionsList''); |
There was a problem hiding this comment.
I'm worried about the '' being confusing here, since it is identical to otherwise valid Nix syntax. Is there a better variable name you can imagine?
|
Will |
|
One of the intentions behind making the inherits explicit here is so we don't need to make all the lib functions available in the top level, but instead say `lib.lists.compareLists`. If this seems like a reasonable thing to do, would you like to not `inherit` them here? It could be we're just not ready for that yet, seeing as I don't think many functions aren't exposed at the top level here.
Hm. I interpret those things differently. I though the intention of doing it this way was that you should inherit anything generic and not inherit things that are unlikely to be used out of the lib. Those new functions are pretty generic as proved by their usage out of the lib.
Are you sure you want to (eventually) soil every nix module with a bunch of `with lib.<something>` at the top (like C++ does with `use <something>`)?
Why isn't `with lib` enough? I would prefer `lib` to be like Haskell's `Prelude` that gives the cohesive set of things you most likely want by default.
Will `relatedPackages` cover cases like `python3Packages.requests`?
Not as a string because I didn't want to deal with parsing attributes (and the way `rename.nix` does it is super-ugly).
You can specify `{ name = "python3Packages.requests"; package = pkgs.python3Packages.requests; }`, though.
However, its is a valid question whenever something better can be invented, like, say, allowing both simple strings and ugly lists of `rename.nix`. I'll think about it.
What will happen if a relatedPackage is deleted, and the reference is left dangling?
An error. I think that's a good thing. You are not supposed to delete packages that are referenced in documentation.
Please provide a usage example for these functions.
The code I introduced next does provide it, but, okay, will fix.
Also, is this really trivial?
Well, I know some people fear formulas, but it is the best way I have to describe what it does without giving in-words description of the actual implementation. Feel free to propose a better one.
I'm worried about the `''` being confusing here, since it is identical to otherwise valid Nix syntax. Is there a better variable name you can imagine?
Will fix.
|
|
I agree with everything you said, sounds great! I look forward to the few remaining fixes :) |
oxij
force-pushed
the
nixos/related-packages-v5
branch
from
995241d
to
68197ce
Compare
oxij
force-pushed
the
nixos/related-packages-v5
branch
2 times, most recently
from
08bb21b
to
ed980aa
Compare
|
Done.
|
…f their option group Why? Because this way configuration.nix(5) can be read linearly. Before: > virtualisation.xen.bootParams > ... > virtualisation.xen.enable > ... > virtualisation.xen.package > ... After: > virtualisation.xen.enable > virtualisation.xen.package > virtualisation.xen.bootParams > ...
This allows one to specify "related packages" in NixOS that get rendered into the configuration.nix(5) man page. The interface philosophy is pretty much stolen from TeX bibliography. See the next several commits for examples.
This is a trivial example of `relatedPackages` option usage.
This is an attribute path example of `relatedPackages` option usage.
This is a custom attribute set example of `relatedPackages` option usage.
oxij
force-pushed
the
nixos/related-packages-v5
branch
from
ed980aa
to
0d1a643
Compare
|
ping? Just as I said, its completely finished now. |
|
😻 |
|
Right :) Thanks! |
LnL7
reviewed
Feb 11, 2018
| in lib.compareLists cmp (splt a) (splt b) < 0; | ||
|
|
||
| # Customly sort option list for the man page. | ||
| optionsList = lib.sort (a: b: optionListLess a.name b.name) optionsListDesc; |
There was a problem hiding this comment.
this is the cause of the slowdown, the cost of sorting all of the options happens during every evaluation.
|
How do I profile this in nix?
How are you sure that the problem is with the whole sort and not with
recomputation of `hasPrefix` or with `compareLists`, for instance?
Normally I would imagine that sorting shouldn't introduce too much of a
problem.
I would guess the problem is that `optionListLess` is very inefficient
when evaluated as written. Can I ask nix to partially evaluate some
code? `cmp` can be partially evaluated into a much cheaper function.
I assume this evaluation time spike influences latency of channels, so
should I make an option to disable this sorting for now?
Which documentation nixos.org displays? From a latest succeeding master,
stable, something else? I would like to keep this sorting for all
publicly visible documentation, because its awesome.
|
|
I used this and only checked the sort, it might be not be the sort itself but some other part. |
|
Moving this sorting to the build should work
Please elaborate, I don't understand what do you mean by that.
|
|
Instead of sorting this in nix at evaluation time, it could be done in the docbook build or some other intermediate drv. Then the sorting only happens once and gets cached as part of the manual/manpages build. |
|
https://nixos.org/nixos/manual/ shows the latest stable channel. |
To be more explicit, it's basically a channel blocker for nixos-unstable ATM, as Hydra almost always runs out of memory during evaluation. Unless we can find a solution very fast, I'd temporarily revert the problematic part until we find a better way. |
|
To be more explicit, it's basically a channel blocker for nixos-unstable ATM
Ok, this now has highest priority. Will either fix or add an option in the next 12 hours.
|
|
Ok, so.
On master (ad78e52) with offending patches applied:
nixos.smallContainer.time 7.811 s
nixos.smallContainer.maxresident 520740 KiB
nixos.smallContainer.allocations 1008637960 B
nixos.smallContainer.values 21474162
nixos.kde.time 12.343 s
nixos.kde.maxresident 527164 KiB
nixos.kde.allocations 1176678544 B
nixos.kde.values 23892286
nixos.lapp.time 8.115 s
nixos.lapp.maxresident 521496 KiB
nixos.lapp.allocations 1019577072 B
nixos.lapp.values 21652224
With a7d75ab reverted (which is untrivial):
nixos.smallContainer.time 2.34 s
nixos.smallContainer.maxresident 440616 KiB
nixos.smallContainer.allocations 262088816 B
nixos.smallContainer.values 4250508
nixos.kde.time 4.52 s
nixos.kde.maxresident 525820 KiB
nixos.kde.allocations 430129400 B
nixos.kde.values 6668632
nixos.lapp.time 2.372 s
nixos.lapp.maxresident 456340 KiB
nixos.lapp.allocations 273027928 B
nixos.lapp.values 4428570
I get almost exactly the same results by replacing the comparison function with `a: b: false`.
So the problem is not with sorting, but with inefficient comparisons.
With some trivial patches of `os/doc/efficient-comparisons` branch (https://github.com/oxij/nixpkgs/commits/os/doc/efficient-comparisons) I get:
nixos.smallContainer.time 3.294 s
nixos.smallContainer.maxresident 517396 KiB
nixos.smallContainer.allocations 365512360 B
nixos.smallContainer.values 6548134
nixos.kde.time 5.367 s
nixos.kde.maxresident 530352 KiB
nixos.kde.allocations 533552944 B
nixos.kde.values 8966258
nixos.lapp.time 3.178 s
nixos.lapp.maxresident 518004 KiB
nixos.lapp.allocations 376451472 B
nixos.lapp.values 6726196
The patches are pretty trivial and the comparison function stays the same.
So, in the light of that, the question.
a7d75ab costs ~3.5-4x in time and mallocs. This is, indeed, too much.
`os/doc/efficient-comparisons` now costs ~1.3-1.5x in time and mallocs without losing any code quality.
Is it still too much?
Should I butcher the understendability of `optionListLess` function in favor of more optimizations?
In theory, the sky is the limit here, since `builtins.sort` doesn't influence the evaluation much.
(Also, please don't merge `os/doc/efficient-comparisons` without advance notice, its WIP.)
|
|
Well, after many more experiments it doesn't get much better (-0.15s) than what `os/doc/efficient-comparisons` already had even with some advanced memoization hackery and hand-made common subexpression elimination. Random jitter thwarts all my efforts.
I can make it evaluate in 2.7s instead of 3.1s (which is statistically significant) with some minor loss of ordering (because `builins.sort` seems to be implemented in an unlucky for my hackery way), but I really prefer not to, because that unordering will get worse as more modules get added to nixos.
Lets merge `os/doc/efficient-comparisons` and then see if you still want an option to disable sorting.
Pushing sorting to docbook generation is very untrivial.
|
3 tasks
|
PR for ofborg tests #34866. |
|
Thanks for the quick reaction ❤️ |
vcunat
added a commit
that referenced
this pull request
Feb 12, 2018
This solves most of the performance regression from a7d75ab (#33898).
|
I only get "related packages" for only 2 options in the entire manpage. Namely |
|
I only get "related packages" for only 2 options in the entire manpage. Namely `tmux` and `xen`. Is this expected behaviour?
Well, if somebody else (i.e. not me) would add more of those we would have more of them. Duh.
Meanwhile, I plan to add some more of those automatically by adding `relatedPackages` to things that are `types.package`. Having all real services have `.package` of `types.package` type seems like a good idea to me, but that requires a bunch of tiresome manual work I don't like doing.
|
|
Aah good to know. I'll see if I can sink in some boring time.
…On Sun, Sep 23, 2018, 15:47 Jan Malakhovski ***@***.***> wrote:
> I only get "related packages" for only 2 options in the entire manpage.
Namely `tmux` and `xen`. Is this expected behaviour?
Well, if somebody else (i.e. not me) would add more of those we would have
more of them. Duh.
Meanwhile, I plan to add some more of those automatically by adding
`relatedPackages` to things that are `types.package`. Having all real
services have `.package` of `types.package` type seems like a good idea to
me, but that requires a bunch of tiresome manual work I don't like doing.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#33898 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAmWo9-K-XRpv39T0dxlfDNF_4tIYBRoks5ud5DsgaJpZM4ReUY2>
.
|
12 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is this?
#32424 reverted by #33006 brought back to you with laziness and
meta.evaluatesof #33057.Closes #33007.
Things done
configuration.nix(5)manual builds on x86_64-linux.configuration.nix(5)manual builds on aarch64-linux.Please check the last item with OfBorg before merging, I'm too lazy to rebuild aarch64 stdenv just to test this.
/cc @grahamc @vcunat