Skip to content
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

Fix dependency injection: use localised packages #39

Merged
merged 2 commits into from
Feb 26, 2021

Conversation

gnunicorn
Copy link
Contributor

So far we injected the local workspace when building the unpacked package.
However, when we are in a workspace with changes compared to dependencies
that are already released, cargo sometimes pulls in the remote and local
package in the check then has dependency mismatches.

This change fixes that by collecting the locally build packages during the
checking-process, and injects only those that it previously build. This
prevents the issue by not ever introducing packages from the local workspace.

So far we injected the local workspace when building the unpacked package.
However, when we are in a workspace with changes compared to dependencies
that are already released, cargo sometimes pulls in the remote and local
package in the check then has dependency mismatches.

This change fixes that by collecting the locally build packages during the
checking-process, and injects only those that it previously build. This
prevents the issue by not ever introducing packages from the local workspace.
@gnunicorn gnunicorn requested a review from Xanewok February 25, 2021 13:30
Copy link
Contributor

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementation LGTM as we discussed, it'd be great to add a comment with rationale

@@ -319,14 +305,29 @@ pub fn check<'a>(
};

c.shell().status("Checking", "Packages")?;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Extra line

@@ -319,14 +305,29 @@ pub fn check<'a>(
};

c.shell().status("Checking", "Packages")?;



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be great to at least add a terse comment explaining why we can't calculate all of that beforehand, as we did before

@Xanewok
Copy link
Contributor

Xanewok commented Feb 25, 2021

Rustfmt is unhappy:

Diff in /home/runner/work/cargo-unleash/cargo-unleash/src/commands/check.rs at line 306:
 
     c.shell().status("Checking", "Packages")?;
 
-
-
     let mut replaces = HashMap::new();
 
     for (pkg_ws, rw_lock) in successes.iter().filter_map(|e| e.as_ref().ok()) {
Diff in /home/runner/work/cargo-unleash/cargo-unleash/src/commands/check.rs at line 321:
         let new_pkg = ws.current().expect("Each workspace is for a package!");
         replaces.insert(
             new_pkg.name().as_str().to_owned(),
-            new_pkg.manifest_path()
-                        .parent()
-                        .expect("Folder exists")
-                        .to_str()
-                        .expect("Is stringifiable")
-                        .to_owned(),
+            new_pkg
+                .manifest_path()
+                .parent()
+                .expect("Folder exists")
+                .to_str()
+                .expect("Is stringifiable")
+                .to_owned(),

@gnunicorn gnunicorn requested a review from Xanewok February 26, 2021 16:35
Copy link
Contributor

@Xanewok Xanewok left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@Xanewok Xanewok merged commit 6c09cac into master Feb 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants