[Refactor]: Decompose Bootstrapper into separate classes #914
Conversation
This commit attempts to refactor the bootstrapper class by breaking it into separate classes. We create a ResolverManager class which handles certain functionality needed by bootstrap. The new class: Uses resolver.py as a building block (calls resolver.resolve(), resolver.GenericProvider, etc.) - Adds caching - avoids redundant resolution via _resolved_requirements dict - Adds history-based resolution - reuses versions from previous bootstrap runs via prev_graph - Handles git URL cloning - clones repos and extracts versions from package metadata - Handles cached wheel lookups - checks local filesystem + remote cache server This refactoring was suggested as a good first step for multiple version bootstrap and also makes the code simpler. Fixes: python-wheel-build#911 Signed-off-by: Rohan Devasthale <[email protected]>
dhellmann
left a comment
dhellmann
left a comment
There was a problem hiding this comment.
I think the split in responsibilities isn't quite right in this draft. I've tried to point out a few examples, but basically I think the new class knows too much about bootstrapping and the fact that that is where it's used. Let me know if you'd like to chat about it before picking this back up.
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| # Type alias for the dependency chain stack used to track why we're processing |
There was a problem hiding this comment.
I would really only expect the bootstrapper to need to know this.
|
|
||
| # Callback type for preparing build dependencies. This is used when resolving | ||
| # versions from git URLs where we need to prepare build deps to get metadata. | ||
| # The callback takes (req, source_dir, build_env) and returns set[Requirement]. |
There was a problem hiding this comment.
Resolving from a git URL may be a little bit of a grey area, but I would normally expect the resolver to not need to know about build environments. Is that something the callback could look up using the work context, for example?
| # Cached wheel lookup methods | ||
| # ------------------------------------------------------------------------- | ||
|
|
||
| def find_cached_wheel( |
There was a problem hiding this comment.
I think I would have left this in the bootstrapper. It knows a lot about the internals of bootstrapping or building, like the working directories.
|
|
||
| return None, None | ||
|
|
||
| def _look_for_existing_wheel( |
There was a problem hiding this comment.
Same with this, it feels like a detail of bootstrapping not resolving.
| Delegation method for backward compatibility. | ||
| The actual implementation is in ResolutionManager. | ||
| """ | ||
| return self.resolver._resolve_source_with_history(req, req_type, why=self.why) |
There was a problem hiding this comment.
By convention, prefixing a symbol with _ means it is private. So for methods of Bootstrapper that we move to the new class and then call from here, we need to rename them to remove the _ and make them public.
There was a problem hiding this comment.
Alternatively, perhaps instead of having a bunch of different methods on this class call different methods on the other class, we can have a single resolve() method on the other class? I would have to look at how the invocations locally are interleaved with other logic, that may not work.
2 participants
This commit attempts to refactor the bootstrapper
class by breaking it into separate classes.
We create a ResolverManager class which handles
certain functionality needed by bootstrap.
The new class:
This refactoring was suggested as a good first step for multiple version bootstrap and also makes the code simpler.
Fixes: #911