Skip to content

fix use of log context in bootstrapper #595

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dhellmann
Copy link
Member

The original implementation managed the context only for some types of dependencies. This change moves the handling into the bootstrap() method where it can be applied to all dependencies.

@dhellmann dhellmann requested a review from tiran May 3, 2025 14:16
@dhellmann dhellmann force-pushed the fix-log-record-context branch from 66dfcad to 30e7c7a Compare May 3, 2025 14:18
@@ -360,11 +367,9 @@ def _handle_build_requirements(
self.progressbar.update_total(len(build_dependencies))

for dep in self._sort_requirements(build_dependencies):
token = requirement_ctxvar.set(dep)
Copy link
Collaborator

Choose a reason for hiding this comment

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

With this change build_environment.maybe_install() has no longer the correct context var. #598 should fix the bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have to have a look at that. The bootstrap() function should be the top level function, so handling the context there should cover everything.

The original implementation managed the context only for some types of
dependencies. This change moves the handling into the bootstrap() method
where it can be applied to all dependencies.

Signed-off-by: Doug Hellmann <[email protected]>
@dhellmann dhellmann force-pushed the fix-log-record-context branch from 30e7c7a to ceb66dc Compare May 5, 2025 19:59
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