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 overlays mechanism to Nixpkgs. #21243

Merged
merged 13 commits into from Jan 16, 2017

Conversation

@nbp
Member

nbp commented Dec 17, 2016

This patch add a new argument to Nixpkgs default expression named "overlays".

By default, the value of the argument is either taken from the environment variable NIXPKGS_OVERLAYS,
or from the directory ~/.nixpkgs/overlays/. If the environment variable does not name a valid directory
then this mechanism would fallback on the home directory. If the home directory does not exists it will
fallback on an empty list of overlays.

The overlays directory should contain the list of extra Nixpkgs stages which would be used to extend the
content of Nixpkgs, with additional set of packages. The overlays, i-e directory, files, symbolic links
are used in alphabetical order.

The simplest overlay which extends Nixpkgs with nothing looks like:

self: super: {
}

More refined overlays can use super as the basis for building new packages, and self as a way to query
the final result of the fix-point.

An example of overlay which extends Nixpkgs with a small set of packages can be found at:
https://github.com/nbp/nixpkgs-mozilla/blob/nixpkgs-overlay/moz-overlay.nix

To use this file, checkout the repository and add a symbolic link to
the moz-overlay.nix file in ~/.nixpkgs/overlays directory.

Motivation

The goal of this patch is to provide an alternative to the pkgs.overridePackages function (#20927), which re-execute Nixpkgs fix-point with new inputs. Instead, the alternative would be use Nixpkgs top-level function, with an overlays attribute which would be a list with the same argument as what is today the argument given to pkgs.overridePackages.

Thus, in addition to provide a useful extensible layer to Nixpkgs third parties, such as Mozilla, or any other company. This would remove the existing leakage of Nixpkgs fix-point out of Nixpkgs. This feature would also be nicer for the security-updates branch (#10851), as the overlays would be under the fix-point on which the patching mechanism works on.

Things done
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 17, 2016

@nbp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ericson2314, @errge and @edolstra to be potential reviewers.

@nbp, thanks for your PR! By analyzing the history of the files in this pull request, we identified @Ericson2314, @errge and @edolstra to be potential reviewers.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Dec 17, 2016

Member

Hmm I'll need to think more about the ramifications for security updates, but otherwise I'm confused by the motivation for this. To me, mkExtensible is one of the best parts of the fix-extends ideom, and one I hope to see used more widely, not less.

CC @cstrahan, who created that abstraction.

Member

Ericson2314 commented Dec 17, 2016

Hmm I'll need to think more about the ramifications for security updates, but otherwise I'm confused by the motivation for this. To me, mkExtensible is one of the best parts of the fix-extends ideom, and one I hope to see used more widely, not less.

CC @cstrahan, who created that abstraction.

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Dec 17, 2016

Member

@Ericson2314, @cstrahan:

The security-updates branch is using the fixed function a first time within the fix-point for base packages, and then as a standalone function for patching packages, and as a guide for another fix-point for propagating the patches.

The makeExtensibleWithCustomName function create a new attribute which let you inject phases in the pipeline of fixed function. If we wanted to carry the security-updates mechanism for the pkgs.overridePackages, then we would have to add special cases for this function, which I consider to be more painful than useful.

By adding the overlays list of extra phases, you can easily combine orthogonal extensions of Nixpkgs, which surpass the pkgs.overridePackages which only take one extra phase. Moreover, the pkgs.overridePackages function can easily be emulated as demonstrated in #20927 (comment)

Thus, I see no need to keep the pkgs.overridePackages function.

Member

nbp commented Dec 17, 2016

@Ericson2314, @cstrahan:

The security-updates branch is using the fixed function a first time within the fix-point for base packages, and then as a standalone function for patching packages, and as a guide for another fix-point for propagating the patches.

The makeExtensibleWithCustomName function create a new attribute which let you inject phases in the pipeline of fixed function. If we wanted to carry the security-updates mechanism for the pkgs.overridePackages, then we would have to add special cases for this function, which I consider to be more painful than useful.

By adding the overlays list of extra phases, you can easily combine orthogonal extensions of Nixpkgs, which surpass the pkgs.overridePackages which only take one extra phase. Moreover, the pkgs.overridePackages function can easily be emulated as demonstrated in #20927 (comment)

Thus, I see no need to keep the pkgs.overridePackages function.

@the-kenny

Some directory-resolution related questions.

Show outdated Hide outdated doc/overlays.xml
<listitem>
<para>As the directory located at

This comment has been minimized.

@the-kenny

the-kenny Dec 25, 2016

Contributor

How does this option work with different users? Will only the overlay set of the user executing nixos-rebuild considered or will it always use root's?

@the-kenny

the-kenny Dec 25, 2016

Contributor

How does this option work with different users? Will only the overlay set of the user executing nixos-rebuild considered or will it always use root's?

This comment has been minimized.

@nbp

nbp Dec 25, 2016

Member

With the current patch, it will take the overlays of the user which is executing the command.

With the above suggestion, none should be considered unless they are specified in the configuration.nix file, or any other module.

@nbp

nbp Dec 25, 2016

Member

With the current patch, it will take the overlays of the user which is executing the command.

With the above suggestion, none should be considered unless they are specified in the configuration.nix file, or any other module.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

In the directory <filename>~/.nixpkgs/overlays/</filename>.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

In the directory <filename>~/.nixpkgs/overlays/</filename>.

Show outdated Hide outdated doc/overlays.xml
<listitem>
<para>As argument of the imported attribute set. When importing Nixpkgs,

This comment has been minimized.

@the-kenny

the-kenny Dec 25, 2016

Contributor

Is it possible to hard-code this directory for nixos-rebuild from configuration.nix? I think that might be useful.

@the-kenny

the-kenny Dec 25, 2016

Contributor

Is it possible to hard-code this directory for nixos-rebuild from configuration.nix? I think that might be useful.

This comment has been minimized.

@nbp

nbp Dec 25, 2016

Member

I agree.

This would be a terrible experience if one configuration.nix is not evaluated the same way across computer depending on the ~/.nixpkgs/overlays directory content.

I will add the overlays argument to nixos/modules/misc/nixpkgs.nix.

@nbp

nbp Dec 25, 2016

Member

I agree.

This would be a terrible experience if one configuration.nix is not evaluated the same way across computer depending on the ~/.nixpkgs/overlays directory content.

I will add the overlays argument to nixos/modules/misc/nixpkgs.nix.

This comment has been minimized.

@nbp

nbp Dec 26, 2016

Member

This is fixed in commit 46ae6b5

@nbp

nbp Dec 26, 2016

Member

This is fixed in commit 46ae6b5

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: As an argument ...

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: As an argument ...

@teh

teh approved these changes Dec 26, 2016

This is really amazing work & a lot of power for such a small change! I'm very much in favour.

Show outdated Hide outdated doc/overlays.xml
<title>Overlays</title>
<para>This chapter describes how to extend and change Nixpkgs content using
overlays. Overlays are used to add phases in the fix-point used by Nixpkgs

This comment has been minimized.

@teh

teh Dec 26, 2016

Contributor

"phases" sounds like pre or post-phase. Is a "phase" one file (and one function), and executed in alphabetical order?

@teh

teh Dec 26, 2016

Contributor

"phases" sounds like pre or post-phase. Is a "phase" one file (and one function), and executed in alphabetical order?

This comment has been minimized.

@nbp

nbp Dec 26, 2016

Member

Yes, one phase / layer is one overlay, which is a single function self: super: { ... }. Maybe I should use the term layer instead of phase?

@nbp

nbp Dec 26, 2016

Member

Yes, one phase / layer is one overlay, which is a single function self: super: { ... }. Maybe I should use the term layer instead of phase?

This comment has been minimized.

@acowley

acowley Dec 26, 2016

Contributor

I think "layer" would be better, too.

@acowley

acowley Dec 26, 2016

Contributor

I think "layer" would be better, too.

Show outdated Hide outdated doc/overlays.xml
<title>Installing Overlays</title>
<para>Overlays are looked for in the following order, the first valid one is
considered, and all the rest are ignored:

This comment has been minimized.

@teh

teh Dec 26, 2016

Contributor

Why would we ignore invalid files instead of throwing an error? This seems like a recipe for "silent, hard-to-debug" problems?

@teh

teh Dec 26, 2016

Contributor

Why would we ignore invalid files instead of throwing an error? This seems like a recipe for "silent, hard-to-debug" problems?

This comment has been minimized.

@nbp

nbp Dec 26, 2016

Member

What I meant is the fallback logic, we consider the environment variable NIXPKGS_OVERLAYS, and check if this is a directory. Then we do the same check for the $HOME/.nixpkgs/overlays.

I agree, we should throw if NIXPKGS_OVERLAYS is set, but the content does not validate.
But I do not think we should do anything if the $HOME/.nixpkgs/overlays directory does not exists.

@nbp

nbp Dec 26, 2016

Member

What I meant is the fallback logic, we consider the environment variable NIXPKGS_OVERLAYS, and check if this is a directory. Then we do the same check for the $HOME/.nixpkgs/overlays.

I agree, we should throw if NIXPKGS_OVERLAYS is set, but the content does not validate.
But I do not think we should do anything if the $HOME/.nixpkgs/overlays directory does not exists.

Show outdated Hide outdated doc/overlays.xml
<listitem>
<para>As a directory pointed by the environment variable named
<varname>NIXPKGS_OVERLAYS</varname>. This directory can contain symbolic

This comment has been minimized.

@teh

teh Dec 26, 2016

Contributor

Does NIXPKGS_OVERLAYS support colon separated paths like PATH?

@teh

teh Dec 26, 2016

Contributor

Does NIXPKGS_OVERLAYS support colon separated paths like PATH?

This comment has been minimized.

@nbp

nbp Dec 26, 2016

Member

For the moment no, I think this would be a nice addition in a future patch.

I also think being able to pull a Nix expression from a trusted remote (such as https://my.company/overlay.nix) would also be interesting.

@nbp

nbp Dec 26, 2016

Member

For the moment no, I think this would be a nice addition in a future patch.

I also think being able to pull a Nix expression from a trusted remote (such as https://my.company/overlay.nix) would also be interesting.

Show outdated Hide outdated nixos/doc/manual/release-notes/rl-1703.xml
@@ -88,6 +90,11 @@ following incompatible changes:</para>
<literal>networking.timeServers</literal>.
</para>
</listitem>
<listitem>
<para><literal>pkgs.overridePackages</literal> function no longer exists. Instead import Nixpkgs a second time using <literal>import pkgs.path {

This comment has been minimized.

@teh

teh Dec 26, 2016

Contributor

That's a very long space after "exists." Maybe a missing line break?

@teh

teh Dec 26, 2016

Contributor

That's a very long space after "exists." Maybe a missing line break?

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Dec 27, 2016

Member

Note in 2278414#diff-19c9515018ff76dea736b57000a62d5bR49 I use overridePackages in a way that I worry would be hard to replicate with this.

Member

Ericson2314 commented Dec 27, 2016

Note in 2278414#diff-19c9515018ff76dea736b57000a62d5bR49 I use overridePackages in a way that I worry would be hard to replicate with this.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Dec 27, 2016

Member

More broadly, as I wrote in #16531 (comment) I think I thinking instead of having nixpkgs vs nixos, we should have nixpkgs vs "nixuser" vs "nixos". nixkpgs would have no config attrset, and no impure.nix, and basically just be packages. nixuser would have some sort of config system (probably module system), things like firefoxWrapper, dotfiles management if we ever get that.

I think splitting the pure and impure parts of this would be an excellent further justification for factoring out this "nixuser".

Member

Ericson2314 commented Dec 27, 2016

More broadly, as I wrote in #16531 (comment) I think I thinking instead of having nixpkgs vs nixos, we should have nixpkgs vs "nixuser" vs "nixos". nixkpgs would have no config attrset, and no impure.nix, and basically just be packages. nixuser would have some sort of config system (probably module system), things like firefoxWrapper, dotfiles management if we ever get that.

I think splitting the pure and impure parts of this would be an excellent further justification for factoring out this "nixuser".

@domenkozar

Overall looks good.

I think documentation needs someone who speaks English natively to flesh it out.

For Overlays I think we should have a tool that manages them. Something similar to git submodule that would sync git repositories. We should decide to go this route from the first day, prohibiting manipulation of ~/.nixpkgs/overlays by hand.

Show outdated Hide outdated nixos/doc/manual/release-notes/rl-1703.xml
<para></para>
<para>Nixpkgs is now extensible through <link
xlink:href="https://github.com/NixOS/nixpkgs/pull/21243">overlays</link>.
See the Nixpkgs manual for more information.</para>

This comment has been minimized.

@domenkozar

domenkozar Dec 28, 2016

Member

There should be a link to the manual with docs.

@domenkozar

domenkozar Dec 28, 2016

Member

There should be a link to the manual with docs.

This comment has been minimized.

@nbp

nbp Jan 4, 2017

Member

@domenkozar How would you link to the new Nixpkgs documentation from the release notes?

@nbp

nbp Jan 4, 2017

Member

@domenkozar How would you link to the new Nixpkgs documentation from the release notes?

Show outdated Hide outdated nixos/doc/manual/release-notes/rl-1703.xml
<listitem>
<para><literal>pkgs.overridePackages</literal> function no longer exists.
Instead import Nixpkgs a second time using <literal>import pkgs.path {
overlays = [ <replaceable>...</replaceable> ]; }</literal>.</para>

This comment has been minimized.

@domenkozar

domenkozar Dec 28, 2016

Member

This is not clear what pkgs is and what would be the contents of overlays. It's better to point to overlays section in the manual.

@domenkozar

domenkozar Dec 28, 2016

Member

This is not clear what pkgs is and what would be the contents of overlays. It's better to point to overlays section in the manual.

@teh

This comment has been minimized.

Show comment
Hide comment
@teh

teh Jan 2, 2017

Contributor

I gave this a spin and I'm not super convinced that a directory is the best public API (it's not terrible either). The additional overlays parameter however is awesome!

@domenkozar I think the docs are OK - do you have a specific thing in mind? Also, what kind of tool do you have in mind?

Both @nbp & @domenkozar - what do you think about splitting the filesystem API (~/.nixpkgs/overlays and NIXPKGS_OVERLAYS) into a separate branch so we can get the uncontroversial parts merged soon? I can do the work :)

Contributor

teh commented Jan 2, 2017

I gave this a spin and I'm not super convinced that a directory is the best public API (it's not terrible either). The additional overlays parameter however is awesome!

@domenkozar I think the docs are OK - do you have a specific thing in mind? Also, what kind of tool do you have in mind?

Both @nbp & @domenkozar - what do you think about splitting the filesystem API (~/.nixpkgs/overlays and NIXPKGS_OVERLAYS) into a separate branch so we can get the uncontroversial parts merged soon? I can do the work :)

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 4, 2017

Member

I think being able to manage overlays by hand is a nice option for experts, probably not the mainstream one to advertise, I agree.

@teh, The file system API is the one which makes the overlays accessible to a large set of users, especially with a tool to manipulate these overlays from a command line.

@domenkozar, Do you have any native English speaker in mind who could do the review of the documentation?

Member

nbp commented Jan 4, 2017

I think being able to manage overlays by hand is a nice option for experts, probably not the mainstream one to advertise, I agree.

@teh, The file system API is the one which makes the overlays accessible to a large set of users, especially with a tool to manipulate these overlays from a command line.

@domenkozar, Do you have any native English speaker in mind who could do the review of the documentation?

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 4, 2017

Member

@Ericson2314

Note in 2278414#diff-19c9515018ff76dea736b57000a62d5bR49 I use overridePackages in a way that I worry would be hard to replicate with this.

Let me disagree, this can easily be replicated by exposing under nixpkgs the set of overlays which are being used, such that you can extend it in the next Nixpkgs import.

The comment above overridePackages states that it should not be used in Nixpkgs, and I would recommend no less for overlays, as in both cases we are re-evaluating Nixpkgs fix-point. To this point I would also agree that we already do that in a different way for stdenv and consumes 60 MB in all Nixpkgs evaluation. Changing from one way of consuming 60 MB to another way is point-less.

This 60 MB consumption is a problem for another bug, which is that stdenv bootstrapping should not use allPackages function to create a new fix-point, but re-use the current one to extract the recipes of the packages that it needs for the bootstrap.

Member

nbp commented Jan 4, 2017

@Ericson2314

Note in 2278414#diff-19c9515018ff76dea736b57000a62d5bR49 I use overridePackages in a way that I worry would be hard to replicate with this.

Let me disagree, this can easily be replicated by exposing under nixpkgs the set of overlays which are being used, such that you can extend it in the next Nixpkgs import.

The comment above overridePackages states that it should not be used in Nixpkgs, and I would recommend no less for overlays, as in both cases we are re-evaluating Nixpkgs fix-point. To this point I would also agree that we already do that in a different way for stdenv and consumes 60 MB in all Nixpkgs evaluation. Changing from one way of consuming 60 MB to another way is point-less.

This 60 MB consumption is a problem for another bug, which is that stdenv bootstrapping should not use allPackages function to create a new fix-point, but re-use the current one to extract the recipes of the packages that it needs for the bootstrap.

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jan 4, 2017

Member

The comment above overridePackages states that it should not be used in Nixpkgs, and I would recommend no less for overlays, as in both cases we are re-evaluating Nixpkgs fix-point.

I copied that note. I am not at all sure the extends idiom is expensive as the original fix-point is only "barely evaluated".

This 60 MB consumption is a problem for another bug, which is that stdenv bootstrapping should not use allPackages function to create a new fix-point, but re-use the current one to extract the recipes of the packages that it needs for the bootstrap.

Ok this is a much larger conversation. I'm on the fence about whether this is a good idea for the existing bootstrapping, but it definitely isn't a replacement for cross which as far as I can tell requires multiple fixes. Let's talk about this in my cross and bootstrapping PRs.

Member

Ericson2314 commented Jan 4, 2017

The comment above overridePackages states that it should not be used in Nixpkgs, and I would recommend no less for overlays, as in both cases we are re-evaluating Nixpkgs fix-point.

I copied that note. I am not at all sure the extends idiom is expensive as the original fix-point is only "barely evaluated".

This 60 MB consumption is a problem for another bug, which is that stdenv bootstrapping should not use allPackages function to create a new fix-point, but re-use the current one to extract the recipes of the packages that it needs for the bootstrap.

Ok this is a much larger conversation. I'm on the fence about whether this is a good idea for the existing bootstrapping, but it definitely isn't a replacement for cross which as far as I can tell requires multiple fixes. Let's talk about this in my cross and bootstrapping PRs.

nbp added some commits Dec 17, 2016

Add overlays mechanism to Nixpkgs.
This patch add a new argument to Nixpkgs default expression named "overlays".

By default, the value of the argument is either taken from the environment variable `NIXPKGS_OVERLAYS`,
or from the directory `~/.nixpkgs/overlays/`.  If the environment variable does not name a valid directory
then this mechanism would fallback on the home directory.  If the home directory does not exists it will
fallback on an empty list of overlays.

The overlays directory should contain the list of extra Nixpkgs stages which would be used to extend the
content of Nixpkgs, with additional set of packages.  The overlays, i-e directory, files, symbolic links
are used in alphabetical order.

The simplest overlay which extends Nixpkgs with nothing looks like:

```nix
self: super: {
}
```

More refined overlays can use `super` as the basis for building new packages, and `self` as a way to query
the final result of the fix-point.

An example of overlay which extends Nixpkgs with a small set of packages can be found at:
  https://github.com/nbp/nixpkgs-mozilla/blob/nixpkgs-overlay/moz-overlay.nix

To use this file, checkout the repository and add a symbolic link to
the `moz-overlay.nix` file in `~/.nixpkgs/overlays` directory.
Improve the realse notes with the upcoming documentation links, and a…
… better example of how to convert overridePackages usage.
@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 14, 2017

Member

I think this changes are good, and should be merged, and if possible before they bit-rot once more.
I fixed the documentation issue reported by @domenkozar , but I have not yet made a tool to handle repository under ~/.nixpkgs/overlay, because I do not think this is yet necessary to enforce such thing yet.

I think having a tool would be better once this feature is getting more adoptions from users and companies. Then we can standardize what layout of overlay repositories should be.

Concerning having a native speaker reviewing the documentation, I did not got any answer from @domenkozar. Maybe @zimbatm could do the review of the documentation?

Member

nbp commented Jan 14, 2017

I think this changes are good, and should be merged, and if possible before they bit-rot once more.
I fixed the documentation issue reported by @domenkozar , but I have not yet made a tool to handle repository under ~/.nixpkgs/overlay, because I do not think this is yet necessary to enforce such thing yet.

I think having a tool would be better once this feature is getting more adoptions from users and companies. Then we can standardize what layout of overlay repositories should be.

Concerning having a native speaker reviewing the documentation, I did not got any answer from @domenkozar. Maybe @zimbatm could do the review of the documentation?

@domenkozar

This comment has been minimized.

Show comment
Hide comment
@domenkozar

domenkozar Jan 14, 2017

Member

Sounds good to me. I think tooling would leverage this code, but I understand you have even less time than me :) So if we can get @zimbatm to review docs, this sounds like a good step forward.

Member

domenkozar commented Jan 14, 2017

Sounds good to me. I think tooling would leverage this code, but I understand you have even less time than me :) So if we can get @zimbatm to review docs, this sounds like a good step forward.

@aneeshusa

This comment has been minimized.

Show comment
Hide comment
@aneeshusa

aneeshusa Jan 14, 2017

Contributor

I'm a native English speaker, I can try to review the docs later today.

Contributor

aneeshusa commented Jan 14, 2017

I'm a native English speaker, I can try to review the docs later today.

@fpletz

fpletz approved these changes Jan 14, 2017

@aneeshusa

Overall docs look good, this is mostly small grammar fixes.

Also a request for a special callPackage to make using overlays easier.

Show outdated Hide outdated doc/overlays.xml
<title>Overlays</title>
<para>This chapter describes how to extend and change Nixpkgs content using

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: content is a bit strange here, maybe use packages or just remove it

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: content is a bit strange here, maybe use packages or just remove it

Show outdated Hide outdated doc/overlays.xml
<title>Overlays</title>
<para>This chapter describes how to extend and change Nixpkgs content using
overlays. Overlays are used to add layers in the fix-point used by Nixpkgs

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: Only use one space between sentences

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: Only use one space between sentences

Show outdated Hide outdated doc/overlays.xml
<section xml:id="sec-overlays-install">
<title>Installing Overlays</title>
<para>The set of overlays are looked for in the following order, only the

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: set of overlays is singular but are is for plurals; I would say The set of overlays is looked for in the following places.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: set of overlays is singular but are is for plurals; I would say The set of overlays is looked for in the following places.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

