projg2 / eclean-kernel

Installed kernel cleanup tool
GNU General Public License v2.0
31 stars 10 forks source link

[WIP] Add unified efistub handling, add actions CI #33

Closed ajakk closed 1 year ago

ajakk commented 1 year ago

This is derived from Jannik's work last year: https://github.com/Jannik2099/eclean-kernel/commit/32b1a2d48813fec60826fa5f23a54c9ff4774588

There's one remaining test failure, and I'm opening this now because I'm not really sure how to handle it yet:

FAILED test/test_file.py::KernelImageTests::test_overflow_ver_string_bz - AssertionError: UnrecognizedKernelError not raised

The semantics of how the file is read have changed, and I'm not sure how best to make it error in this way.

ajakk commented 1 year ago

Hm, I've just found that eclean-kernel doesn't seem to handle dropping the configuration file at loader/entries/${machineid}-${version}.conf. How should it be taught that this file should be dropped too?

ajakk commented 1 year ago

Most recent commit adds support to prune these config entries as mentioned in my previous comment.

mgorny commented 1 year ago

Could you rebase now, please?

mgorny commented 1 year ago

Are you asking me about something specific? I see test failures.

ajakk commented 1 year ago

Yeah, from my first comment:

There's one remaining test failure, and I'm opening this now because I'm not really sure how to handle it yet:

FAILED test/test_file.py::KernelImageTests::test_overflow_ver_string_bz - AssertionError: UnrecognizedKernelError not raised

The semantics of how the file is read have changed, and I'm not sure how best to make it error in this way.

mgorny commented 1 year ago

I'm afraid this "complete rewrite of everything in one commit" approach is not going to work for me. Please split it into smaller changes, adding tests as you add support for new file variations.

mgorny commented 1 year ago

Ping.

ajakk commented 1 year ago
2023-02-28 16:23:34     @ajak   i'm not really sure how to split it up further, it's really one thing being added, but it comes with a bit of code moving around (and much of the additions are comments)
2023-02-28 16:24:09     @ajak   there's not really atomic changes beyond what's there i don't think
2023-02-28 16:24:29     @mgorny "things being moved around" also justifies a separate commit
2023-02-28 16:24:39     @mgorny if it doesn't change the behavior, it should work atomically
2023-02-28 16:28:19     @ajak   oh, there's a bunch more test failures than i remember there being, wtf
2023-02-28 16:29:13     @mgorny yes, pretty much everything fails for me
2023-02-28 16:29:59     @ajak   well, if you come up with a better way to handle it, please do