nixos-generate-config: add Rust stub with tests #91353

Mic92 · 2020-06-23T12:18:15Z

Motivation for this change

our current nixos-generate-config comes without test and strong typing, which makes it hard to refactor given that it also depends on specific hardware
It depends on perl at run-time, which adds quite a bit of closure overhead that could be avoided
Perl has the connotation of a write right only-language which could be avoided by using strictly modern perl features. However this knowledge is not available in our community.
evidently I have received high-quality code reviews in the past in nixpkgs when submitting Rust code, indicating that community knows Rust well and can contribute to it.
Alternatives to Perl might be other languages i.e. Python/Bash has been considered but rules out:
- Python: - Also comes with a big run-time closure, + Is well understood by the community, +has optional typing, In the past attempts to rewrite other Perl scripts in Python has been rejected
- bash: + small dependency closure, - poor builtin string parsing capabilities/data structures/types

The goal of this PR is:

add hardware-independent, fast tests by mocking the environment to make it easy to refactor
In a followup PR this port should detect more hardware features in particular:
- detect CPU and apply appropriate profiles (microcode updates, Intel-graphic drivers)
- detect storage type (SSD -> change swap settings)
- detect dedicated graphic cards and apply drivers.
- detect laptops and apply energy saving settings.
in a more far-away future it might also detect buggy hardware and apply workarounds.

Please use this PR only for code review and discuss high-level design choices in RFC 70

nixos/modules/installer/tools/nixos-generate-config/Cargo.toml

Mic92 · 2020-06-23T12:31:33Z

nixos/modules/installer/tools/nixos-generate-config/.envrc

@@ -0,0 +1 @@
+use nix


This one should also go away.

Mic92 · 2020-06-23T12:31:56Z

nixos/modules/installer/tools/nixos-generate-config/default.nix

+  ];
+
+  buildPhase = ''
+    ln -sfn ${vendoredCrates}/vendor/ vendor


See Ma, no cursed fixed-input derivation!

Also dropped some unnecessary/one-off imports (such as `use std::string::String`, which is included in the default prelude). For rationale around using write{,ln}! over print{,ln}!, see BurntSushi/advent-of-code#17 -- print! and friends can panic.

I don't see us needing any additional binaries for this, so a folder dedicated to a single binary seems weird.

Because we print the binary name and the relevant error when e.g. `show_help()` or `parse_options()` errors out, we don't need to add the binary name to the error message until we're ready to print it.

cole-h

Mic92#1

Already pinged you over there, but just wanted to make it more obvious to any onlookers -- PR'd some Rust changes to your branch.

danieldk · 2020-06-23T18:08:17Z

nixos/modules/installer/tools/nixos-generate-config/src/lib.rs

