Draft for overlays using flake.nix `outputs.flakeModule` by LiGoldragon · Pull Request #222155 · NixOS/nixpkgs

LiGoldragon · 2023-03-20T09:01:41Z

Description of changes

A draft for overlay using flake modules as developped by
flake-parts. A pull
request for flake-parts will complement this for testing soon.

Things done

(removed (dependencyOn flake-parts)) (addedOption `nixpkgs.output`)])

roberth

Hi, and thank you for proposing a contribution!

This is quite a significant addition, so I've made thorough first review as you can see.

I think your commit messages are really interesting. Do you have tooling to process those sexprs?
We do have some guidelines for the format of commit messages. Would you be willing to adapt to those? They're listed here. They're probably not hugely specific rules for these commits, but I know that the other contributors will appreciate commit messages that fit in with the rest; certainly for the title part of the message.

I'd also like to ask you to squash your commits in the end, but keep a commit boundary between changes that could be used separately, such as for example the addition of types.overlay.

flake.nix

roberth · 2023-03-21T23:42:27Z

flakeModule.nix

+    merge = _loc: defs:
+      let
+        logWarning =
+          if builtins.length defs > 1


If you could implement support for mkOrder, that'd be awesome, but otherwise, I'd be ok to just note this and consider this to be MVP.

Suggested change

if builtins.length defs > 1

# TODO: do not warn when lib.mkOrder/mkBefore/mkAfter are used unambiguously

if builtins.length defs > 1

roberth · 2023-03-21T23:45:27Z

flakeModule.nix

+            (builtins.seq
+              logWarning


You could use lib.warnIf to replace the seq + if combination.

flakeModule.nix

roberth · 2023-03-21T23:48:48Z

flakeModule.nix

+        description = "Nixpkgs overlays";
+        default = [ ];
+      };
+


You could add the Nixpkgs config parameter if you like.

Yes, that was already planned.

roberth · 2023-03-21T23:51:12Z

flakeModule.nix

