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

Move BootLoaderConfig to context manager #2438

Merged
merged 2 commits into from
Jan 26, 2024

Conversation

schaefi
Copy link
Collaborator

@schaefi schaefi commented Jan 19, 2024

Change the BootLoaderConfig class to be a context manager. All code using BootLoaderConfig was updated to the following with statement:

with BootLoaderConfig.new(...) as bootloader_config:
    bootloader_config.some_member()

This is related to Issue #2412

@schaefi schaefi requested a review from dcermak January 19, 2024 15:57
@schaefi schaefi self-assigned this Jan 19, 2024
Change the BootLoaderConfig class to be a context manager.
All code using BootLoaderConfig was updated to the following
with statement:

    with BootLoaderConfig.new(...) as bootloader_config:
        bootloader_config.some_member()

This is related to Issue #2412
@schaefi schaefi force-pushed the move_bootloader_to_context_manager branch from c830c19 to d209a57 Compare January 23, 2024 14:11
@schaefi
Copy link
Collaborator Author

schaefi commented Jan 23, 2024

I created a Staging release with this patch... waiting for the integration tests to complete

@schaefi
Copy link
Collaborator Author

schaefi commented Jan 23, 2024

All tests passing, open for review :)

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

I think it would be cleaner if the BootLoaderConfigBase would use a contextlib.ExitStack, as the volume_mounts are already context managers

@schaefi
Copy link
Collaborator Author

schaefi commented Jan 25, 2024

I think it would be cleaner if the BootLoaderConfigBase would use a contextlib.ExitStack, as the volume_mounts are already context managers

The reason why I did not push it into an ExitStack was because I think the elements of the ExitStack will disappear one after the other when we go forward moving the other classes into context managers. With each follup pull request my plan is to come more and more into that nested structure of context managers which will reduce the existing ExitStack until we reach the point where it is empty.

The current implementation of some builders keeps resources active for way too long. BootLoaderConfig is one example. It was created super early and kept until the end of the complete build process. But within that process there is only one time where it actually operates and depending on the bootloader mounts stuff, does its bootloader config job and should now free the resources but it holds it too long. One of the nice effects of moving into the context managers is to find out how to release as early as possible.

That's why I did not use the ExitStack because for the BootLoaderConfig I knew when I need it and when I can release.

Hope this makes sense to you too ?

@dcermak
Copy link
Collaborator

dcermak commented Jan 26, 2024

With each follup pull request my plan is to come more and more into that nested structure of context managers which will reduce the existing ExitStack until we reach the point where it is empty.

Well, the idea of the ExitStack is that you do not have to nest contextmanagers and just have one exitstack that cleans them all up. But I think we can add these later on if they turn out to be useful.

Copy link
Collaborator

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

LGTM, I've added two small nit-picky comments

kiwi/builder/live.py Outdated Show resolved Hide resolved
kiwi/builder/live.py Outdated Show resolved Hide resolved
@dcermak dcermak merged commit b29e491 into main Jan 26, 2024
13 checks passed
@dcermak dcermak deleted the move_bootloader_to_context_manager branch January 26, 2024 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants