-
-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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 pkgs.nixos function #32422
Add pkgs.nixos function #32422
Conversation
23b1ac6
to
3ccd49a
Compare
pkgs/top-level/all-packages.nix
Outdated
import (pkgs.path + "/nixos/lib/eval-config.nix") { | ||
inherit modules system; | ||
pkgs = self; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reasons to not have short-cuts such as:
https://github.com/NixOS/nixpkgs/blob/master/nixos/default.nix#L35-L39
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nixos/default.nix
defines three shortcuts, system
, vm
and vmWithBootLoader
.
Adding system
overloads the meaning of 'system' for (at least?) the third time. It can already mean 'system architecture' or config.system
.
The vm
shortcut seems arbitrary, because vm
is only one of a number of useful attributes in config.system.build
. vmWithBootloader
is redundant because it only defines an option.
Also, these vm shortcuts may cause the configuration to be evaluated more than once, even when you're invoking nixosEvaluate
only once. This can be surprising. For example, when you write a function that returns vm
and config
, that looks very reasonable, but config
does not actually correspond to the configuration of vm
.
I prefer to keep this function clean. Perhaps, maybe, something like let result = ...eval-config...; in result // { inherit (result.config.system) build; }
might be future proof and unsurprising, but only if you agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good to me, to have { config, options, build }
, pkgs
should not be necessary as it is already exposed through config._module.args.pkgs
bd058bc
to
00504e0
Compare
@nbp, thanks for the review. I have taken the liberty to simplify it further, based on the insights:
As a result I believe the interface is simpler, correct and equally powerful.
|
00504e0
to
f1838f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This patch looks good, with the comment addressed.
I also think that this at least deserved to figure in the release notes with an example, as this might help a lot of users to add NixOS configurations to Hydra.
pkgs/top-level/all-packages.nix
Outdated
nixosEvaluate = configuration: | ||
(import (pkgs.path + "/nixos/lib/eval-config.nix") { | ||
inherit (pkgs) system; | ||
modules = [{ config._module.args.pkgs = self; }] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I understand that this is supposed to be a memory optimization, but in this particular case, this modification would have no effect because the _module.args
is an attribute set which is merged with the //
update operator.
Also, pinning a specific version of Nixpkgs is frequent in companies, and the intent of this line would prevent the pinning from having any effect. I guess, what you might do is add the nixpkgs.externalPkgs
option to nixos/modules/misc/nixpkgs.nix
and check that the user wants to use an external Nixpkgs if an another option is true.
What this module should do instead is set nixpkgs.externalPkgs
to self
, and let the configuration opt-in this optimization. Note, the reason of adding an externalPkgs
option should also benefit NixOps, which suffer from exactly the same issue.
So, I suggest to first remove this line, and do the externalPkgs
option and setting it here in a follow-up issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching my mistake.
I think we disagree on the purpose of the function and both purposes are legitimate, so maybe we should have two functions? Like
pkgs.nixos
- build a NixOS from current
pkgs
- pinned by pinning
pkgs
- implemented in terms of
nixosNew
withexternalPkgs
and assertions that preventconfig.nixpkgs.*
settings because they conflict. - does not introduce
NIX_PATH
dependency
pkgs.nixosNew
- build a NixOS from a full-blown configuration that defines its own
config.nixpkgs.*
- NixOS-style pinning with
config.nixpkgs.*
- evaluates a new NixPkgs based on config unless
externalPkgs
is set.externalPkgs
comes with documentation about ignoring otherconfig.nixpkgs.*
options. - does not introduce
NIX_PATH
dependency
I'll focus on externalPkgs
before continuing on this PR though.
f1838f8
to
be9f42a
Compare
68eac3f
to
e2f5af7
Compare
@nbp, I've added two changelog entries (also one for the I've also updated the code to use |
I've checked the rendering of the release notes. |
I suppose the label "9. needs changelog" can be removed. Is there anything else I can do? |
@nbp is anything missing? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds good to me! Please add a changelog entry, and it would be good to merge.
Thanks, and sorry for the delay.
pkgs/top-level/all-packages.nix
Outdated
inherit (pkgs) system; | ||
modules = [( | ||
{lib,...}: { | ||
config.nixpkgs.pkgs = lib.mkDefault self; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: config.nixpkgs.pkgs = lib.mkDefault pkgs;
Otherwise this would only capture packages from all-packages and none of the one added by any of the overlays. This is a defect of the all-packages.nix
file.
Seems like I've pushed an outdated version on the 5th. I've noticed the weird naming in |
@nbp The delay is no problem, except 18.03 has been branched off. Shall we add it to 18.03 as well? |
Sorry this got buried. Could you move the release notes to 18.09 and we can merge for that. (provided no objections by other people). |
@matthewbauer, done. Thanks for digging this up :) |
@matthewbauer this PR can be merged |
Similar PR for |
Any reason you get the |
@lheckemann Alternatively, we could make I would like to see your module use the |
There are some other useful bits in the top of the evaluation — Also, And finally, I can't take credit for the rescue_boot module, it's all @cleverca22 's work :) |
The same trick should work for anything that's accessible in a module, including The problem you describe is an instance of a more general pattern that exists in NixOS where some options mask others. There isn't much we can do with the current module system, because For @cleverca22's rescue boot module this is not a problem, because the rescue boot NixOS doesn't declare its own overlays; it will only declare |
I think I'd prefer removing |
@lheckemann Better late than never #62046 |
Motivation for this change
This introduces a new function to streamline the use of NixOS in the context of NixPkgs.
Applications of this function include
This is an area where Nix really shines, so for me and probably others, it was a surprise that a function like this was not available.
Although it's a small addition, it is easily discoverable (
pkgs.nixos<TAB>
in repl) and steers towards using NixOS without using the legacy options ineval-config.nix
.The default value for
system
ispkgs.system
, so in its current state, users on OS X have to pass asystem
value explicitly. I have considered introducing some logic to choose linux x86-64 automatically, but that might do more harm than good. It should be easy enough to pass it explicitly or override the default in an overlay.WIPconfig.nixpkgs.externalPkgs
optionexternalPkgs
(should fix current ordering mistake)split intorenamed topkgs.nixos
andpkgs.nixosNew
nixos
.pkgs.nixosNew
doesn't really usepkgs
, so adds little value and could be confusingadd assertions innot feasible at this point. Need to introspect priorities of definitions, which isn't possible now.pkgs.nixos
to prevent unintended use ofexternalPkgs
bothfunctionsThings done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in./result/bin/
)