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.

Sign up for GitHub

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

Jump to bottom

Lanzatool: respect configuration limit #34

Merged
merged 6 commits into from
Jan 1, 2023
Merged

Lanzatool: respect configuration limit #34

merged 6 commits into from
Jan 1, 2023

Conversation

nikstur
Copy link
Member

@nikstur nikstur commented Dec 24, 2022 •
edited
Loading

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.

@nikstur nikstur mentioned this pull request Dec 24, 2022
Open
@nikstur nikstur force-pushed the configuration-limit branch 12 times, most recently from 17e88a5 to 5eaf7d1 Compare December 30, 2022 15:22
@nikstur nikstur changed the title Draft: Respect configuration limit Respect configuration limit Dec 30, 2022
@nikstur nikstur marked this pull request as ready for review December 30, 2022 15:53
@nikstur nikstur changed the title Respect configuration limit Lanzatool: respect configuration limit Dec 30, 2022
@nikstur nikstur force-pushed the configuration-limit branch from 5eaf7d1 to e1e61ec Compare December 30, 2022 20:09
@nikstur nikstur mentioned this pull request Dec 30, 2022
Merged
@nikstur nikstur force-pushed the configuration-limit branch from e1e61ec to f832324 Compare December 30, 2022 20:37
@nikstur
Copy link
Member Author

nikstur commented Dec 30, 2022

Depends on #42

@nikstur nikstur force-pushed the configuration-limit branch 3 times, most recently from 18e8e8f to 4b1a525 Compare December 31, 2022 00:05
blitz
blitz requested changes Dec 31, 2022
View reviewed changes
Copy link
Member

@blitz blitz left a comment

Choose a reason for hiding this comment

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

Nice! I especially like the tests. ❤️

rust/lanzatool/src/pe.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/pe.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/signature.rs Outdated Show resolved Hide resolved
rust/lanzatool/tests/common/mod.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/cli.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/install.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/install.rs Outdated Show resolved Hide resolved
rust/lanzatool/src/install.rs Outdated Show resolved Hide resolved
@nikstur nikstur force-pushed the configuration-limit branch 2 times, most recently from 9ea6fcb to 5d3f9c9 Compare December 31, 2022 01:02
@nikstur nikstur force-pushed the configuration-limit branch from 5d3f9c9 to f86c04b Compare December 31, 2022 01:10
@nikstur nikstur mentioned this pull request Dec 31, 2022
Merged
RaitoBezarius
RaitoBezarius approved these changes Dec 31, 2022
View reviewed changes
Copy link
Member

@RaitoBezarius RaitoBezarius left a comment

Choose a reason for hiding this comment

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

Amazing <3.

@nikstur nikstur force-pushed the configuration-limit branch from f86c04b to d3a96b1 Compare January 1, 2023 23:12
@nikstur
Copy link
Member Author

nikstur commented Jan 1, 2023

@blitz @RaitoBezarius thank you for the feedback!

@nikstur nikstur merged commit f431622 into master Jan 1, 2023
@nikstur nikstur deleted the configuration-limit branch January 2, 2023 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@blitz blitz blitz approved these changes

@RaitoBezarius RaitoBezarius RaitoBezarius approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nikstur @blitz @RaitoBezarius