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

Overlays #26321

Closed
wants to merge 1 commit into from
Closed

Overlays #26321

wants to merge 1 commit into from

Conversation

qknight
Copy link
Member

@qknight qknight commented Jun 2, 2017

Motivation for this change

extended the overlays section of nixpkgs doc. took me quite a few hours until i figued how to use it, see https://stackoverflow.com/questions/44316127/implementing-override-in-my-repository/44318322#44318322

@nbp this is very good work, thanks for creating this overlays stuff!

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • OS X
    • [ x] Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

};
}
</programlisting>

<para>The first argument, usually named <varname>self</varname>, corresponds to the final package
<para>The first argument, usually named <varname>pkgsself</varname>, corresponds to the final package
Copy link
Member

Choose a reason for hiding this comment

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

since when is it "usually named" `pkgsself``?

Copy link
Member

Choose a reason for hiding this comment

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

It is not, and while I think self should probably be named fixed in the long run, adding the pkgs prefix becomes quite verbose.

Copy link
Member

@Mic92 Mic92 Jun 5, 2017

Choose a reason for hiding this comment

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

apparently the same notation is used in doc/language-frameworks/python.md. It also imports from <nixpkgs there.

Copy link
Member

Choose a reason for hiding this comment

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

yep... #23669

@@ -96,4 +96,68 @@ overridden and/or new packages.</para>

</section>

<section xml:id="sec-overlays-extend-modify">
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, the instructions in this section seem more complicated to me than what has already been recommended in the preceding section. The manual has already stated how to add new packages or modify existing ones. Maybe turn this into an "extended example" or at least clarify how it is different than what precedes it.

};
} ) ]; };
in newpkgs
</programlisting>
Copy link
Member

Choose a reason for hiding this comment

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

Thanks, I think code snippets like this are helpful 👍

Copy link
Member

Choose a reason for hiding this comment

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

This snippet is not the recommended way to make overlays, because it is not modular. Also it is incorrect because it mention nixpkgs as one input. Your overlay should probably look like that instead:

self: super:

let
  path = if builtins.pathExists <nixcloud> then <nixcloud> else ../.;
in  

{
  nixcloud-backend  = super.callPackage (super.lib.cleanSource "${path}/nixcloud-backend") { };
  nixcloud-frontend = super.callPackage "${path}/nixcloud-frontend/release.nix" {
    nixcloud-editor = super.lib.cleanSource "${path}/nixcloud-editor";
  };
}

And be installed in the ~/.config/nixpkgs/overlays directory with a symbolic link, such that the path resolution can use a relative path.

@@ -30,10 +30,17 @@
<para>
Example usages:

The listing below can be used to change existing attributes like `src` or `buildInputs`:
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect, the pkg.override function allow you to change the argument given to the package function. If you want to change the src or the buildInputs attributes, then you would have to use overrideDerivation function.

<programlisting>pkgs.foo.override { arg1 = val1; arg2 = val2; ... }</programlisting>

The listing below is similar to the one above but applies to all pkgs in nixpkgs. It can be used to extend pkgs with new ones or alter exiting ones. Since there is `self` and `super` one can update a library package and add a program which uses it at the same time.
Copy link
Member

Choose a reason for hiding this comment

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

This paragraph does not distinguish clearly what self and super are, and how they help in doing what is suggested.

<programlisting>import pkgs.path { overlays = [ (self: super: {
foo = super.foo.override { barSupport = true ; };
})]};</programlisting>


This third example shows how to modify an input dependency.
Copy link
Member

@nbp nbp Jun 3, 2017

Choose a reason for hiding this comment

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

If you want to provide a description for each example, I would recommend to avoid using "the listing", "this ... example" as they are redundant.

I would suggest to reverse the statement the following way:

An override can be used to change a package, only for giving it as input of another, instead of applying the modification globally.

};
}
</programlisting>

<para>The first argument, usually named <varname>self</varname>, corresponds to the final package
<para>The first argument, usually named <varname>pkgsself</varname>, corresponds to the final package
Copy link
Member

Choose a reason for hiding this comment

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

It is not, and while I think self should probably be named fixed in the long run, adding the pkgs prefix becomes quite verbose.

nc-frontend = nixcloud-frontend;

newpkgs = import pkgs.path { overlays = [ (pkgsself: pkgssuper: {
nixcloud-backend = pkgs.callPackage nc-backend { inherit newpkgs; };
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned above in the documentation, this should be super.callPackage What you are doing it here is using the fix-point of the pkgs argument, to look for the inputs of nc-backend.

};
} ) ]; };
in newpkgs
</programlisting>
Copy link
Member

Choose a reason for hiding this comment

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

This snippet is not the recommended way to make overlays, because it is not modular. Also it is incorrect because it mention nixpkgs as one input. Your overlay should probably look like that instead:

self: super:

let
  path = if builtins.pathExists <nixcloud> then <nixcloud> else ../.;
in  

{
  nixcloud-backend  = super.callPackage (super.lib.cleanSource "${path}/nixcloud-backend") { };
  nixcloud-frontend = super.callPackage "${path}/nixcloud-frontend/release.nix" {
    nixcloud-editor = super.lib.cleanSource "${path}/nixcloud-editor";
  };
}

And be installed in the ~/.config/nixpkgs/overlays directory with a symbolic link, such that the path resolution can use a relative path.

If you want to install a package from this new set, do this:

<programlisting>
nix-build nixcloud-pkgs.nix -A nixcloud-backend
Copy link
Member

Choose a reason for hiding this comment

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

Once you created the overlays, as I mentioned in the previous comment and installed it, then you should be able to install the package like this:

$ nix-build -A nixcloud-backend

Note, that overlays are extending Nixpkgs by the fact that Nixpkgs import the overlays, not the opposite. Nixpkgs is made visible in your overlay code, by the mean of self and super, and thus should not be imported with import <nixpkgs> {} expression, which can be incorrect, as someone might be using nixpkgs in a cross compilation environment for example.

<programlisting>
{...}:
let
ncpkgs = import ./nixcloud-pkgs.nix {};
Copy link
Member

Choose a reason for hiding this comment

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

If you want to use your overlay in NixOS, then you must list it in the nixpkgs.overlays attribute, and then any of the packages you listed there would appear in the pkgs attribute provided to NixOS modules as argument.

Importing another <nixpkgs> would be wrong for the same reasons listed in my previous comment.

I will also note, that this is the Nixpkgs documentation and not the NixOS documentation. Thus this example should probably be moved to the NixOS documentation of overlays.

Copy link
Member

Choose a reason for hiding this comment

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

If I add overlays to global nixpkgs.overlays, they are not available by in my user environment. Is this intended?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the same as with packageOverrides, right? Those options pertain only to the package set used in the module eval context.

Copy link
Member

@Mic92 Mic92 Jun 5, 2017

Choose a reason for hiding this comment

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

I think I can use a global <nixpkgs-overlays> instead. No I can not use this for nixos-rebuild.

Copy link
Member

@Mic92 Mic92 Jun 5, 2017

Choose a reason for hiding this comment

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

Can import from <nixpkgs-overlays> in nixpkgs.overlays?

Copy link
Member

Choose a reason for hiding this comment

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

@nbp
Copy link
Member

nbp commented Jun 3, 2017

@qknight I see that you might have got inspiration from https://github.com/mozilla/nixpkgs-mozilla/ repository, but the default.nix file is not an overlay yet, and suffer from the same issues mentioned above. It makes use of overlays, such as the rust-overlay.nix file.

@qknight qknight closed this Jun 4, 2017
@bachp
Copy link
Member

bachp commented Jun 4, 2017

I'm sad to see people giving up on contributions to the manual. I myself found it pretty hard too and the comments were not very encouraging either, so I gave up too and closed the merge request.

Maybe we need to see if we can improve here?

@nbp
Copy link
Member

nbp commented Jun 4, 2017

@bachp What was not encouraging in the comments?

@qknight Can you update the stack overflow answers to have a pointer to this pull request?

@Mic92
Copy link
Member

Mic92 commented Jun 4, 2017

That's why we decided to review and edit contributions afterwards in the wiki instead of having a pull request system in place. My recommendation would be as we deal with written text instead of code to edit the content directly as a maintainer. Git is not really optimized for written text and the comments to request changes probably take longer to write/read then to change them directly.

@nbp
Copy link
Member

nbp commented Jun 4, 2017

@Mic92 Honestly, I prefer having a review process to catch errors as opposed to having documentation which is miss-leading into more complex solutions which would have to be explained later to a larger audience.

@qknight
Copy link
Member Author

qknight commented Jun 4, 2017

@nbp @bachp

What was not encouraging in the comments?
the answers were great but i had 3 windows open and a nix-build with the docu to understand what the comments could mean. i'm not good at looking at commented split snippets of PR code, this PR technique is not working well for me.
also my use case is different and 'not encouraged' so my contribution seems to be unwanted.

that said, my documentation of this feature is now documentet in this PR, so with a little google search one can find it. like i did with #23016

@nbp i've added the stackover-flow link to this PR. thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants