Lanzatool: respect configuration limit #34

2022-12-24T15:25:16Z

nikstur commented

2022-12-24 15:25:16 +00:00

(Migrated from github.com)

This PR enables setting a configuration limit for lanzatool via the --configuration-limit parameter.

It includes

Unit tests for the garbage collector.
Some fixes that I made while debugging (incl. a unit test in esp.rs)
Infrastructure for integration testing of the lanzatool binary via assert_cmd and an initial test for the GC. Some minor changes
needed to be made to the flake to support the new lanzatool integration tests.

It is missing a NixOS integration test because it turns out these are really hard to implement with an out-of-tree module (and even nixpkgs does not contain any tests for the config limit).

I recommend review commit-by-commit.

This PR enables setting a configuration limit for lanzatool via the `--configuration-limit` parameter. It includes - Unit tests for the garbage collector. - Some fixes that I made while debugging (incl. a unit test in esp.rs) - Infrastructure for integration testing of the lanzatool binary via assert_cmd and an initial test for the GC. Some minor changes needed to be made to the flake to support the new lanzatool integration tests. It is missing a NixOS integration test because it turns out these are really hard to implement with an out-of-tree module (and even nixpkgs does not contain any tests for the config limit). I recommend review commit-by-commit.

👍 2

nikstur commented

2022-12-30 20:37:51 +00:00

(Migrated from github.com)

Depends on #42

blitz (Migrated from github.com) requested changes

2022-12-31 00:20:30 +00:00

blitz (Migrated from github.com) left a comment

Nice! I especially like the tests. ❤️

Nice! I especially like the tests. :heart:

rust/lanzatool/src/cli.rs

You may want to keep this as usize to avoid the awkward conversion between u64 and usize.

You may want to keep this as `usize` to avoid the awkward conversion between `u64` and `usize`.

rust/lanzatool/src/install.rs

					
				@ -8,37 +8,71 @@ use nix::unistd::sync;

				use tempfile::tempdir;

It would be helpful to add a comment why RefCell is necessary here. Did you lose against the borrow checker? :)

It would be helpful to add a comment why `RefCell` is necessary here. Did you lose against the borrow checker? :)

nit: Technically this is a lossy conversion and that Rust allows this is sad. :) I tend to write .try_into().unwrap() when I don't explicitly intend a lossy conversion. But configuration_limit could just be usize to begin with.

nit: Technically this is a lossy conversion and that Rust allows this is sad. :) I tend to write `.try_into().unwrap()` when I don't explicitly intend a lossy conversion. But `configuration_limit` could just be `usize` to begin with.

You want .sort_by_key(|l| l.version) instead of implementing PartialEq, Eq and Ord for GenerationLink.

The problem with your trait implementations is that they do not actually implement the traits as they are intended. Two GenerationLink instances will compare as equal even though they have different paths. This may be surprising in other circumstances.

You want `.sort_by_key(|l| l.version)` instead of implementing `PartialEq`, `Eq` and `Ord` for `GenerationLink`. The problem with your trait implementations is that they do not actually implement the traits as they are intended. Two `GenerationLink` instances will compare as equal even though they have different paths. This may be surprising in other circumstances.

rust/lanzatool/src/pe.rs

This function could use a comment that says that it outputs a UEFI filename, which is a bit non-obvious from the name alone.

rust/lanzatool/src/pe.rs

There is no need to open the file. You can just do fs:metadata(path).

There is no need to open the file. You can just do `fs:metadata(path)`.

rust/lanzatool/src/signature.rs

Why not propagate the error (with some context)?

rust/lanzatool/tests/common/mod.rs

Why not propagate the error?

nikstur (Migrated from github.com) reviewed

2022-12-31 01:03:41 +00:00

rust/lanzatool/src/install.rs

					
				@ -8,37 +8,71 @@ use nix::unistd::sync;

				use tempfile::tempdir;

This is what I was looking for! I guess I was just overcome with small brain syndrome and didn't bother to look into a more elegant solution. Thanks for the pointer!

nikstur (Migrated from github.com) reviewed

2022-12-31 01:04:27 +00:00

rust/lanzatool/src/cli.rs

I guess my intuition was that I'd rather have an explicit type than usize. Thinking about it more, I don't really see a good reason for why u64 would be better.

nikstur (Migrated from github.com) reviewed

2022-12-31 01:09:27 +00:00

rust/lanzatool/tests/common/mod.rs

There is no good reason why I didn't, just a leftover.

But is there a good reason to propagate the error in a test? My belief was that unwrap or expect is fine there.

There is no good reason why I didn't, just a leftover. But is there a good reason to propagate the error in a test? My belief was that unwrap or expect is fine there.

nikstur (Migrated from github.com) reviewed

2022-12-31 02:00:50 +00:00

rust/lanzatool/src/pe.rs

This is a good suggestion. Since these changes were pulled out into a (now already merged) separate PR (#42), I have opened another PR which implements your requested changes: #43

nikstur (Migrated from github.com) reviewed

2022-12-31 02:00:54 +00:00

rust/lanzatool/src/pe.rs

See #43

nikstur (Migrated from github.com) reviewed

2022-12-31 02:00:59 +00:00

rust/lanzatool/src/signature.rs

See #43

blitz (Migrated from github.com) reviewed

2022-12-31 09:58:40 +00:00

rust/lanzatool/tests/common/mod.rs

Ah, yes. It's fine in the test. If you can, it's nice to propagate it anyway, because code tends to multiply and people take anything as template.

blitz (Migrated from github.com) reviewed

2022-12-31 10:08:56 +00:00

rust/lanzatool/tests/common/mod.rs

Can you add a developer-actionable error message somehow? Right now a developer not using our shell environment will see:

running 1 test
test keep_only_configured_number_of_generations ... FAILED

failures:

---- keep_only_configured_number_of_generations stdout ----
thread 'keep_only_configured_number_of_generations' panicked at 'Failed to setup generation link: Failed to setup toplevel

Caused by:
    environment variable not found', tests/gc.rs:18:18

Alternatively a well placed comment in the code would also be ok.

Can you add a developer-actionable error message somehow? Right now a developer not using our shell environment will see: ``` running 1 test test keep_only_configured_number_of_generations ... FAILED failures: ---- keep_only_configured_number_of_generations stdout ---- thread 'keep_only_configured_number_of_generations' panicked at 'Failed to setup generation link: Failed to setup toplevel Caused by: environment variable not found', tests/gc.rs:18:18 ``` Alternatively a well placed comment in the code would also be ok.

blitz (Migrated from github.com) approved these changes

2022-12-31 10:10:15 +00:00

RaitoBezarius (Migrated from github.com) reviewed

2022-12-31 21:03:23 +00:00

rust/lanzatool/tests/common/mod.rs

					
				@ -0,0 +100,4 @@

				        ID=lanzaos

				        NAME=LanzaOS

				        PRETTY_NAME="LanzaOS 23.05 (Goat)"

				        VERSION="23.05 (Goat)"

I really like the subtle change

RaitoBezarius (Migrated from github.com) approved these changes

2022-12-31 21:04:07 +00:00

RaitoBezarius (Migrated from github.com) left a comment

Amazing <3.

nikstur (Migrated from github.com) reviewed

2023-01-01 23:15:19 +00:00

rust/lanzatool/src/install.rs

					
				@ -8,37 +8,71 @@ use nix::unistd::sync;

				use tempfile::tempdir;

I thought interior mutability would be the right pattern here, so I don't have to pass the entire struct around as mutable. But, since RefCell also incurs a runtime penality making the entire struct mutable is probably better.