-
Notifications
You must be signed in to change notification settings - Fork 39
feature: add wheels to download-sequence #162
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
Conversation
Add an option to download wheels to download-sequence. This is useful for pre-populating a development environment with built wheels to avoid building expensive dependencies when debugging the bootstrap logic.
29a4efe to
6c3139b
Compare
|
|
||
| def evaluate_marker(req: Requirement, extras: dict | None = None) -> bool: | ||
| def evaluate_marker( | ||
| parent_req: Requirement, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I understood, this parent_req was added mainly for the purposes of logging? If that's the case then I feel like we shouldn't change the function since this parameter is not really required for the correct implementation of evaluate_marker. For example here: https://github.com/python-wheel-build/fromager/pull/162/files#diff-3f261cb4d56f2826fc58f44967dd7d1b2a92b5e4e9da0133b832d0e9027ff011 since there is no parent_req we just end up using the original_req.
If we want to add a log statement to keep track of where the requirement being evaluated is coming from, then I think we should leave that to the caller of the function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally I would agree. Fromager produces a lot of log output, though, and given the way it switches context as it builds different packages it can be challenging to understand which package any one message relates to. To address that, I've been trying to include the "current" requirement as a prefix to most of the log output. That allows someone to grep the logs for a package name, for example, and then see all of the activity related to resolving, downloading, and building it even though those lines might be separated by hundreds of other lines as other packages were processed.
In this case, understanding how the markers are evaluated is important for debugging why a dependency is included (or not), but the caller doesn't have all of the information (the extras, environment, etc.) and so I wanted to add the context requirement (maybe that's a better name than "parent"?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, is there a way to set a prefix for the logger? This way whenever we start working with a requirement we can set a prefix that includes that requirements name (and other relevant information for the current context) instead of passing them around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there is a way to do this:
https://docs.python.org/3/howto/logging-cookbook.html#adding-contextual-information-to-your-logging-output
We can probably do this in a separate patch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bootstrapping relies heavily on recursion. During the processing of a top-level requirement, we can go up and down many levels of requirements. A LoggerAdapter would need be told how to manage that stack of requirements are they are being processed so that the log messages include the right name at the right time. We could do that manipulation in sdist.handle_requirement(), but it seemed simpler to just pass the value we need into the function.
I'll think about ways to apply a LoggerAdapter separately from this PR. Can we merge it in the mean time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see make sense
Add an option to download wheels to
download-sequence. This is useful for
pre-populating a development environment with
built wheels to avoid building expensive
dependencies when debugging the bootstrap logic.
Fixes #153