I would split this into two sentences between order and only.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

I would split this into two sentences between order and only.

Show outdated Hide outdated nixos/modules/misc/nixpkgs.nix
openssh = super.openssh.override {
hpnSupport = true;
withKerberos = true;
kerberos = self.libkrb5

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: missing semicolon

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: missing semicolon

Show outdated Hide outdated nixos/doc/manual/release-notes/rl-1703.xml
pkgs.overridePackages (self: super: ...)
</programlisting>
Should be replaced by:

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: don't capitalize, use should

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

nit: don't capitalize, use should

Show outdated Hide outdated doc/overlays.xml
</orderedlist>
</para>
<para>For the second and third option, the directory contains either

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

..., the directory should contain Nix expressions defining the overlays. Each overlay can be a file, a directory containing a default.nix file, or a symlink to one of those. The expressions should follow the syntax described in ...

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

..., the directory should contain Nix expressions defining the overlays. Each overlay can be a file, a directory containing a default.nix file, or a symlink to one of those. The expressions should follow the syntax described in ...

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

Also, this section should describe any ordering guarantees (or lack thereof) about overlays retrieved from a directory.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

Also, this section should describe any ordering guarantees (or lack thereof) about overlays retrieved from a directory.

This comment has been minimized.

@nbp

nbp Jan 15, 2017

Member

I added the following paragraph right after to describe the ordering of overlays, which has an impact both when providing a list of overlays as arguments of Nixpkgs, or when using a directory.

The order of the overlay layers can influence the recipe of packages if two layers override the same recipe. In the case where overlays are loaded from a directory, these are loaded in alphabetical order.

@nbp

nbp Jan 15, 2017

Member

I added the following paragraph right after to describe the ordering of overlays, which has an impact both when providing a list of overlays as arguments of Nixpkgs, or when using a directory.

The order of the overlay layers can influence the recipe of packages if two layers override the same recipe. In the case where overlays are loaded from a directory, these are loaded in alphabetical order.

Show outdated Hide outdated doc/overlays.xml
following the syntax described in <xref
linkend="sec-overlays-layout"/>.</para>
<para>To install an overlay, using the last option. Clone the repository of

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: To install an overlay using the last option, you can clone the overlay's repository and add a symbolic link to it in the ...

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

grammar: To install an overlay using the last option, you can clone the overlay's repository and add a symbolic link to it in the ...

Show outdated Hide outdated doc/overlays.xml
<para>This chapter describes how to extend and change Nixpkgs content using
overlays. Overlays are used to add layers in the fix-point used by Nixpkgs
to bind the dependencies of all packages.</para>

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

Overlays can also be used for adding packages, so I would make this a bit more general, maybe used by Nixpkgs to compose the set of all packages.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

Overlays can also be used for adding packages, so I would make this a bit more general, maybe used by Nixpkgs to compose the set of all packages.

Show outdated Hide outdated doc/overlays.xml
<para>As a directory pointed by the environment variable named
<varname>NIXPKGS_OVERLAYS</varname>. This directory can contain symbolic
links to Nix expressions.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

No need to mention symlinks here since it's described below.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

No need to mention symlinks here since it's described below.

Show outdated Hide outdated doc/overlays.xml
<para>As the directory located at
<filename>~/.nixpkgs/overlays/</filename>. This directory can contain
symbolic links to Nix expressions.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

No need to mention symlinks here since it's described below.

@aneeshusa

aneeshusa Jan 14, 2017

Contributor

No need to mention symlinks here since it's described below.

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 14, 2017

Member

@aneeshusa , Thanks a lot of the review, I will fix the nits tomorrow.

For the custom callPackage, I think that is a good idea, I will think about it and the consequences/applications with the security-updates branch.

Member

nbp commented Jan 14, 2017

@aneeshusa , Thanks a lot of the review, I will fix the nits tomorrow.

For the custom callPackage, I think that is a good idea, I will think about it and the consequences/applications with the security-updates branch.

@aneeshusa

This comment has been minimized.

Show comment
Hide comment
@aneeshusa

aneeshusa Jan 15, 2017

Contributor

FYI, just noticed I had made a typo in my example for a custom callPackage, I fixed it at #21243 (comment).

Contributor

aneeshusa commented Jan 15, 2017

FYI, just noticed I had made a typo in my example for a custom callPackage, I fixed it at #21243 (comment).

All review comments are addressed. I am slowly working on a nix-overlay tool.

@aneeshusa

aneeshusa suggested changes Jan 15, 2017 edited

A few more grammar nits, and some confusion about the usage of callPackage here.

IDK why GitHub is showing these as outdated, they should still be valid comments at time of writing...

Show outdated Hide outdated doc/overlays.xml
to the entry Nix expression of the overlay. These Nix expressions are
following the syntax described in <xref
linkend="sec-overlays-layout"/>.</para>
<para>For the second and third option, the directory should contain Nix expressions defining the

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

grammar: options

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

grammar: options

Show outdated Hide outdated doc/overlays.xml
the overlay, and add a symbolic link to it in the
<filename>~/.nixpkgs/overlays/</filename> directory.</para>
<para>The order of the overlay layers can influence the recipe of packages if multiple layers override
the same recipe. In the case where overlays are loaded from a directory, these are loaded in

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

they are loaded

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

they are loaded

Show outdated Hide outdated doc/overlays.xml
alphabetical order.</para>
<para>To install an overlay using the last option, you can clone the overlay's repository and add
a symbolic link to in the <filename>~/.nixpkgs/overlays/</filename> directory.</para>

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

link to it in

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

link to it in

Show outdated Hide outdated doc/overlays.xml
<para>An overlay is a Nix expression, which is a function which accepts 2
arguments.</para>
<para>Overlays are expressed as Nix functions which accept 2 arguments and return a set of
packages</para>

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

Needs a period or colon at the end.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

Needs a period or colon at the end.

Show outdated Hide outdated doc/overlays.xml
boost = super.boost.override {
python = self.python3;
};
rr = super.callPackage ./pkgs/rr {

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

Why is this super.callPackage? I would think super.callPackage would use packages from super for dependencies, where the documentation says to use packages from self.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

Why is this super.callPackage? I would think super.callPackage would use packages from super for dependencies, where the documentation says to use packages from self.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

E.g. I would think we need to provide a custom callPackage for each self layer using
lib.callPackagesWith.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

E.g. I would think we need to provide a custom callPackage for each self layer using
lib.callPackagesWith.

This comment has been minimized.

@nbp

nbp Jan 15, 2017

Member

Technically you could use both, but this would get some extra meaning in the futur (security-update branch), because self.callPackage goes twice through the self attribute evaluation. The first one to resolve the callPackage function, and the second time to resolve the dependencies from self which are captured by the callPackage function.

Note that self is the same provided to all layers. So using self.callPackage is for the moment identical to super.callPackage, except for this extra hop through the fix-point.

I documented it as functions should be taken from super, because they already capture self if they have to.

Also note that this way, we make it syntactically verifiable, as anything which comes around the dependencies uses super, and anything which is use to find dependencies uses self.

@nbp

nbp Jan 15, 2017

Member

Technically you could use both, but this would get some extra meaning in the futur (security-update branch), because self.callPackage goes twice through the self attribute evaluation. The first one to resolve the callPackage function, and the second time to resolve the dependencies from self which are captured by the callPackage function.

Note that self is the same provided to all layers. So using self.callPackage is for the moment identical to super.callPackage, except for this extra hop through the fix-point.

I documented it as functions should be taken from super, because they already capture self if they have to.

Also note that this way, we make it syntactically verifiable, as anything which comes around the dependencies uses super, and anything which is use to find dependencies uses self.

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

That sounds reasonable, but can you explain how functions already capture self if they need to? E.g. if I have an overlay that adds two new packages:

self: super: {
    # Let's say all deps are already in super,
    # so I can see this working with super.callPackages or self.callPackages
    foo = super.callPackage {};
    # But this depends on foo, which isn't in super
    bar = super.callPackage {}; 
}

How does the second super.callPackage get a reference to foo to provide to bar?

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

That sounds reasonable, but can you explain how functions already capture self if they need to? E.g. if I have an overlay that adds two new packages:

self: super: {
    # Let's say all deps are already in super,
    # so I can see this working with super.callPackages or self.callPackages
    foo = super.callPackage {};
    # But this depends on foo, which isn't in super
    bar = super.callPackage {}; 
}

How does the second super.callPackage get a reference to foo to provide to bar?

This comment has been minimized.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

OK, I tried it out and looked at the implementation and I now understand why this works, albeit the current behavior is slightly surprising to me.

@aneeshusa

aneeshusa Jan 15, 2017

Contributor

OK, I tried it out and looked at the implementation and I now understand why this works, albeit the current behavior is slightly surprising to me.

This comment has been minimized.

@nbp

nbp Jan 15, 2017

Member
self: super:

{
  callWithSelf = f: f self;
}

Any layer defined after could then do:

self: super:

{
  mySelfClone = super.callWithSelf (x: x);
}
@nbp

nbp Jan 15, 2017

Member
self: super:

{
  callWithSelf = f: f self;
}

Any layer defined after could then do:

self: super:

{
  mySelfClone = super.callWithSelf (x: x);
}
@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 15, 2017

Member

Tested locally on top of nixpkgs-unstable channel:

  • ./maintainers/scripts/travis-nox-review-pr.sh nixpkgs-verify nixpkgs-manual nixpkgs-tarball nixpkgs-unstable
  • ./maintainers/scripts/travis-nox-review-pr.sh nixos-options nixos-manual
Member

nbp commented Jan 15, 2017

Tested locally on top of nixpkgs-unstable channel:

  • ./maintainers/scripts/travis-nox-review-pr.sh nixpkgs-verify nixpkgs-manual nixpkgs-tarball nixpkgs-unstable
  • ./maintainers/scripts/travis-nox-review-pr.sh nixos-options nixos-manual

@nbp nbp merged commit 8366525 into NixOS:master Jan 16, 2017

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
<programlisting>
let
pkgs = import &lt;nixpkgs&gt; {}; in

This comment has been minimized.

@teh

teh Jan 16, 2017

Contributor

just noticed a double in (see next line) - @nbp if you have direct push permissions maybe a quick fix? Otherwise I'll try to mop it up with some other change.

@teh

teh Jan 16, 2017

Contributor

just noticed a double in (see next line) - @nbp if you have direct push permissions maybe a quick fix? Otherwise I'll try to mop it up with some other change.

This comment has been minimized.

@nbp

nbp Jan 17, 2017

Member

This is fixed as part of commit 0214d94. Thanks!

@nbp

nbp Jan 17, 2017

Member

This is fixed as part of commit 0214d94. Thanks!

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jan 16, 2017

Member

Wait, do overlays currently apply to bootstrapping stages after all?

Member

Ericson2314 commented Jan 16, 2017

Wait, do overlays currently apply to bootstrapping stages after all?

@@ -80,7 +83,7 @@ in let
boot = import ../stdenv/booter.nix { inherit lib allPackages; };

This comment has been minimized.

@Ericson2314

Ericson2314 Jan 16, 2017

Member

If we do want all bootstraping stages to include the overlay unconditionally, we could instead "partially apply" overlays to all packages (3 lines above the inherit lib nixpkgsFun;, but I cannot comment there because unchanged).

@Ericson2314

Ericson2314 Jan 16, 2017

Member

If we do want all bootstraping stages to include the overlay unconditionally, we could instead "partially apply" overlays to all packages (3 lines above the inherit lib nixpkgsFun;, but I cannot comment there because unchanged).

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Jan 17, 2017

Member

@Ericson2314

Wait, do overlays currently apply to bootstrapping stages after all?

Yeah, but I think it would makes sense to disable that as soon as we can move the bootstraping process in Nixpkgs fix-point, as this would no longer be needed. For the moment I would not recommend anybody to rely on overlays for changing the stdenv, except by doing it explicitly.

@teh Thanks, I will fix it later, if nobody does it before ;)

Member

nbp commented Jan 17, 2017

@Ericson2314

Wait, do overlays currently apply to bootstrapping stages after all?

Yeah, but I think it would makes sense to disable that as soon as we can move the bootstraping process in Nixpkgs fix-point, as this would no longer be needed. For the moment I would not recommend anybody to rely on overlays for changing the stdenv, except by doing it explicitly.

@teh Thanks, I will fix it later, if nobody does it before ;)

