disk-image: Fix "unexpected argument 'customQemu'" eval error #851
Conversation
|
|
||
| vmToolsSupportsCustomQemu :: pkgs -> bool | ||
| */ | ||
| vmToolsSupportsCustomQemu = pkgs: lib.versionAtLeast pkgs.lib.version "24.11.20240709"; |
There was a problem hiding this comment.
I think it's good enough to assume people are either on a release channel or on the latest unstable one, so no need to check for the date IMO.
And while I really appreciate the detailed description, I do think it's a bit verbose for an inline comment and the commit message might be a better place to explain how it came to be?
There was a problem hiding this comment.
I think it's good enough to assume people are either on a release channel or on the latest unstable one, so no need to check for the date IMO.
I disagree. The entire nixpkgs revision history lasts forever. People might use outdated versions for example to bisect compatibility issues upstream, doing research or to get a specific version of a package. The qemu option is simply incompatible with a certain range of nixpkgs. I found what that range was, and this function encapsulates that knowledge as good as is feasible. I don't think I should actively write a more incorrect implementation of this check. What would be the benefit of that?
And while I really appreciate the detailed description
Thank you :)
I do think it's a bit verbose for an inline comment and the commit message might be a better place to explain how it came to be?
Why? The contribution experience for developers depends on good documentation. We're not generating formatted docs from these comments yet, but they are de-facto documentation and IMO should be of the quality that we would expect from documentation for developers.
Again, I don't understand what would be the benefit of shortening this comment? If you can think of a more terse formulation, I'm all for it, but the fact that this function even exists bears an explanation, and if I saw something like this, I would appreciate it being documented.
There was a problem hiding this comment.
I disagree. The entire nixpkgs revision history lasts forever. People might use outdated versions for example to bisect compatibility issues upstream, doing research or to get a specific version of a package. The qemu option is simply incompatible with a certain range of nixpkgs.
Sure, but the current state of things is that diskos master is broken for the latest official release (without this PR) and we didn't catch that in CI or otherwise until @Nebucatnetzer ran into it at NixCon. So I don't think supporting each and every nixpkgs commit is a realistic goal and therefore would rather focus on actually supporting both -unstable and the latest stable release.
No big deal to keep the date in, I don't care too much about it as long as it's just a single check and not an if-else-tree of date-specific clauses ;)
Again, I don't understand what would be the benefit of shortening this comment?
I am not arguing against documentation :D the first and last line of that comment are totally fine. The 2 paragraphs in between would IMO be better placed in the commit message, because the do provide historical context. Comments should tell me what that function does and possible pitfalls. I think its good to keep documentation somewhat consistent and don't think we document the genesis of other values anywhere? (And neither do I think we should).
But again, no strong feelings. I'd rather have VMs on diskos master branch work again with nixos stable sooner than later and don't want to block anything here.
There was a problem hiding this comment.
Alright, I think that makes sense. I shortened the two paragraphs to a single note that explains that the function isn't perfectly accurate for commits on 2024-07-08, which is everything you really need to know to use it properly, and moved the two paragraphs to the commit message instead :)
There was a problem hiding this comment.
until @Nebucatnetzer ran into it at NixCon
oh no, was it during a live demo? 😬
There was a problem hiding this comment.
No I just wanted to try it on the hackday on Sunday.
The usual “the user touched the product” 😁.
There was a problem hiding this comment.
Haha, good question. My second demo did actually fail (got stuck at lsblk) but I have yet to investigate why
There was a problem hiding this comment.
@Nebucatnetzer just to be sure, did you use the latest disko from this flake, or whatever was packaged in nixpkgs 24.05? I'm preparing a release right now to make the fix public and will create PRs for nixpkgs as well, and was wondering whether this should be backported to 24.05.
iFreilicht
force-pushed
the
fix-eval-error-unexpected-argument-customQemu
branch
from
6946279 to
53e2e74
Compare
Previously, an error like
error: function 'anonymous lambda' called with unexpected argument 'customQemu'
at /nix/store/lbqj1cndic4121whnx8xm0jgb1c8x4xx-source/pkgs/build-support/vm/default.nix:1:1:
was printed when trying to evaluate `config.system.build.vmWithDisko` or
`config.system.build.diskoImagesScript` with nixpkgs 24.05 (and below).
This argument was added in
NixOS/nixpkgs@65c851c,
so technically the minimum version is 24.11.20240708.65c851c. However,
`versionAtLeast` only compares versions alphabetically, so the comparison's
result will be incorrect for other commits made on the same day.
Instead, we compare against 24.11.20240709, which is the next day.
This means this function returns false for some commits that DO support the
customQemu argument, but if it returns true, we can be 100% certain that this
is correct, and we can pass the customQemu argument to vmTools without an
evaluation error.
Fixes #850
iFreilicht
force-pushed
the
fix-eval-error-unexpected-argument-customQemu
branch
from
53e2e74 to
bba79f6
Compare
|
@Nebucatnetzer just to be sure, did you use the latest disko from this flake, or whatever was packaged in nixpkgs 24.05? I'm preparing a release right now to make the fix public and will create PRs for nixpkgs as well, and was wondering whether this should be backported to 24.05.
I used the flake.
|
Previously, an error like
was printed when trying to evaluate
config.system.build.vmWithDiskoorconfig.system.build.diskoImagesScriptwith nixpkgs 24.05 (and below).Fixes #850