+        show_hardware_config: false,
+        show_help: false,
+    };
+    loop {


Did you consider clap or if you are really against a lot of dependencies perhaps getopts? Nice thing about clap is that it can also generate shell completions for you, etc. Plus has nice usage information.

Yeah, I thought about that. Maybe later right now I think it quite significantly adds libraries that we would not have otherwise. And we don't need that many options right now. The current variant is more or less bug compatible with the old code.

cole-h · 2020-06-23T19:40:57Z

nixos/modules/installer/tools/nixos-generate-config/default.nix

@@ -0,0 +1,79 @@
+with import <nixpkgs> {};


Instead of relying on a channel, shouldn't this become with import ../../../.. {}; (or whatever the correct level of nesting is)?

Ideally this should be some sort of callPackage invocation rather than re-importing nixpkgs.
I might add a shell.nix convenience wrapper though for incremental compilation.

This should be a "normal" function like every other package; it shouldn't import nixpkgs at all. Also it shouldn't need a separate shell.nix, since nix-shell '<nixpkgs/nixos>' -A config.system.build.nixos-generate-config should work.

Ok. I will document the use of nix-shell than in a small hacking guide.

cole-h · 2020-06-23T19:41:03Z

nixos/modules/installer/tools/nixos-generate-config/default.nix

+  # FIXME:
+  # move nixos-generate-config manpages out of this since users can disable manpages
+  # and updating manpages generate unnecessary rebuild of this package itself.
+  nixos-manpages = (import <nixpkgs/nixos> {}).config.system.build.manual.manpages;


FRidh · 2020-06-25T07:20:18Z

Alternatives to Perl might be other languages i.e. Python/Bash has been considered but rules out

I don't think anyone ever mentioned Nim before. To me Nim seems a good alternative to Rust and NimScript (an interpretable subset of Nim) for other scripts. NimScript can also, like Python, be used on e.g. Windows, so it could be very interesting as a stdenv language.

Anyway, I'm not saying we should change. I think this here is fine.

Mic92 · 2020-06-25T08:11:21Z

Alternatives to Perl might be other languages i.e. Python/Bash has been considered but rules out

I don't think anyone ever mentioned Nim before. To me Nim seems a good alternative to Rust and NimScript (an interpretable subset of Nim) for other scripts. NimScript can also, like Python, be used on e.g. Windows, so it could be very interesting as a stdenv language.

Anyway, I'm not saying we should change. I think this here is fine.

Cannot say much about the language - I assume the binaries will be a bit larger due larger runtime support. However portability is not really a concern since nixos-generate-config will depend on Linux APIs quite heavily to detect hardware. For further language discussions please use the RFC instead.

edolstra · 2020-06-25T13:20:13Z

nixos/modules/installer/tools/nixos-generate-config/default.nix

+       $out/bin/nixos-generate-config
+  '';
+
+  NIXOS_MANPAGES = "${nixos-manpages}/share/man";


IMHO this derivation shouldn't install manpages at all, it creates a collision if both config.system.build.manual.manpages and config.system.build.nixos-generate-config install the same manpage.

It actually does not, it only capture the path to config.system.build.manual.manpages, the man page should not be propagated to the nix profile.

I will decouple this from the binary eventually and move it to an environment variable so that generating a new manpage does not trigger a rebuild of this executable.

edolstra · 2020-06-25T13:21:34Z

nixos/modules/installer/tools/nixos-generate-config/src/lib.rs

+    let status = try_with!(
+        Command::new(MAN_EXECUTABLE)
+            .arg("nixos-generate-config")
+            .env("MANPATH", NIXOS_MANPAGES)


Just call man nixos-generate-config here without MANPATH.

This would break unit tests and adds minor impurities to nixos-generate-config. Are you sure about that, given that it should generate any conflict? #91353 (comment)

edolstra · 2020-06-25T13:28:07Z

nixos/modules/installer/tools/nixos-generate-config/Cargo.toml

@@ -0,0 +1,11 @@
+[package]
+name = "nixos-generate-config"


I think we should have a single crate (and binary) for the Rust NixOS tools. That should cut down on the binary size if we rewrite nixos-rebuild, nixos-install, switch-to-configuration etc. in Rust.

I thought about the same. A multi-call binary could be added in the future to cover that.
I would leave the tools separate though and have multiple crates that may share common utility crates. But this can be also discussed once we add more Rust ports.

samueldr · 2020-06-25T20:07:37Z

This will greatly hurt the efforts of having the installation image be cross-compilable, until rust cross-compilation works as simply as pkgsCross.aarch64-multiplatform.hello does.

Being able to cross-compile the image is pretty important to get people started on some platforms. On armv7l, there is simply no cache, so it's the main option available here.

On aarch64, there is a cache, but sometimes you're stuck between a rock and a hard place. Let's take for example the Pine64 Pinebook Pro, where at first you needed a kernel with some additions (that were on the path to mainline). Without cross-compiling, it would have been extremely hard for some users to get started in a trustful manner.

To help with this, a simple enough expression builds the base image, either natively or cross. No, using the community box is not a good solution, it cannot be trusted.

This is why I believe it's important that, for the installer images, we need to be mindful to not add additional hurdles for cross-compilation.

For the record, I'm totally fine with Rust, I'd even say I prefer Rust to many other alternatives. But any solution will have to cross-compile (or be trivial to disable, but that's a loss in functionality).

(I'm assuming the language selection is an implementation detail, and not a high level design choice.)

Mic92 · 2020-06-25T20:19:27Z

This will greatly hurt the efforts of having the installation image be cross-compilable, until rust cross-compilation works as simply as pkgsCross.aarch64-multiplatform.hello does.

Being able to cross-compile the image is pretty important to get people started on some platforms. On armv7l, there is simply no cache, so it's the main option available here.

On aarch64, there is a cache, but sometimes you're stuck between a rock and a hard place. Let's take for example the Pine64 Pinebook Pro, where at first you needed a kernel with some additions (that were on the path to mainline). Without cross-compiling, it would have been extremely hard for some users to get started in a trustful manner.

To help with this, a simple enough expression builds the base image, either natively or cross. No, using the community box is not a good solution, it cannot be trusted.

This is why I believe it's important that, for the installer images, we need to be mindful to not add additional hurdles for cross-compilation.

For the record, I'm totally fine with Rust, I'd even say I prefer Rust to many other alternatives. But any solution will have to cross-compile (or be trivial to disable, but that's a loss in functionality).

(I'm assuming the language selection is an implementation detail, and not a high level design choice.)

We put some effort into cross-compiling rust in the past, did it broke again? I can have a look at this. I can assure you for sure that the cross compiling story for rust is a lot saner than for perl. It is supported by cargo and since this is the defacto standard one does not have to fix every single package. We also enabled strictDeps in buildRustPackage: #82852

samueldr · 2020-06-25T22:32:00Z

I will say that I haven't looked at it since earlier this year, and my lack of experience with Rust may have caused me to do wrong things. But when I tried things with cross-compilation and rust it didn't work as expected.

I don't know that right now it is an issue, but I wanted to make my concern known. I don't really know either how to test this correctly :).

Mic92 · 2020-06-26T11:08:35Z

Inspired by crate2nix from @kolloch, I added https://tera.netlify.app/docs as a template engine.
The reasoning behind this was to not having to manually concatenate configuration snippets and to get automatic escaping and an easy way of indenting nix expressions correctly. Its dependencies seems sane and does not go common standard ones. The release binary size is now 2.7mb with the templates compiled in. If that is too big the alternative would be to write by concatenation strings again (likely less readable). However if we plan to also rewrite other tools in nix with that and put in the same binary that might be bearable due to code sharing of crates.

Mic92 · 2020-06-26T11:12:42Z

nixos/modules/installer/tools/nixos-generate-config/templates/hardware-configuration.nix.tera

+  boot.initrd.kernelModules = {{ initrd_kernel_modules }};
+  boot.kernelModules = {{ kernel_modules }};
+  boot.extraModulePackages = {{ module_packages }};
+{%- for fs in filesystems %}


I would say this is more readable than the concatenation that was used before.

While I don't wish to oppose the use of this Tera, I wonder whether the standard fmt module would be enough. Was it already tried and determined not to suffice? If not, here's a rough but runnable sketch in the Rust Playground of how this could look.

I don't think it support loops? This is actually my only concern because we need to do string concatenation again which is not very readable. I will reconsider it later if the binary size is considered too large.

I had hoped it would be comparably readable to Tera, but it didn't turn out as well as I'd hoped. The explicit string concatenation (line 56) could be replaced with Itertools::format (adjusted Playground sketch). Using that itertools method for the loop (and also to simplify the Display implementation for NixList) increases the number of lines of assembly code to which this minimal example compiles by 11%; presumably the effect would be proportionally smaller for a real program — but then I don't wager Tera will be large enough to be a problem.

Perhaps the largest difference to me between the two approaches (to be clear, I don't mean to say either is superior for it) is that in the Tera templates one has the structure of the output file and loop logic inlined together whereas with fmt parts of the output file such as the list of filesystems necessarily get broken out, like smaller functions being factored out of a larger function.

I think the size increase of tera is also due to other crates being linked in, which might end up using later as well i.e. serde. It's good to have an alternative but first I would prioritize finishing the prototype first.

Is there a checklist tracking the progress of this prototype?

I've looked at the original post at the top of the PR but it's unclear what is and isn't complete (or at least implemented and might need future tweaks)

It's not usable yet. I will post updates as I add new features.

stale · 2021-01-24T09:52:04Z

I marked this as stale due to inactivity. → More info

Mic92 · 2021-01-24T10:35:35Z

still relevant.

Profpatsch · 2021-01-25T14:57:24Z

Relevant is #109094 which would also add rust to the nixos build closure.

I think this is a good step to take now, many people know how to write Rust and it’s a very nice language with good stdlib.

06kellyjac · 2021-07-23T08:44:50Z

@Mic92 Just wondering what changed since 24 Jan

Is this no-longer of use?

Mic92 · 2021-07-23T10:07:54Z

@06kellyjac it would be still nice to have but I don't think I will have time any time soon to actually work on this.

grahamc approved these changes Jun 23, 2020

View reviewed changes

Mic92 mentioned this pull request Jun 23, 2020

[RFC 0070] Merge nixos-hardware in to nixpkgs (#70) NixOS/rfcs#70

Closed

ofborg bot added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 23, 2020

Mic92 commented Jun 23, 2020

View reviewed changes

nixos/modules/installer/tools/nixos-generate-config/Cargo.toml Outdated Show resolved Hide resolved

Mic92 commented Jun 23, 2020

View reviewed changes

nixos/modules/installer/tools/nixos-generate-config/.envrc Outdated

@@ -0,0 +1 @@

use nix

Copy link

Member Author

Mic92 Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should also go away.

Mic92 commented Jun 23, 2020

View reviewed changes

Mic92 force-pushed the nixos-generate-config branch from f3cd2d2 to 9fcf18f Compare June 23, 2020 14:04

nixos-generate-config: add Rust stub with tests

1b7dc49

Mic92 force-pushed the nixos-generate-config branch from 9fcf18f to 1b7dc49 Compare June 23, 2020 14:44

cole-h added 3 commits June 23, 2020 10:18

nixos-generate-config: move binary to main.rs

c53b19a

I don't see us needing any additional binaries for this, so a folder dedicated to a single binary seems weird.

nixos-generate-config: don't show binary name multiple times

5de9191

Because we print the binary name and the relevant error when e.g. `show_help()` or `parse_options()` errors out, we don't need to add the binary name to the error message until we're ready to print it.

cole-h reviewed Jun 23, 2020

View reviewed changes

danieldk reviewed Jun 23, 2020

View reviewed changes

Mic92 and others added 2 commits June 23, 2020 19:57

Merge pull request #1 from cole-h/minor-nits

4b2a914

nixos-generate-config: cleanups

4dd5b06

Mic92 requested a review from edolstra June 23, 2020 19:03

cole-h requested changes Jun 23, 2020

View reviewed changes

davidak mentioned this pull request Jun 23, 2020

nixos/systemPackages: clean up #91213

Merged

10 tasks

edolstra reviewed Jun 25, 2020

View reviewed changes

nixos-generate-config: use tera templates

27ac73d

Mic92 commented Jun 26, 2020

View reviewed changes

stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 24, 2021

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 25, 2021

Mic92 closed this Jul 23, 2021

Mic92 deleted the nixos-generate-config branch July 23, 2021 08:29

Mic92 restored the nixos-generate-config branch July 23, 2021 10:07

Mic92 deleted the nixos-generate-config branch November 20, 2022 19:14

nixos-generate-config: add Rust stub with tests #91353

nixos-generate-config: add Rust stub with tests #91353

Conversation

Mic92 commented Jun 23, 2020 • edited

Motivation for this change

Choose a reason for hiding this comment

Mic92 Jun 23, 2020 • edited

Choose a reason for hiding this comment

cole-h left a comment

Choose a reason for hiding this comment

danieldk Jun 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cole-h Jun 23, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jun 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh commented Jun 25, 2020

Mic92 commented Jun 25, 2020

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

Mic92 Jun 25, 2020 • edited

Choose a reason for hiding this comment

samueldr commented Jun 25, 2020 • edited

Mic92 commented Jun 25, 2020 • edited

samueldr commented Jun 25, 2020 • edited

Mic92 commented Jun 26, 2020 • edited

Mic92 Jun 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mic92 Jul 25, 2020 • edited

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

stale bot commented Jan 24, 2021

Mic92 commented Jan 24, 2021

Profpatsch commented Jan 25, 2021 • edited

06kellyjac commented Jul 23, 2021

Mic92 commented Jul 23, 2021

Mic92 commented Jun 23, 2020 •

edited

Mic92 Jun 23, 2020 •

edited

danieldk Jun 23, 2020 •

edited

cole-h Jun 23, 2020 •

edited

Mic92 Jun 25, 2020 •

edited

Mic92 Jun 25, 2020 •

edited

samueldr commented Jun 25, 2020 •

edited

Mic92 commented Jun 25, 2020 •

edited

samueldr commented Jun 25, 2020 •

edited

Mic92 commented Jun 26, 2020 •

edited

Mic92 Jun 26, 2020 •

edited

Mic92 Jul 25, 2020 •

edited

Profpatsch commented Jan 25, 2021 •

edited