@Ericson2314

This comment has been minimized.

Show comment
Hide comment
@Ericson2314

Ericson2314 Jan 17, 2017

Member

@nbp, well, to be clear, we can disable it in the meantime too. But that plan sounds fine to me.

Member

Ericson2314 commented Jan 17, 2017

@nbp, well, to be clear, we can disable it in the meantime too. But that plan sounds fine to me.

@globin globin referenced this pull request Jan 19, 2017

Closed

ccacheWrapper broken #21984

@grahamc grahamc referenced this pull request Jan 23, 2017

Closed

#2 weekly news #14

0 of 17 tasks complete

@grahamc grahamc added the weekly label Jan 23, 2017

@aneeshusa aneeshusa referenced this pull request Jan 28, 2017

Merged

NixOS: Respect nixpkgs.overlays #22221

4 of 7 tasks complete
@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Jan 31, 2017

Member

IMHO we should not add more ad hoc environment variables like $NIXPKGS_OVERLAYS. Why not use the NIX_PATH mechanism for this?

Member

edolstra commented Jan 31, 2017

IMHO we should not add more ad hoc environment variables like $NIXPKGS_OVERLAYS. Why not use the NIX_PATH mechanism for this?

@edolstra

This comment has been minimized.

Show comment
Hide comment
@edolstra

