modularize handle_requirement
#453
Conversation
shubhbapna
force-pushed
the
modularize-handle-requirement
branch
from
4029933 to
fbfc377
Compare
src/fromager/bootstrapper.py
Outdated
| self.ctx.add_to_build_order( | ||
| req=req, | ||
| version=resolved_version, | ||
| source_url=source_url, | ||
| source_url_type=source_url_type, | ||
| prebuilt=pbi.pre_built, | ||
| constraint=constraint, | ||
| ) |
There was a problem hiding this comment.
I moved this to after building of wheels because:
- We have graph.json that as already recorded this dependency
- if the build fails then anyways bootstrapping stops and we don't end up using that build order file
shubhbapna
force-pushed
the
modularize-handle-requirement
branch
from
fbfc377 to
f13515e
Compare
shubhbapna
force-pushed
the
modularize-handle-requirement
branch
from
f13515e to
7d0dab3
Compare
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class Bootstrapper: |
There was a problem hiding this comment.
With this new class, maybe some of the things we have in the context, like managing the build order file and graph, should move here? Or their own classes used by the bootstrapper? Those would definitely be changes for other PRs.
There was a problem hiding this comment.
I think the graph is accessed in other commands as well. Maybe I can try to move build order stuff, mark_as_seen and has_been_seen
There was a problem hiding this comment.
okay moved these to the bootstrapper class
| build_system_dependencies = dependencies.get_build_system_dependencies( | ||
| ctx=self.ctx, req=req, sdist_root_dir=sdist_root_dir | ||
| ) | ||
| self._handle_build_requirements( | ||
| RequirementType.BUILD_SYSTEM, | ||
| build_system_dependencies, | ||
| ) | ||
|
|
||
| build_backend_dependencies = dependencies.get_build_backend_dependencies( | ||
| ctx=self.ctx, req=req, sdist_root_dir=sdist_root_dir | ||
| ) | ||
| self._handle_build_requirements( | ||
| RequirementType.BUILD_BACKEND, | ||
| build_backend_dependencies, | ||
| ) | ||
|
|
||
| build_sdist_dependencies = dependencies.get_build_sdist_dependencies( | ||
| ctx=self.ctx, req=req, sdist_root_dir=sdist_root_dir | ||
| ) | ||
| self._handle_build_requirements( | ||
| RequirementType.BUILD_SDIST, | ||
| build_sdist_dependencies, | ||
| ) |
There was a problem hiding this comment.
This repetition has always bothered me, but it was never a big enough of an issue to clean up. Maybe that's another MR, to use a loop here.
There was a problem hiding this comment.
Refactored it let me know if the current version is better
There was a problem hiding this comment.
nevermind the refactor caused some e2e tests to fail so maybe it is better suited for a seperate PR
| ) -> None: | ||
| self.progressbar.update_total(len(build_dependencies)) | ||
|
|
||
| for dep in self._sort_requirements(build_dependencies): |
There was a problem hiding this comment.
Why sort here? Were we doing that before?
src/fromager/sdist.py
Outdated
There was a problem hiding this comment.
It might be unfortunate to lose the git history of this file. I don't know if git is smart enough to recognize that the new bootstrapper.py is mostly the same content, in a different format. One way to deal with that would be to include a commit in this PR that renames sdist.py to bootstrapper.py, and then make all of the edits in a second commit. I wouldn't expect the tests to pass on that rename commit.
I could also be overthinking it, so let's see what other reviewers think.
There was a problem hiding this comment.
Why would we like to preserve the history of this file?
There was a problem hiding this comment.
Maybe I'm a packrat.
I like to be able to see the evolution of code, even if that means spanning files sometimes. It's not the end of the world to not have it.
shubhbapna
force-pushed
the
modularize-handle-requirement
branch
from
7d0dab3 to
f6ae08a
Compare
Signed-off-by: Shubh Bapna <[email protected]>
shubhbapna
force-pushed
the
modularize-handle-requirement
branch
from
f6ae08a to
f9cfba2
Compare
fixes #452
This is essentially a one-to-one refactor of sdist module and breaks down
handle_requirementto multiple functions (no logic change). This is to help with #343