Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 7 commits into
base: master
from

Conversation

@Mic92
Copy link
Contributor

Mic92 commented Jun 23, 2020

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

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Author Contributor

This one should also go away.

];

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Author Contributor

See Ma, no cursed fixed-input derivation!

@Mic92 Mic92 force-pushed the Mic92:nixos-generate-config branch from f3cd2d2 to 9fcf18f Jun 23, 2020
@Mic92 Mic92 force-pushed the Mic92:nixos-generate-config branch from 9fcf18f to 1b7dc49 Jun 23, 2020
cole-h added 3 commits Jun 23, 2020
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.
Copy link
Member

cole-h left a comment

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.

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

This comment has been minimized.

Copy link
@danieldk

danieldk Jun 23, 2020

Member

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Author Contributor

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.

Mic92 added 2 commits Jun 23, 2020
@Mic92 Mic92 requested a review from edolstra Jun 23, 2020
@@ -0,0 +1,79 @@
with import <nixpkgs> {};

This comment has been minimized.

Copy link
@cole-h

cole-h Jun 23, 2020

Member

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 23, 2020

Author Contributor

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 comment has been minimized.

Copy link
@edolstra

edolstra Jun 25, 2020

Member

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 25, 2020

Author Contributor

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

# 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;

This comment has been minimized.

Copy link
@cole-h

cole-h Jun 23, 2020

Member

Same here.

@davidak davidak mentioned this pull request Jun 23, 2020
6 of 10 tasks complete
@FRidh
Copy link
Member

FRidh commented Jun 25, 2020

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
Copy link
Contributor Author

Mic92 commented Jun 25, 2020

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.

$out/bin/nixos-generate-config
'';

NIXOS_MANPAGES = "${nixos-manpages}/share/man";

This comment has been minimized.

Copy link
@edolstra

edolstra Jun 25, 2020

Member

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 25, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 26, 2020

Author Contributor

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.

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

This comment has been minimized.

Copy link
@edolstra

edolstra Jun 25, 2020

Member

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 25, 2020

Author Contributor

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)

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

This comment has been minimized.

Copy link
@edolstra

edolstra Jun 25, 2020

Member

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 25, 2020

Author Contributor

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
Copy link
Member

samueldr commented Jun 25, 2020

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
Copy link
Contributor Author

Mic92 commented Jun 25, 2020

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
Copy link
Member

samueldr commented Jun 25, 2020

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
Copy link
Contributor Author

Mic92 commented Jun 26, 2020

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.

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

This comment has been minimized.

Copy link
@Mic92

Mic92 Jun 26, 2020

Author Contributor

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

This comment has been minimized.

Copy link
@8573

8573 Jul 25, 2020

Contributor

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jul 25, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@8573

8573 Jul 25, 2020

Contributor

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.

This comment has been minimized.

Copy link
@Mic92

Mic92 Jul 25, 2020

Author Contributor

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.

This comment has been minimized.

Copy link
@06kellyjac

06kellyjac Jul 25, 2020

Contributor

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)

This comment has been minimized.

Copy link
@Mic92

Mic92 Jul 28, 2020

Author Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.