Prevent failure while creating BuildPlan from lock file in the case of a dependency removal #1974

kayagokalp · 2022-06-14T18:02:03Z

About this PR

~~Still in WIP, opening up for visibility. I will be adding testing steps~~

Merge Add forc feature for overriding packages in the package graph, akin to cargo's [patch] feature #1836 first and add required patching checks to this one as well

closes #1778.

Motivation

Before this PR, BuildPlan generation from the lock file was failing if after the lock file is generated, a dependency is removed from the manifest file (specifically if the removed dependency is a path dependency).

This was resulting in the BuildPlan being generated from scratch. To generate the BuildPlan from scratch we generally need connectivity since std is implicitly added as a git dependency (if we do not explicitly prevent it with implicit-std = false). Ideally, we should be able to remove any dependency in offline mode.

With this PR, we should be able to create the BuildPlan from the lock file even if a path dependency is removed. This should mean apply_pkg_diff will be able to remove any dependency in offline mode.

To Test

run forc new parent
run forc new --library child
add child as a dependency to parent
run forc build in parent
remove the child from parent's dependencies
run forc build in parent

This should output something like

Creating a new `Forc.lock` file. (Cause: lock file did not match manifest `diff`)

Rather than

Creating a new `Forc.lock` file. (Cause: dependency required for path reconstruction has been removed from the manifest)

…pendency removal from manifest file.

kayagokalp · 2022-06-16T11:11:15Z

I found out a bug about this one (as it can be seen from CI failure) so leaving this as a draft and will come back to here after I am done with some formatter stuff.

forc-pkg/src/pkg.rs

mitchmindtree

Thanks for diving into this @kayagokalp, it's certainly a bit of an awkward one to address.

I'm starting to wonder if we should be doing the validate step as a part of the BuildPlan construction... Ideally it shouldn't be possible to construct a BuildPlan that is invalid. I think originally the idea behind having the validate method be in a following step was to allow for the comparison of the build plans to work out the package diff to print to the user, but perhaps this can just be returned alongside the BuildPlan constructor?

forc-pkg/src/pkg.rs

kayagokalp · 2022-06-17T15:07:35Z

I am applying the changes requested rn and while doing so realized that introduction of #1836 will have a tiny effect on this one. So I would like to merge that one first if possible so that I can add the required changes in this PR as well

kayagokalp · 2022-06-17T18:40:29Z

I'm starting to wonder if we should be doing the validate step as a part of the BuildPlan construction... Ideally it shouldn't be possible to construct a BuildPlan that is invalid. I think originally the idea behind having the validate method be in a following step was to allow for the comparison of the build plans to work out the package diff to print to the user, but perhaps this can just be returned alongside the BuildPlan constructor?

@mitchmindtree, that sounds better to me as I feel like it would allow us to do some extra cleaning as well. Should I file an issue and address it with a separate PR or do you think we should have it with this one?

mitchmindtree

Should I file an issue and address it with a separate PR or do you think we should have it with this one?

Yeah I think it would be great if we could address it in this PR!

forc-pkg/src/pkg.rs

Co-authored-by: mitchmindtree <mitchell.nordine@fuel.sh>

kayagokalp · 2022-06-23T09:54:51Z

I can use a re-review at this point, especially about #1974 (comment).

Also, I will apply #1974 (comment) too other than that I believe It started to look a lot better. Thanks for the extensive reviews so far 🙌

mitchmindtree

Now that we have a load_from_manifest constructor, perhaps we can make the from_lock and from_lock_file constructors private to avoid exposing an invalid BuildPlan to downstream users? We can probably also make the apply_pkg_diff and generate_diff methods private now too.

forc-pkg/src/pkg.rs

kayagokalp · 2022-06-27T09:35:53Z

I think this should be good to go for a final review but I couldn't make the from_lock_file private since sway-lsp uses it.

mitchmindtree · 2022-06-28T04:32:31Z

forc-pkg/src/pkg.rs

+    /// Attempt to find the difference between the graph constructed from the lock file
+    /// and the graph that is described by the manifest file. The difference is used to
+    /// update BuildPlan.
+    pub fn generate_diff(&self, manifest: &Manifest) -> Result<PkgDiff> {


I think we can make this private too

mitchmindtree · 2022-06-28T04:44:24Z

sway-lsp/src/core/document.rs

@@ -162,7 +162,7 @@ impl TextDocument {
            pkg::ManifestFile::from_dir(&manifest_dir, forc::utils::SWAY_GIT_TAG).unwrap();
        let lock_path = forc_util::lock_path(manifest.dir());
        let plan = pkg::BuildPlan::from_lock_file(&lock_path, forc::utils::SWAY_GIT_TAG).unwrap();


Ahh I wonder if this BuildPlan should be constructed via load_from_manifest rather than from_lock_file - otherwise LSP won't pick up changes to the manifest until the user manually triggers a build and updates the lock file again. @JoshuaBatty does this sound right?

If we can change this over to load_from_manifest, we should be able to make from_lock_file private.

Depending on the input from @JoshuaBatty maybe we can do something like

let plan = pkg::BuildPlan::load_from_manifest(&manifest, false, false, forc::utils::SWAY_GIT_TAG) .unwrap_or_else(|_| { pkg::BuildPlan::from_lock_file(&lock_path, forc::utils::SWAY_GIT_TAG) .map(|plan| plan.0) .unwrap() });

I think rather than falling back to from_lock_file, we might be better off producing an error from LSP to the user so that they're aware of whatever the failure is. It might be a bit unexpected if LSP keeps using an old BuildPlan after they've updated the manifest.

If I add a crate to the Cargo.toml file and save it in a rust project then I am able to start using types from that crate and even jump to the definition of items in that crate without needing to build the project again. Not sure if it's rust or rust-analyzer that is downloading the added crate once the manifest file has been saved. It would be nice if we could replicate this behavior somehow.

In that case it sounds like we do want to change the use of from_lock_file to load_from_manifest here - @kayagokalp are you happy to do so in this PR so we can make from_lock_file private too?

Should be good to go but didn't add an explicit error throwing for the failure as we didn't have one for the current version too:

sway/sway-lsp/src/core/document.rs

Line 164 in 92ec0a6

let plan = pkg::BuildPlan::from_lock_file(&lock_path, forc::utils::SWAY_GIT_TAG).unwrap();

Should we change the current behavior and explicitly return an error? or keep the current logic there (which is simply unwrapping)

mitchmindtree · 2022-07-04T07:19:57Z

Hey @kayagokalp, just a heads up that I have my eyes on this! I'm working on a PR with some refactoring in an attempt to simplify all of this a little further and cover a few more failure cases, aiming to post tonight or tomorrow.

mitchmindtree · 2022-07-07T11:47:04Z

Closing as these commits were all included as a part of #2234.

BuildPlan generation from lock file does not break when there is a de…

f985bf0

…pendency removal from manifest file.

kayagokalp added bug Something isn't working forc dependencies labels Jun 14, 2022

kayagokalp self-assigned this Jun 14, 2022

While removing the deleted nodes handle orphaned childs

b4219a7

kayagokalp requested review from ra0x3 and a team June 16, 2022 18:12

kayagokalp commented Jun 16, 2022

View reviewed changes

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

kayagokalp marked this pull request as ready for review June 16, 2022 18:13

removed debug println

9495057

kayagokalp requested a review from eureka-cpu June 16, 2022 18:16

kayagokalp enabled auto-merge (squash) June 16, 2022 21:35

mitchmindtree reviewed Jun 17, 2022

View reviewed changes

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

kayagokalp disabled auto-merge June 17, 2022 15:07

kayagokalp marked this pull request as draft June 17, 2022 15:09

kayagokalp added 3 commits June 17, 2022 19:10

removed_deps removed from BuildPlan

e97c684

While checking from parent's manifest check for package name as well

38af42e

removed unnecessary reference

e469399

mitchmindtree reviewed Jun 20, 2022

View reviewed changes

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Show resolved Hide resolved

kayagokalp added 3 commits June 20, 2022 12:21

split validate into generate_diff and validate

4be5880

apply_pkg_diff visits nodes to delete in reverse BFS order

aa90853

docs added for generate_diff

316a871

kayagokalp marked this pull request as ready for review June 20, 2022 15:45

Merge branch 'master' into kayagokalp/1778

4548afb

mitchmindtree suggested changes Jun 21, 2022

View reviewed changes

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

forc-pkg/src/pkg.rs Show resolved Hide resolved

forc-pkg/src/pkg.rs Show resolved Hide resolved

mitchmindtree mentioned this pull request Jun 22, 2022

Add forc check command #2026

Merged

kayagokalp and others added 4 commits June 23, 2022 12:26

removed unnecessary logic in apply_diff_after_lock

38cc164

doc updates

344ceaa

clippy && extra dereferencing removed

cb255e2

Apply suggestions from code review effor -> effort

cb6a2e0

Co-authored-by: mitchmindtree <mitchell.nordine@fuel.sh>

kayagokalp requested a review from mitchmindtree June 23, 2022 09:55

mitchmindtree reviewed Jun 27, 2022

View reviewed changes

forc-pkg/src/pkg.rs Outdated Show resolved Hide resolved

kayagokalp added 6 commits June 27, 2022 10:38

duplicate code removed

bb40017

change visibility of apply_pkg_diff and from_lock to private

1905046

doc updated

5d466ec

Merge master

6de10da

patch checks are added

4ac21d4

check for patches corrected

057afeb

kayagokalp requested a review from mitchmindtree June 27, 2022 09:35

kayagokalp enabled auto-merge (squash) June 27, 2022 09:36

mitchmindtree reviewed Jun 28, 2022

View reviewed changes

kayagokalp and others added 3 commits June 29, 2022 10:52

lsp uses load_from_manifest, from_lock_file is private

f11442b

Merge branch 'master' into kayagokalp/1778

476efa7

add use::collections::HashSet

2bc0d94

kayagokalp requested review from JoshuaBatty and mitchmindtree June 29, 2022 20:57

Merge branch 'master' into kayagokalp/1778

b590f03

mitchmindtree mentioned this pull request Jul 4, 2022

forc-pkg: Improve error handling around removed path deps and patches #2209

Merged

mitchmindtree mentioned this pull request Jul 6, 2022

forc-pkg: Refactor BuildPlan construction and validation to minimize fetching, improve offline support, reduce I/O, fix bugs. #2234

Merged

mitchmindtree closed this Jul 7, 2022

auto-merge was automatically disabled July 7, 2022 11:47
Pull request was closed

kayagokalp deleted the kayagokalp/1778 branch August 30, 2022 22:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent failure while creating BuildPlan from lock file in the case of a dependency removal #1974

Prevent failure while creating BuildPlan from lock file in the case of a dependency removal #1974

kayagokalp commented Jun 14, 2022 •

edited by mitchmindtree

Loading

kayagokalp commented Jun 16, 2022

mitchmindtree left a comment

kayagokalp commented Jun 17, 2022

kayagokalp commented Jun 17, 2022 •

edited

Loading

mitchmindtree left a comment

kayagokalp commented Jun 23, 2022

mitchmindtree left a comment •

edited

Loading

kayagokalp commented Jun 27, 2022

mitchmindtree Jun 28, 2022

mitchmindtree Jun 28, 2022

kayagokalp Jun 28, 2022

mitchmindtree Jun 28, 2022

JoshuaBatty Jun 29, 2022 •

edited

Loading

mitchmindtree Jun 29, 2022 •

edited

Loading

kayagokalp Jun 29, 2022

mitchmindtree commented Jul 4, 2022

mitchmindtree commented Jul 7, 2022

Prevent failure while creating BuildPlan from lock file in the case of a dependency removal #1974

Prevent failure while creating BuildPlan from lock file in the case of a dependency removal #1974

Conversation

kayagokalp commented Jun 14, 2022 • edited by mitchmindtree Loading

About this PR

Motivation

To Test

kayagokalp commented Jun 16, 2022

mitchmindtree left a comment

Choose a reason for hiding this comment

kayagokalp commented Jun 17, 2022

kayagokalp commented Jun 17, 2022 • edited Loading

mitchmindtree left a comment

Choose a reason for hiding this comment

kayagokalp commented Jun 23, 2022

mitchmindtree left a comment • edited Loading

Choose a reason for hiding this comment

kayagokalp commented Jun 27, 2022

mitchmindtree Jun 28, 2022

Choose a reason for hiding this comment

mitchmindtree Jun 28, 2022

Choose a reason for hiding this comment

kayagokalp Jun 28, 2022

Choose a reason for hiding this comment

mitchmindtree Jun 28, 2022

Choose a reason for hiding this comment

JoshuaBatty Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

mitchmindtree Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

kayagokalp Jun 29, 2022

Choose a reason for hiding this comment

mitchmindtree commented Jul 4, 2022

mitchmindtree commented Jul 7, 2022

kayagokalp commented Jun 14, 2022 •

edited by mitchmindtree

Loading

kayagokalp commented Jun 17, 2022 •

edited

Loading

mitchmindtree left a comment •

edited

Loading

JoshuaBatty Jun 29, 2022 •

edited

Loading

mitchmindtree Jun 29, 2022 •

edited

Loading