+let
+  inherit (lib) mkOption types;
+
+  nixpkgsOverlaySubmodule = types.mkOptionType {


It would be great to have this in lib.types instead of hidden away in a let binding.
That way we can also reuse the existing test infrastructure in lib/tests/modules.sh to test this type.

Suggested change

nixpkgsOverlaySubmodule = types.mkOptionType {

nixpkgsOverlayType = types.mkOptionType {

Just a type, not really a submodule. Confused me for a sec when I read it below.

Yep, the nixos module will have to be merged with this, to avoid duplication. It is indeed a big change, structurally, but you already did most of the discovering with flake-parts. Good catch on the name. I copied a lot from flake-parts, so will be putting you in the authors of the final squash.

roberth · 2023-03-22T00:05:05Z

flakeModule.nix

+    options = {
+      overlays = mkOption {
+        type = listOf nixpkgsOverlaySubmodule;
+        description = "Nixpkgs overlays";


A link to the Nixpkgs manual would be helpful.

Hi, and thank you for proposing a contribution!

This is quite a significant addition, so I've made thorough first review as you can see.

I think your commit messages are really interesting. Do you have tooling to process those sexprs? We do have some guidelines for the format of commit messages. Would you be willing to adapt to those? They're listed here. They're probably not hugely specific rules for these commits, but I know that the other contributors will appreciate commit messages that fit in with the rest; certainly for the title part of the message.

I'd also like to ask you to squash your commits in the end, but keep a commit boundary between changes that could be used separately, such as for example the addition of types.overlay.

Yes, those are a proof of concept lang being developped dynamically. They will be processed eventually, when the first sajban implementation is out. For sure, I will learn about NixOS style commits and squash this PR when done. This is just a documenting PR. Thank you so much for flake-parts and for your review. I will be sending you some ouf our tokens when they come out in a few weeks. I'd be happy to have you on a live podcast to discuss Nix and its future. Cheers.

roberth · 2023-03-22T00:07:26Z

flakeModule.nix

Considering that the logic in this file is specific to the Nixpkgs part of the repo, could you move it into pkgs/top-level?

You mean the whole module? I was also wondering wether or not to bypass the impurity checks, since flakes are supposed (as far as I know) to be pure. Any thoughts?

Indeed it shouldn't matter much. You can pass --impure to override the pure behavior, but I don't think we should support any of the environment variables if that causes more complexity or effort. Users should pass any overrides and such in their expressions, and not through the environment, because most likely it affects more code than they wish for.

(added (flakeModule allPkgsPerSystem))]

LiGoldragon · 2023-03-22T04:42:47Z

The review done at this time were done in a live on youtube around 1:30:00

[(removed import) (added documentingComments)])

LiGoldragon · 2023-03-23T09:08:40Z

The draft is ready for eager testing. It is used currently in a fork of flake-parts.
https://github.com/hercules-ci/flake-parts/blob/9c9ec49c197754a719f4546dc121ecdaab121ce5/modules/nixpkgs.nix#L23

and you can see a naive test here:
https://github.com/sajban/tests/blob/8b582dab126811ce41bc76d1dc705aa0d0ff8728/nix/perSystem.nix#L9
which overrides the following mkFlake import, so hello should become bash and not mksh
https://github.com/sajban/tests/blob/8b582dab126811ce41bc76d1dc705aa0d0ff8728/nix/flakeModule.nix#L8

It will be used soon in clojix, which is an empty repo at time of writing.

LiGoldragon · 2023-03-23T10:44:23Z

As an aside there is an argument to be made that NixOS' nixpkgs.nix module should really be called pkgs.nix along with the option (nixpkgs -> pkgs). Perhaps, also to avoid option name clashes with NixOS, the introduced option could be called pkgs, and assume purity, thus avoiding the cost of evaluating the impurity wrapping code on pkgs.

LiGoldragon · 2023-04-01T02:42:26Z

I have gone ahead and bypassed impure.nix. There seems to be a bug with optional arguments in functions that are passed through a module option. This introduces the nixpkgs.mkPkgsFromArgs which does not support the legacy system argument, but by default will use builtins.currentSystem, which I don't know the behaviour of and/or plans for. Thanks in advance. This module is being used by clojix.

LiGoldragon · 2023-04-01T02:43:17Z

You can see how flake-parts uses this here.

roberth · 2023-04-01T16:16:17Z

I have gone ahead and bypassed impure.nix.

👍

There seems to be a bug with optional arguments in functions that are passed through a module option.

Optionality in module arguments themselves is a bit weird and unsupported, but if the value of module argument is a function, you should just get that function and no special behavior. In other words, if you have

{ hi, ... }:
{ config._module.args = f; }

Then hi should be identical to f.

This introduces the nixpkgs.mkPkgsFromArgs which does not support the legacy system argument, but by default will use builtins.currentSystem, which I don't know the behaviour of and/or plans for. Thanks in advance. This module is being used by clojix.

If it relies on builtins.currentSystem it won't work in pure mode, which really should be supported.

roberth · 2023-04-01T16:22:23Z

As an aside there is an argument to be made that NixOS' nixpkgs.nix module should really be called pkgs.nix along with the option (nixpkgs -> pkgs).

Not sure about pkgs.nix, as I'd read that as the nix package, but in theory I'd like pkgs, because setting that module argument is the goal of the module.

Perhaps, also to avoid option name clashes with NixOS, the introduced option could be called pkgs,

A clean slate. I see the appeal, but it might actually make the migration harder rather than easier. Realistically, we'll want to improve the individual options and take our time to do it, but I doubt that it's really worth renaming. Probably best to let the ecosystem complete a system+*System -> {host,build}Platform migration first.

and assume purity, thus avoiding the cost of evaluating the impurity wrapping code on pkgs.

This evaluation cost is negligible, especially in comparison with all the things that have to be evaluated just to get to a stdenv.

LiGoldragon · 2023-04-02T08:54:27Z

Thanks Robert, your comments are welcome and encouraging. I will polish this PR as soon as I start using it, which I am working on achieving with all my coding time.

LiGoldragon · 2023-08-07T01:43:23Z

Any future work related to this will probably start from #231940

(drafted (flake.nix outputs.flakeModule))

7097865

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Mar 20, 2023

li added 2 commits March 20, 2023 18:06

(movedToFile (flake.nix outputs.flakeModule))

4dc2aea

(developped flakeModule [smallFixes

75c31d0

(removed (dependencyOn flake-parts)) (addedOption `nixpkgs.output`)])

ghost mentioned this pull request Mar 21, 2023

Draft for nixpkgs flake module hercules-ci/flake-parts#126

Closed

roberth reviewed Mar 22, 2023

View reviewed changes

[(renamed flakeModule flakeModules.default)

c295813

(added (flakeModule allPkgsPerSystem))]

li added 3 commits March 23, 2023 08:44

(appliedSuggestionsFrom roberth

54766d1

[(removed import) (added documentingComments)])

(firstEvaluation (naiveWorkingTest flakePartsModule))

048003b

[(renamed nixpkgsOverlayType) (added mkPkgsFromArgs)]

7028c36

(appliedSuggestionsFrom roberth (renamed flakeMOdule.nix))

2613e69

LiGoldragon force-pushed the overlayedFlakes branch from 31fa2dd to 2613e69 Compare March 23, 2023 10:39

(fixed [byPassOfNixpkgsImpureEval missingArguments])

b4e3577

LiGoldragon force-pushed the overlayedFlakes branch from 07ab644 to b4e3577 Compare April 1, 2023 02:40

Janik-Haag added the 12.first-time contribution This PR is the author's first one; please be gentle! label Jun 13, 2023

LiGoldragon closed this Aug 7, 2023

	if builtins.length defs > 1
	# TODO: do not warn when lib.mkOrder/mkBefore/mkAfter are used unambiguously
	if builtins.length defs > 1

	nixpkgsOverlaySubmodule = types.mkOptionType {
	nixpkgsOverlayType = types.mkOptionType {

Uh oh!

Conversation

LiGoldragon commented Mar 20, 2023

Description of changes

Things done

Uh oh!

roberth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LiGoldragon commented Mar 22, 2023

Uh oh!

LiGoldragon commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiGoldragon commented Mar 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LiGoldragon commented Apr 1, 2023

Uh oh!

LiGoldragon commented Apr 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roberth commented Apr 1, 2023

Uh oh!

roberth commented Apr 1, 2023

Uh oh!

LiGoldragon commented Apr 2, 2023

Uh oh!

LiGoldragon commented Aug 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LiGoldragon commented Mar 23, 2023 •

edited

Loading

LiGoldragon commented Mar 23, 2023 •

edited

Loading

LiGoldragon commented Apr 1, 2023 •

edited

Loading