-
Couldn't load subscription status.
- Fork 31
Improve DependencyNode, get recursive build/install dependencies. #763
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
tiran
wants to merge
1
commit into
python-wheel-build:main
Choose a base branch
from
tiran:depnode
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+167
−0
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 would expect to see D in this list because D is a build requirement of C. We can't build A until we can install C, and we can't install C until we can build it.
Like I think I said on another review, we have the same issue here that we had with determining whether something was a build dependency during bootstrap. There, the package "inherits" the "build dependency" type from the
whychain. If anything earlier in thewhychain is a build dependency, then so is the current package.In the bootstrap process we build packages using in-order traversal. We build the build dependencies, then the package, then its install dependencies. That separates the dependency types and allows us to cope with potential circular dependencies in installation dependencies, since those are not forbidden.
I think we need to apply the same logic when building in parallel. That is going to potentially limit the amount we do in parallel, but I think that's required to achieve a correct build, without deadlocking on cyclic dependencies or trying to use something that isn't available.
I think that means keeping track of 2 things in build-parallel for each package: Is the package ready to be built, and is the package ready to be used to build other packages. That will complicate the logic for deciding what work to do, and while a literal topological traversal will give us the right order for sequential build I don't think it gives us parallel builds on its own.
The current implementation in the main branch doesn't attempt to traverse the graph explicitly. During each pass it iterates over the set of all unbuilt nodes and asks if the build dependencies for the node have been built. If so, it adds the current node to the set to be built. I think we need to extend that logic so that for each build dependency we check not only that it has been built, but that its installation dependencies are built. Only at that point can we say that the build dependencies are ready to be used to build the current node.
We could build that using the topological sorter by having the sorter tell us when a package is ready to be built, but then keeping those built nodes in a separate list and waiting to mark them as done in the graph when the installation dependencies are also built.
Alternatively, we could fix the loop we have already to keep track of the same information.
I'm not sure which will be easier to follow.
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've done some refactoring of the existing logic and added install dependency checking in #794
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.
No, A does not need D to build. A only needs C+E+F to build. The indirect dependencies A -> C -> D is handled by the topology graph. C does not become available until D is built. Because C has a dependency on D and A has a dependency on C, the topology converges on the solution D, then C, then A.