edolstra Feb 1, 2017

Member

I've changed $NIXPKGS_OVERLAYS to <nixpkgs-overlays>, and ~/.nixpkgs/overlays to ~/.config/nixpkgs/overlays in order to not add more non-XDG-compliant paths.

BTW, since the point of overlays is that they can be stacked easily, should the overlays in <nixpkgs-overlays> and ~/.config/nixpkgs/overlays be merged, rather than having the former override the latter?

Member

edolstra commented Feb 1, 2017

I've changed $NIXPKGS_OVERLAYS to <nixpkgs-overlays>, and ~/.nixpkgs/overlays to ~/.config/nixpkgs/overlays in order to not add more non-XDG-compliant paths.

BTW, since the point of overlays is that they can be stacked easily, should the overlays in <nixpkgs-overlays> and ~/.config/nixpkgs/overlays be merged, rather than having the former override the latter?

@nbp

This comment has been minimized.

Show comment
Hide comment
@nbp

nbp Feb 1, 2017

Member

@edolstra

BTW, since the point of oveprlays is that they can be stacked easily, should the overlays in <nixpkgs-overlays> and ~/.config/nixpkgs/overlays be merged, rather than having the former override the latter?

I honestly do not know yet what would be the most desirable behaviour.

My initial though was that we could replace the default configuration and test with a different one, which is interesting in terms of reproducibility.

Member

nbp commented Feb 1, 2017

@edolstra

BTW, since the point of oveprlays is that they can be stacked easily, should the overlays in <nixpkgs-overlays> and ~/.config/nixpkgs/overlays be merged, rather than having the former override the latter?

I honestly do not know yet what would be the most desirable behaviour.

My initial though was that we could replace the default configuration and test with a different one, which is interesting in terms of reproducibility.

@michaelpj michaelpj referenced this pull request Aug 13, 2017

Merged

Overlays: allow overlays to be specified as a file #28228

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment