lib: add flake information to lib.trivial #278522

lf- · 2024-01-03T17:08:11Z

Description of changes

This is a splitting of the now-rewritten lib parts off of #254405, with the hopes it will be easier to merge.

This adds lib.trivial.selfPath (name subject to discussion) and lib.isFlake, implemented via the overlay we use for versions. The purpose of lib.trivial.selfPath is as a way of incurring a dependency on this nixpkgs that is not affected by NixOS/nix#9428. Please see the commit messages for more context

Things done

Add a 👍 reaction to pull requests you find important.

IreneKnapp · 2024-01-08T05:26:24Z

Looks very solid. Minimal and effective.

lib/trivial.nix

lf- · 2024-01-09T13:03:54Z

(CI exploded due to github shenanigans)

infinisil

Overall looking good :)

lib/trivial.nix

infinisil · 2024-01-15T20:08:38Z

lib/flake-info.nix

@@ -16,5 +16,8 @@ finalLib: prevLib: # lib overlay
    versionSuffix =
      ".${finalLib.substring 0 8 (self.lastModifiedDate or "19700101")}.${self.shortRev or "dirty"}";
    revisionWithDefault = default: self.rev or default;
+
+    isFlake = true;
+    nixpkgsStorePathString = toString self;


Needs a comment why this is needed and correct.

use builtins.storePath to establish a dependency on this nixpkgs' sources

I believe we'll soon want this to be a path value. Nix 2.20 already includes a significant part of the lazy trees infrastructure, so a compatible and reproducible variation of that branch is on the horizon. That is, compatible and reproducible wrt traditional Nix expressions. I do not know if Flakes will change, but it may.

Normally toString should not be called on path values, because the result lacks a string context.
In this case, self is already string-coercible, thanks to self.outPath, so we don't seem to need the positive functionality of toString here. I think you've used it to isolate the "path-like" property and expose that in an encapsulated way. I believe this could be done more reliably by returning self.outPath, which has the same effect, and does not break if self.outPath is changed to a path value.

This will have the effect of passing along Nix 2.18's string with context, but naturally upgrade that to a path value if that becomes the new behavior.

Regardless of how we decide to implement this, I believe this attribute should not be part of the public interface, ie nixpkgsFlake.lib.trivial.nixpkgsStorePathString (nor that it is trivial. I think it's the worst name, or on par with "util". Should we have lib.flake? Maybe, but certainly not for this purpose.).
Presumably this is for pkgs.path, right? I don't see a reason for this to exist as a flake output, because flake consumers can do nixpkgsFlake + "/some/path" (not that they should reach into the internals, but that's a different conversation). That does need to be fixed somehow, but my last attempt did not go well: #153594 (comment).
Solutions that increase the interface area may well make it much harder to solve the problem properly, so I humbly ask you to remove this attribute.

No, the reasoning for why this is here is written down in the comment itself. Without this attribute, we cannot ship #254405.

That is why we are doing this. Maybe it should be somewhere internal, I'm indifferent about that. But we do need it for good UX in NixOS flake systems. This particular use case does involve copying the whole nixpkgs into the store, unavoidably, since it will wind up in a derivation; it's not intended to be used for imports as we can already do that with pkgs.path, modulesPath, etc etc.

I'm personally lightly pessimistic about lazy trees getting in, but regardless of my own views on this, it would be annoying to gate the UX improvement that many people want on a new Nix version that will take some time to stabilize and shake out the regressions.

I do acknowledge this is extremely cursed code that depends on a load of C++nix implementation details and I would like to minimize the amount that it will break in the foreseeable future.

We need some way to inject the dependency to NixOS so we can put the nixpkgs into the resulting closure so the UX of nix-shell and nix shell isn't awful. My last attempt was just injecting it directly into NixOS, but I saw this infrastructure that felt more appropriate.

But now I wonder if injecting it directly into NixOS is the least bad option; the disadvantage of that one is this infrastructure not being reusable and not working for non-flakes, but perhaps that's for the better since it's rather cursed?

depends on a load of C++nix implementation details

I don't think path value semantics can be considered implementation details.

Path values have been an integral part of the language since basically forever. Any Nix implementation needs to implement path value semantics. I don't think the lazy-trees author appreciated that when they wrote the branch.
The problems with self.outPath and in-flake ./. are squarely with Flakes.

gate the UX improvement that many people want on a new Nix version

This is unfortunate, but I don't think it's an excuse for making a mess in nixpkgs.

wonder if injecting it directly into NixOS is the least bad option

I'll take a messy implementation over a messy interface any day.

ship #254405.

For context, this can't be done correctly and I don't think we should try to pretend that we have a mechanism that forwards pkgs to other evaluations on the machine for which it was used. Usually I support incremental improvements, but if they can't lead to a good end state, the approach should be reconsidered in its entirety.

