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

nixos systemPackages: rework default outputs #12653

Merged
merged 5 commits into from Apr 7, 2016

Conversation

vcunat
Copy link
Member

@vcunat vcunat commented Jan 28, 2016

  • Now pkg.outputUnspecified = true but this attribute is missing in
    every output, so we can recognize whether the user chose or not.
    If (s)he didn't choose, we put pkg.bin or pkg.out or pkg into
    systemPackages.
  • outputsToLink is replaced by extraOutputsToLink.
    We add extra outputs regardless of whether the user chose anything. (EDIT: this is why I didn't consider 7410c8f good enough.)
    It's mainly meant for outputs with docs and debug symbols.
  • Note that as a result, some libraries will disappear from system path.

/cc people who touched these things: @edolstra, @lethalman, @dezgeg. Comments are welcome! I expect to merge in a few days.

- Now `pkg.outputUnspecified = true` but this attribute is missing in
  every output, so we can recognize whether the user chose or not.
  If (s)he didn't choose, we put `pkg.bin or pkg.out or pkg` into
  `systemPackages`.
- `outputsToLink` is replaced by `extraOutputsToLink`.
  We add extra outputs *regardless* of whether the user chose anything.
  It's mainly meant for outputs with docs and debug symbols.
- Note that as a result, some libraries will disappear from system path.
@mention-bot
Copy link

By analyzing the blame information on this pull request, we identified @lethalman, @edolstra and @shlevy to be potential reviewers

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

For reference, how we handle the outputs:

  • For build inputs we take the default output (typically "dev") extended by propagating some others. That seems to work very well.
  • For string references the default output is chosen. That's suboptimal, typically. The output should be always specified and we could do eval-time checks, but that would imply too much rewriting. In the meantime we might use buildEnv of some outputs, e.g. those that are propagated into build inputs by default.
  • For systemPackages: minimalistic variant pkg.bin or pkg.out or pkg proposed in this PR.
  • For nix-env: currently it looks at the outputs attribute and installs all those. (If it doesn't exist, it installs just what is passed.) We might better make it configurable in future, perhaps similarly to systemPackages. Note that consequently even nix-env -iA pkg.out will install all outputs. We might avoid this e.g. by removing outputs attribute from every output.

@edolstra
Copy link
Member

What is outputUnspecified?

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

Now pkg.outputUnspecified = true but this attribute is missing in every output

I needed a way to differentiate between [ myPackage ] and [ myPackage.dev ] so I put an attribute into the first one.

@shlevy
Copy link
Member

shlevy commented Jan 28, 2016

@vcunat Why can't you just check outputName?

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

@shlevy: Checking outputName isn't enough, as the default output has the same name as one of the "choosable" outputs. I originally wanted to change outputName instead of introducing outputUnspecified, but that had adverse effects on nix-env.

@shlevy
Copy link
Member

shlevy commented Jan 28, 2016

Ah, I see. Can you give a specific use case where we need to distinguish "this is the first output by default" vs "this is the first output by selection"?

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

Hmm, maybe outputName would be enough after carrying on with the plan (above) that without choosing you'd get a buildEnv of all outputs.

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

@shlevy: example directly in this PR: https://github.com/NixOS/nixpkgs/pull/12653/files#diff-dcba170e079cd72958889058d01a4a02R128 I want to choose some defaults for systemPackages but only if the code didn't specify the output explicitly.

@shlevy
Copy link
Member

shlevy commented Jan 28, 2016

Hm, I see. I guess I think it would be better to just settle on the convention that the first output is the one we want in user envs, or install all outputs.

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

Well, the first one is what is put into build inputs, and typically that's different from what you want in user envs (e.g. headers).

@shlevy
Copy link
Member

shlevy commented Jan 28, 2016

Right, but I think the simple path should be the one that works for users, not devs. If anyone has to specify, it should be people writing build expressions, not people just trying to get their system working.

@vcunat
Copy link
Member Author

vcunat commented Jan 28, 2016

Well, string references are the largest problem here, as that's a "simple path" usage mostly used by devs. Of course, now that we can override conversion to strings, it is possible to hack around that, but I'm still leaning to make the "simple path" refer to a buildEnv of all outputs – that seems a nice backward-compatible layer suitable for both devs and users.

inherit (config.environment) pathsToLink;
# The default output probably shouldn't be globally configurable.
# Services and users should specify them explicitly unless they want this default.
map (p: if p.outputUnspecified or false then p.bin or p.out or p else p)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be factored out into a separate function, e.g. getOutputsToInstall or something like that, so it can be re-used in places like the handling of systemd.services.<unit>.path.

This should probably also be overridable via a package attribute, e.g.

meta.outputsToInstall = [ "bin" "man" ];

which would also be respected by nix-env.

Copy link
Member Author

Choose a reason for hiding this comment

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

So let's use meta.outputsToInstall directly, and stdenv shall fill it in if not specified. The needed nix-env changes are clear (and trivial, I think).

I'll think on a suitable customization mechanism via the nixpkgs config, but I feel like in the long term we would better unify this perhaps through nixUP or something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm still not certain how to best make it customizable, but the nix-env part really seems clear, so I implemented that now. People would better get that functionality early NixOS/nix#815.

@dezgeg
Copy link
Contributor

dezgeg commented Feb 2, 2016

IMHO this per-package meta.outputsToInstall shouldn't contain anything related to man or info but rather keep that in the systemPackages logic, since that would allow us to have a configuration option to disable man- or infopages and not install them in the first place.

vcunat added a commit to vcunat/nix that referenced this pull request Feb 23, 2016
As a result `systemPackages` now also respect it.
Only nix-env remains and that has a PR filed:
    NixOS/nix#815
This is to get more consistent with `meta.outputsToInstall`.
@vcunat vcunat merged commit 9a824f2 into NixOS:closure-size Apr 7, 2016
vcunat added a commit that referenced this pull request Apr 7, 2016
@vcunat vcunat deleted the p/default-outputs branch April 12, 2016 08:57
edolstra pushed a commit to NixOS/nix that referenced this pull request Mar 21, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 9a824f2 on vcunat:p/default-outputs into ** on NixOS:closure-size**.

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

7 participants