forc-pkg: Refactor BuildPlan construction and validation to minimize fetching, improve offline support, reduce I/O, fix bugs.
#2234
Conversation
This file contains 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
…pendency removal from manifest file.
Co-authored-by: mitchmindtree <mitchell.nordine@fuel.sh>
This commit reduces the number of times that we need to read each dependency's manifest from file in order to reduce the amount of I/O performed. Each dependency's manifest should now only be loaded once during either `graph_to_manifest_map` or `fetch_graph`. This required implementing `Clone` for `ManifestFile` to allow for constructing the build plan in a manner that does not require consuming the manifest file. `ManifestFile::clone` should only ever be called for the root node. As a side-effect, the `build` and `check` commands no longer require loading each package's manifest as they are already provided by the build plan. As a result both commands no longer require the sway git tag to be passed through.
mitchmindtree
changed the title
forc-pkg: Refactor BuildPlan construction and validation to minimize fetching, improve offline support, reduce I/O.forc-pkg: Refactor BuildPlan construction and validation to minimize fetching, improve offline support, reduce I/O, fix bugs.
Previously, all dependency nodes were removed in the case that they were invalid for any one of their parents. This commit changes the behaviour to not remove the node, but rather each invalid dependency *edge* instead. This means that if a node is valid for one parent but not another, only the edge to the other parent is removed and not the whole node itself. The node is only removed in the case that it is invalid for all of its parents. This is necessary for handling the case where a dependency no longer exists in one parent's `[dependencies]` but still exists in another. The most common case for this is `std`, as many nodes may depend on `std` however we don't want to remove `std` and have to re-fetch it just because one of the nodes removed their depenency on it.
mitchmindtree
force-pushed
the
mitchmindtree/build-plan-refactor
branch
from
6daaea1 to
02ceb92
Compare
This is checked during the `validate_graph` phase. Also improves a bunch of comments.
kayagokalp
previously approved these changes
Jul 6, 2022
There was a problem hiding this comment.
Looks good to me 🙌 , just left a small nit (which is not a blocker, just a simple typo).
Also a side note I realized the build process is considerably faster (while adding a new dep) with this PR. A small benchmark on my m1 MBA:
Adding dep (existing Forc.lock) before this PR: 3.18 seconds
Adding dep (existing Forc.lock) with this PR: 0.79 seconds
Co-authored-by: Kaya Gökalp <kayagokalp@sabanciuniv.edu>
kayagokalp
approved these changes
Jul 6, 2022
|
Should this be merged into #1974 instead of |
|
I think it's fine to merge this into master and close #1974. |
Ah I should have clarified this in my OP a bit further! I decided to base directly against |
Closed
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This continues @kayagokalp's great work on #1974 with a wider overhaul of the package graph
BuildPlanvalidation and construction.BuildPlanrefactorDuring kaya's work on #1974 it became clear we needed to reconsider the approach to package graph validation in order to be able to more generally account for all of the different ways a node might have been invalidated since the lock file was last written, and to be able to handle invalidated nodes in a more consistent manner.
This refactors the build plan construction process into the following distinct steps:
Sourcediffers from the source or patch described in the parent's manifest.offline, only fail if we are missing agitnode.Closes #1903.
Closes #1778.
Makes progress on #895, #1787.
PathMapManifestMapThis PR also reduces the number of times that we need to read each dependency's manifest from file in order to reduce the amount of I/O performed. Each dependency's manifest should now be loaded once during either
graph_to_manifest_maporfetch_graph.This required implementing
CloneforManifestFileto allow for constructing the build plan in a manner that does not require consuming the manifest file.ManifestFile::cloneis only ever called for the project manifest, and never for the dependencies.As a side-effect, the
buildandcheckcommands no longer require loading each package's manifest as they are already provided by the build plan. As a result both commands no longer require the sway git tag to be passed through.