Hmmm wait a minute. Wait a minute. We can probably actually fix this properly by forwarding the metadata of the flake input locking into the registry entry. 🤦🏻‍♀️ which would actually do this properly and not add to closure size, since we can forward to the flake registry for NIX_PATH.

The only thing we lose there is sending nixpkgs to the new installation automatically so it works offline, rather than just merely putting the metadata in to get the right version lazily.

Hmm. Nope. You can't get info of your own lock file entry from inside a flake. This is .... probably reasonable, because it would enable some truly spectacular spooky action at a distance, but it is quite sad for this purpose.

In conclusion, we still need something like this.

infinisil

Looking good to me! I'm guessing @roberth probably also has an opinion on this, but otherwise I'll merge soon (ping me if I don't)

lib/trivial.nix

…xt for self This is an extremely subtle feature which is necessary to be able to incur dependencies on this nixpkgs's source code without copying it twice (NixOS/nix#9428, NixOS/nix#5868). pkgs.path is not sufficient, because actually using it incurs a copy, which winds up looking like /nix/store/HASH-HASH-source in flakes. Similarly, `toString pkgs.path` is not sufficient because that does not have any string context, so it will not incur a dependency if it's used. It's exceptionally subtle. There are four cases: - non flakes, pure mode: can't do anything about this, we must copy to the store. - non flakes, not already in the store: can't do anything about this, we are copying to the store. - non flakes, already in the store: storePath gives us a string with context for free. - flakes: overlay makes it a stringification of self.outPath. In all cases, this is a string with appropriate context to transfer this nixpkgs to another system.

This is intended to be used for making NixOS pick up that it is being used from a flake and configure the NIX_PATH and flake registry appropriately.

lf- · 2024-01-20T07:42:36Z

Poked at the comments some more.

roberth · 2024-01-20T08:06:18Z

lib/flake-info.nix

+    # this overrides lib/trivial.nix, in which isFlake is unconditionally set
+    # to false.
+    isFlake = true;


We have a bunch of dynamic type checking functions, such as isString, isPath, etc. This looks like that, and that's a problem, because flakes can be checked reliably with x._type or null == "flake", and it is important enough to warrant a function.

(and I think time is coming to revisit lib.types: Add flake type #210572 (comment))

Suggested change

# this overrides lib/trivial.nix, in which isFlake is unconditionally set

# to false.

isFlake = true;

# this overrides lib/trivial.nix, in which isFlake is unconditionally set

# to false.

isFlakeLib = true;

Note that this may suffer from a similar expectation, but that expectation is less realistic to be executed, unless we give lib a special treatment in Nix's call-flake.nix. I don't like that idea.

roberth · 2024-01-20T08:15:19Z

lib/flake-info.nix

@@ -16,5 +16,8 @@ finalLib: prevLib: # lib overlay
    versionSuffix =
      ".${finalLib.substring 0 8 (self.lastModifiedDate or "19700101")}.${self.shortRev or "dirty"}";
    revisionWithDefault = default: self.rev or default;
+
+    isFlake = true;
+    nixpkgsStorePathString = toString self;


use builtins.storePath to establish a dependency on this nixpkgs' sources

I believe we'll soon want this to be a path value. Nix 2.20 already includes a significant part of the lazy trees infrastructure, so a compatible and reproducible variation of that branch is on the horizon. That is, compatible and reproducible wrt traditional Nix expressions. I do not know if Flakes will change, but it may.

Normally toString should not be called on path values, because the result lacks a string context.
In this case, self is already string-coercible, thanks to self.outPath, so we don't seem to need the positive functionality of toString here. I think you've used it to isolate the "path-like" property and expose that in an encapsulated way. I believe this could be done more reliably by returning self.outPath, which has the same effect, and does not break if self.outPath is changed to a path value.

This will have the effect of passing along Nix 2.18's string with context, but naturally upgrade that to a path value if that becomes the new behavior.

Regardless of how we decide to implement this, I believe this attribute should not be part of the public interface, ie nixpkgsFlake.lib.trivial.nixpkgsStorePathString (nor that it is trivial. I think it's the worst name, or on par with "util". Should we have lib.flake? Maybe, but certainly not for this purpose.).
Presumably this is for pkgs.path, right? I don't see a reason for this to exist as a flake output, because flake consumers can do nixpkgsFlake + "/some/path" (not that they should reach into the internals, but that's a different conversation). That does need to be fixed somehow, but my last attempt did not go well: #153594 (comment).
Solutions that increase the interface area may well make it much harder to solve the problem properly, so I humbly ask you to remove this attribute.

roberth · 2024-01-20T08:58:30Z

lib/trivial.nix

+  /* Whether this nixpkgs is being used directly as a flake.
+
+     Note that this is not set for `import nixpkgs` from another flake; it is
+     only set if using `nixpkgs.lib` or the `lib` flake.
+
+     Type: Bool
+   */
+  isFlake =


This isn't just about how it's loaded, but about an architectural property.

Suggested change

/* Whether this nixpkgs is being used directly as a flake.

Note that this is not set for `import nixpkgs` from another flake; it is

only set if using `nixpkgs.lib` or the `lib` flake.

Type: Bool

*/

isFlake =

/* Whether this is the `nixpkgs.lib` library (`nixpkgs` being the Nixpkgs flake), or just the Nixpkgs library.

The name `lib` may refer to, among other things:

- the Nixpkgs library, or

- a `lib` attribute on any flake, which has the purpose of exposing functions that are relevant to the consumer of the flake.

The Nixpkgs flake uniquely combines both roles, and does so by extending its traditional library of non-building functions with extra attributes, such as `nixos` and `nixosSystem`.

Note that this is not set for `import nixpkgs` from another flake; it is

only set if using `nixpkgs.lib` or the `lib` flake.

Type: Bool

*/

isFlakeLib =

Ohh
I glossed over the Nixpkgs library flake. Should that even have isFlakeLib? I think not; that lib is / should be very very close to the traditional library.

The library flake should definitely not expose its parent directory. That was never the intended architecture before flakes, and it would perpetuate (well, relatively) the current inefficiency of subdirectory flakes.

Hmmm. I can see why this would be faulty. Putting it in pkgs is ugly for different reasons of (very) poor discoverability, since it's mixed into a delicious soup of packages in all-packages, which is annoying.

lf- · 2024-02-04T04:39:04Z

Closing since most of this is probably a bad idea.

lf- requested a review from infinisil as a code owner January 3, 2024 17:08

github-actions bot added 6.topic: flakes The experimental Nix feature 6.topic: lib The Nixpkgs function library labels Jan 3, 2024

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jan 3, 2024

infinisil reviewed Jan 8, 2024

View reviewed changes

lib/trivial.nix Outdated Show resolved Hide resolved

infinisil reviewed Jan 8, 2024

View reviewed changes

lib/trivial.nix Outdated Show resolved Hide resolved

lf- force-pushed the jade/lib-flake-info branch from 575987d to 07d87e8 Compare January 9, 2024 13:00

ofborg bot added the ofborg-internal-error Ofborg encountered an error label Jan 9, 2024

lf- force-pushed the jade/lib-flake-info branch from 07d87e8 to 6e58051 Compare January 9, 2024 13:06

cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 9, 2024

infinisil requested changes Jan 15, 2024

View reviewed changes

lf- force-pushed the jade/lib-flake-info branch 2 times, most recently from 33e6358 to 0cf661a Compare January 19, 2024 00:56

lf- requested a review from infinisil January 19, 2024 00:57

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Jan 19, 2024

infinisil approved these changes Jan 19, 2024

View reviewed changes

lib/trivial.nix Outdated Show resolved Hide resolved

lib/trivial.nix Show resolved Hide resolved

lf- added 3 commits January 19, 2024 23:40

lib/trivial: add isFlake information

ab35850

This is intended to be used for making NixOS pick up that it is being used from a flake and configure the NIX_PATH and flake registry appropriately.

lib.trivial: add test for nixpkgsStorePathString

4c934f1

lf- force-pushed the jade/lib-flake-info branch from 0cf661a to 4c934f1 Compare January 20, 2024 07:42

roberth requested changes Jan 20, 2024

View reviewed changes

lf- closed this Feb 4, 2024

lf- mentioned this pull request Feb 4, 2024

nixos/flake: set up NIX_PATH and system flake registry automatically #254405

Merged

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib: add flake information to lib.trivial #278522

lib: add flake information to lib.trivial #278522

lf- commented Jan 3, 2024 •

edited

Loading

IreneKnapp commented Jan 8, 2024

lf- commented Jan 9, 2024

infinisil left a comment

infinisil Jan 15, 2024

roberth Jan 20, 2024

lf- Jan 20, 2024

lf- Jan 20, 2024 •

edited

Loading

roberth Jan 20, 2024

roberth Jan 20, 2024

lf- Jan 20, 2024

lf- Jan 22, 2024

infinisil left a comment

lf- commented Jan 20, 2024

roberth Jan 20, 2024

roberth Jan 20, 2024

roberth Jan 20, 2024

roberth Jan 20, 2024

roberth Jan 20, 2024

lf- Jan 20, 2024

lf- commented Feb 4, 2024

lib: add flake information to lib.trivial #278522

lib: add flake information to lib.trivial #278522

Conversation

lf- commented Jan 3, 2024 • edited Loading

Description of changes

Things done

IreneKnapp commented Jan 8, 2024

lf- commented Jan 9, 2024

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf- Jan 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

lf- commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lf- commented Feb 4, 2024

lf- commented Jan 3, 2024 •

edited

Loading

lf- Jan 20, 2024 •

edited

Loading