flake: fix lib.trivial.version when used from a flake
#257100
Conversation
ofborg
bot
added
10.rebuild-darwin: 0
flake.nix
Outdated
| overlays.default = final: prev: { | ||
| lib = prev.lib.extend libVersionInfoOverlay; | ||
| }; |
There was a problem hiding this comment.
Not sure if this should be exposed, and if so, I don't know about default - maybe setLibVersion? Shouldn't this always be pre-applied? Users shouldn't have to care about this.
I guess import nixpkgsFlake { } will not work like that, so we should provide an alternative, such as nixpkgsFlake.pkgs { }.
There was a problem hiding this comment.
I don't know about default - maybe setLibVersion?
Yeah, that's a bad name, yours seems better.
Shouldn't this always be pre-applied? Users shouldn't have to care about this.
It is pre-applied when using inputs.nixpkgs.legacyPackages.
If you use import inputs.nixpkgs I'd expect that default.nix is evaluated in pure mode for me, i.e. no additional flake involvement.
so we should provide an alternative, such as nixpkgsFlake.pkgs { }.
I'm not sure I follow here, what exactly do you mean?
Given the expectation I described above, I think it's OK though to not apply overlays magically here. And isn't it under discussion whether import inputs.foo (i.e. as a path) should be deprecated anyways?
There was a problem hiding this comment.
Fixed the naming of the overlay.
Won't resolve though until we're aligned on how to deal with import inputs.nixpkgs.
There was a problem hiding this comment.
Is the naming OK and the fact that you'd have to apply this manually when doing import inputs.nixpkgs @roberth? Also, anything else to discuss / fix? :)
There was a problem hiding this comment.
I think we should discourage import inputs.nixpkgs, because it just can't work well. It's just not something that can ever work as well as the actual flake outputs, unless flakes allows outPath to be a __functor function. I really don't know what to think of that... It should probably be an explicit feature of flakes and not something that allows arbitrary stuff to be put in outPath. And even then it's kind of bad because of import "${inputs.nixpkgs}".
Ignoring all that ugliness for now, I think the nixpkgs should provide a proper lib-based entrypoint into Nixpkgs, just like we have lib.nixosSystem. That way it can actually work well, and not be a strange experience. (import a flake, really?)
Some possible names
lib.nixpkgs: not a fan because Nixpkgs should be more than just the function that returns "pkgs"lib.callNixpkgs: arbitrary enough that we don't paint ourselves into a corner just yet
We should probably get the arguments right this time, along the lines of
{ hostPlatform, buildPlatform ? hostPlatform, config , overlays }: instead of the unholy mess that we have in default.nix. Maybe default.nix should stay bad, as a motivator for people to use the better entrypoint?
(As mentioned in the review, I think we should solve all these problems in a new PR after merging this one, and not have overlays in the outputs for this)
Ma27
force-pushed
the
version-info-lib
branch
from
9209b2e to
11b4242
Compare
|
gentle ping @roberth |
ofborg
bot
added
10.rebuild-linux: 1-10
and removed
10.rebuild-linux: 0
There was a problem hiding this comment.
Only nit picks. Would like another reviewer to have a look.
Maybe @infinisil or @lheckemann have opinions?
|
I'll take a look later today 👍 |
lib/__flake-version-info.nix
Outdated
| versionSuffix = | ||
| ".${finalLib.substring 0 8 (self.lastModifiedDate or self.lastModified or "19700101")}.${self.shortRev or "dirty"}"; | ||
| version = finalLib.trivial.release + finalLib.trivial.versionSuffix; | ||
| revisionWithDefault = default: self.rev or default; |
There was a problem hiding this comment.
This is partially duplicated code and is not very re-usable. Instead we can add attributes like these to proper lib.trivial:
lib.trivial.lastModifiedDate(nolastModifiedsince now Flakes always providelastModifiedDate), defaulting tonull.lib.trivial.revision(maybe also ashortRev, but not necessary since it can easily be computed), defaulting to whatrevisionWithDefaultcurrently defaults to.
Then there's no need to have any implementation here, because it can be moved to the original definitions for versionSuffix and revisionWithDefault. You will have to remove the rec from lib/trivial.nix though in order for version to be updated correctly.
Then this here can just be
{
lastModifiedDate = self.lastModifiedDate;
revision = self.rev;
}This also allows us to test and document this functionality in lib/tests, which I really think should be done.
There was a problem hiding this comment.
trivial
I don't think this is insignificant or self-evident.
re-usable
Not sure if that should be a goal here.
test and document this functionality
Good point, but lib/tests can import a file like this to achieve that.
There was a problem hiding this comment.
like these to proper lib.trivial:
To make sure I'm not misunderstanding you: you're talking about lib/trivial.nix itself, correct?
no lastModified since now Flakes always provide lastModifiedDate
Good point. Was there for historical reasons only I guess.
You will have to remove the rec from lib/trivial.nix though in order for version to be updated correctly.
That can be done independently of the other changes you suggested here IMHO.
That said, the downside is that you'll have to touch the entire code affecting .version-suffix because that's where the {lastmodified}.{shortrev} thing actually comes from in the non-flake case.
but not necessary since it can easily be computed
Not necessarily because git IIRC determines a length that makes each short rev unique (I think hardcoding a short-rev to e.g. 8 may cause a few collisions already).
There was a problem hiding this comment.
you're talking about
lib/trivial.nixitself, correct?
Yes.
That said, the downside is that you'll have to touch the entire code affecting
.version-suffixbecause that's where the{lastmodified}.{shortrev}thing actually comes from in the non-flake case.
No I'm thinking to change the definition of revisionWithDefault
Lines 212 to 221 in 0a1051c
if condition to use lib.trivial.revision if it's not null. But I'm not insisting on that, I think it's okay the way this PR is now.
Not necessarily because git IIRC determines a length that makes each short rev unique (I think hardcoding a short-rev to e.g. 8 may cause a few collisions already).
Ah true, I guess the short revs would still be prone to collisions over longer time though. I'd prefer a sufficiently long constant short rev length that is reasonably collision resistant for the future.
There was a problem hiding this comment.
to have an extra if condition to use lib.trivial.revision if it's not null. But I'm not insisting on that, I think it's okay the way this PR is now.
Hmm I don't think I like that: having an attribute that only works magically when using flakes and that provides a revision despite all the other attributes for that seems unintuitive to me at best.
IMHO redesigning this version code here is another topic and the what we have here is quite an improvement already, so I'd actually prefer the current version for now.
|
Implemented a few of the suggestions, namely
Re introducing As mentioned above, I'll update the documentation about the corner cases of flake's versionSuffix when everything is done (in the default case it behaves the same as when using channel tarballs). |
A lot of fetchers from Nix's own `libfetchers` also provide the
information that `lib.trivial` aims to expose with
`version`/`versionSuffix`/`revision`. In fact you don't even need a
`nixpkgs` channel to get a proper version suffix because of that!
Unfortunately this isn't used currently. When using the
nixpkgs flake, but not `nixpkgs.lib.nixosSystem` to build a NixOS
configuration, the version will always be `YY.MMpre-git`. One example is
e.g. `colmena` which evaluates configurations via
`import (npkgs.path + "/nixos/lib/eval-config.nix")`.
This patch ensures that the version suffix (i.e. the normalized last
modified date + git revision) is correctly exposed in `lib.trivial`.
Additionally, the change is injected into the following locations:
* `lib`: with that, something like
$ nix eval nixpkgs#lib.trivial.version
23.05.20230921.cf8bf79
is working fine (i.e. rather than `23.05pre-git`).
* `legacyPackages` to make sure that e.g. `legacyPackages.<system>.nixos`
has correct version info. This also applies to everything else using
`pkgs.lib.trivial` for that purpose.
* `overlays.default` which can be applied to a `nixpkgs` and changes the
previous `pkgs.lib` from said `nixpkgs` to also contain the correct
`version`/`revision`/etc..
This is useful for people using `nixpkgs` as flake input, but
importing it manually with
import inputs.nixpkgs { }
Co-authored-by: Linus Heckemann <git@sphalerite.org>
This effectively means that
nixpkgs$ nix eval ./lib#lib.trivial.version
"23.11.20231020.ee0d6b5"
now gives meaningful results as well.
See NixOS#257100 (comment)
for the discussion around this.
Ma27
force-pushed
the
version-info-lib
branch
from
16579eb to
29ede03
Compare
|
Fixed @infinisil's remark, rebased onto master and also fixed an evaluation error in Any final remarks or can we merge @roberth @lheckemann @infinisil ? |
There was a problem hiding this comment.
Can merge after implementing the suggestions.
Scoping out nixpkgs entrypoint problems for now.
lib/trivial.nix
Outdated
| @@ -446,7 +446,7 @@ rec { | |||
| annotated with function args. | |||
| */ | |||
| isFunction = f: builtins.isFunction f || | |||
| (f ? __functor && isFunction (f.__functor f)); | |||
| (f ? __functor && lib.trivial.isFunction (f.__functor f)); | |||
There was a problem hiding this comment.
| (f ? __functor && lib.trivial.isFunction (f.__functor f)); | |
| (f ? __functor && isFunction (f.__functor f)); |
Performance regression. Use let inherit (lib) isFunction; near start of file.
There was a problem hiding this comment.
Please do the same for the rest - at least for everything that doesn't perform I/O like warn.
There was a problem hiding this comment.
Done.
Btw I'm curious, is "dereferencing" an attribute-set really that more expensive? (not that deep into the evaluator I must admit 😅 )
There was a problem hiding this comment.
Generally no, it'd only be measurable if it's in a hot path, but functions like isFunction probably are. Not worth risking.
Also less noise.
That way each expression uses the final version of other lib.trivial declarations. For instance, when replacing `versionSuffix` with the string `"fnord"` in a lib overlay, `trivial.version` uses `"fnord"` as suffix now rather than `pre-git`.
* Improves the comments of `lib/flake-version-info.nix` and drops the `__`-prefix from the filename. * `lib'` -> `lib0` in `nixpkgs/lib`. * Drop the declaration of `trivial.version` in the overlay because this declaration already uses the final expressions of `versionSuffix` and `release` now. * No need to fall back to `self.lastModified` anymore, this was a workaround for pre2.4 Nix. Co-authored-by: Robert Hensing <robert@roberthensing.nl> Co-authored-by: Silvan Mosberger <contact@infinisil.com>
We cannot pass `overlays = ...` to `nixpkgs` directly because by default overlays from `~/.config/nixpkgs` are loaded in there. This doesn't happen by default, but when using `--impure`. Explicitly specifying that ignores these overlays. By using `pkgs.extend` the old behavior can be kept and the new overlay can be applied. Co-authored-by: Silvan Mosberger <contact@infinisil.com>
It's not really a pkgs overlay, but a lib overlay.
Ma27
force-pushed
the
version-info-lib
branch
from
29ede03 to
b96ba98
Compare
This effectively means that
nixpkgs$ nix eval ./lib#lib.trivial.version
"23.11.20231020.ee0d6b5"
now gives meaningful results as well.
See NixOS/nixpkgs#257100 (comment)
for the discussion around this.
This effectively means that
nixpkgs$ nix eval ./lib#lib.trivial.version
"23.11.20231020.ee0d6b5"
now gives meaningful results as well.
See NixOS/nixpkgs#257100 (comment)
for the discussion around this.
Description of changes
A lot of fetchers from Nix's own
libfetchersalso provide the information thatlib.trivialaims to expose withversion/versionSuffix/revision. In fact you don't even need anixpkgschannel to get a proper version suffix because of that!Unfortunately this isn't used currently. When using the nixpkgs flake, but not
nixpkgs.lib.nixosSystemto build a NixOS configuration, the version will always beYY.MMpre-git. One example is e.g.colmenawhich evaluates configurations viaimport (npkgs.path + "/nixos/lib/eval-config.nix").This patch ensures that the version suffix (i.e. the normalized last modified date + git revision) is correctly exposed in
lib.trivial. Additionally, the change is injected into the following locations:lib: with that, something likeis working fine (i.e. rather than
23.05pre-git).legacyPackagesto make sure that e.g.legacyPackages.<system>.nixoshas correct version info. This also applies to everything else usingpkgs.lib.trivialfor that purpose.overlays.defaultwhich can be applied to anixpkgsand changes the previouspkgs.libfrom saidnixpkgsto also contain the correctversion/revision/etc..This is useful for people using
nixpkgsas flake input, but importing it manually withThings done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)