Basic implementation for `cargo install --dry-run` #13598

PaulDance · 2024-03-17T22:30:39Z

What does this PR try to resolve?

This is an attempt at providing a starting implementation for cargo install --dry-run discussed in #11123. The precise objective is more to have something that works now rather than a much more complete version requiring time and complexity-facing skills I don't currently have.

Indeed, #11123 (comment) suggested to descend into InstallablePackage::install_one and its inner guts. That would enable getting much more accurate information relatively to the usual non-dry-run operation. However, it looked a bit too complicated for a reviewable first step and also not too orthogonal with the build-plan and unit-graph features. The plan I adopted in this attempt is therefore to:

only handle the CLI flag in cargo_install::install;
basically only use the information provided by InstallablePackage::new and other easily-retrievable ones;
put itself between that and the start of the compilation to cut it entirely short;
display status messages indicating which very broad actions would be performed, but not much more.

As a quick preview:

$ cargo run --bin cargo -- -Z unstable-options install --dry-run cargo-liner gitoxide bat git-delta
    Finished dev [unoptimized + debuginfo] target(s) in 0.14s
     Running `target/debug/cargo -Z unstable-options install --dry-run cargo-liner gitoxide bat git-delta`
    Updating crates.io index
     Ignored package `cargo-liner v0.6.0` is already installed, use --force to override
    Updating crates.io index
  Downloaded gitoxide v0.33.0
  Downloaded 1 crate (75.5 KB) in 0.18s
     Ignored package `bat v0.24.0` is already installed, use --force to override
     Summary dry-run
 Destination /home/pauldance/.cargo/bin
     Install gitoxide: to 0.33.0 using registry `crates-io`
     Upgrade git-delta: from 0.16.5 to 0.17.0 using registry `crates-io`
  Up-to-date cargo-liner
  Up-to-date bat

How should we test and review this PR?

Several tests have been added in order to check that the usual combinations of the flag with --git, --path and installation states behave as expected and print the desired outputs. See the end of tests/testsuite/install.rs for more details about this.

Review should be made easier using the few provided commits:

CLI change.
Implementation.
Tests.
Docs.

In particular, I might not be too fond of the wording I chose here and there. The way the information is displayed in a split manner using several new status kinds might not be the best either considering the rest of Cargo's output. Feel free to correct me on any of this.

Additional information

This is very much a first implementation that could evolve in the near future depending on the precision of the emulation that is desirable. As-is, most of the operation is entirely avoided and only informational messages are printed instead. This feels to me enough for a first step. Being more precise would require emulating the compilation jobs or even their inner operations, at the cost of additional complexity.

Obviously, any feedback is welcome, especially if it is about the next steps potentially requiring the mentioned accuracy or if these should be done now instead.

Signed-off-by: Paul Mabileau <[email protected]>

rustbot · 2024-03-17T22:30:45Z

r? @epage

rustbot has assigned @epage.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

PaulDance · 2024-03-17T22:31:57Z

r? @weihanglo since you kindly wrote the mentor notes.

PaulDance · 2024-03-18T13:45:08Z

src/cargo/ops/cargo_install.rs

+                    &[krate.map_or_else(
+                        || -> CargoResult<&str> {
+                            // `krate` is `None` usually for a Git source at
+                            // this point. Select the package again in order to
+                            // be able to retrieve its name.
+                            if let SourceKind::Git(_) = source_id.kind() {
+                                Ok(select_pkg(
+                                    &mut GitSource::new(source_id, gctx)?,
+                                    None,
+                                    |git_src| git_src.read_packages(),
+                                    gctx,
+                                    current_rust_version.as_ref(),
+                                )?
+                                .name()
+                                .as_str())
+                            } else {
+                                Ok("")
+                            }
+                        },
+                        |kr| Ok(kr),
+                    )?],


This feels hackish to me, but I couldn't find a better way. If you have one, I would be glad to integrate it instead.

PaulDance · 2024-03-18T13:48:02Z

I also don't know exactly what to do about check-version-bump this time: should I simply bump the cargo library minor version?

epage · 2024-03-18T14:13:15Z

src/bin/cargo/commands/install.rs

+        .arg(
+            flag(
+                "dry-run",
+                "Display what would be installed without actually performing anything (unstable)",
+            )
+            .short('n'),
+        )


FYI

cargo/src/cargo/util/command_prelude.rs

Lines 350 to 352 in 6972630

fn arg_dry_run(self, dry_run: &'static str) -> Self {

self._arg(flag("dry-run", dry_run).short('n'))

}

Ah yes, didn't see that one, thanks!

epage · 2024-03-18T14:14:07Z

src/bin/cargo/commands/install.rs

@@ -201,6 +208,9 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
    if args.flag("list") {
        ops::install_list(root, gctx)?;
    } else {
+        if args.flag("dry-run") {


FYI

cargo/src/cargo/util/command_prelude.rs

Lines 529 to 531 in 6972630

fn dry_run(&self) -> bool {

self.flag("dry-run")

}

epage · 2024-03-18T14:37:07Z

tests/testsuite/install.rs

@@ -2623,3 +2623,598 @@ fn uninstall_running_binary() {
        )
        .run();
 }
+
+#[cargo_test(nightly, reason = "--dry-run is unstable")]


These do not need nightly, only to masquerade as nightly

epage · 2024-03-18T14:37:33Z

tests/testsuite/install.rs

@@ -2623,3 +2623,598 @@ fn uninstall_running_binary() {
        )
        .run();
 }
+
+#[cargo_test(nightly, reason = "--dry-run is unstable")]
+fn dry_run_freshinstall_one() {


Please add these tests in their own commit at the start, without the dry-run flag, so we can compare what the dry-run flag does to the output

Just to be clear, you want a copy of each test where only the flag is removed and the output adapted in consequence, right? Also, when you say "add these tests in their own commit at the start", are you referring to the already-written tests, the "copies" that need writing or all of them?

Each test that exists in this PR should be added in their own commit, the first one. The --dry-run flag would be missing. These tests would pass.

The next commit in the PR would update the tests to add --dry-run and update the output accordingly.

epage · 2024-03-18T14:37:46Z

tests/testsuite/install.rs

@@ -2623,3 +2623,598 @@ fn uninstall_running_binary() {
        )
        .run();
 }
+
+#[cargo_test(nightly, reason = "--dry-run is unstable")]
+fn dry_run_freshinstall_one() {


freshinstall is two words

epage · 2024-03-18T14:43:27Z

crates/cargo-test-support/src/compare.rs

@@ -222,6 +222,12 @@ fn substitute_macros(input: &str) -> String {
        ("[REPLACING]", "   Replacing"),
        ("[UNPACKING]", "   Unpacking"),
        ("[SUMMARY]", "     Summary"),
+        ("[DESTINATION]", " Destination"),


Please add these in the commit where they are used

epage · 2024-03-18T14:46:24Z

crates/cargo-test-support/src/compare.rs

+        ("[INSTALL]", "     Install"),
+        ("[UPGRADE]", "     Upgrade"),
+        ("[DOWNGRADE]", "   Downgrade"),
+        ("[UPTODATE]", "  Up-to-date"),
+        ("[REINSTALL]", "  Re-install"),


imo the dry-run should generally match the regular behavior.

If we want to improve the cargo install output, that is a change on its own to be evaluated.

epage · 2024-03-18T14:55:30Z

src/cargo/ops/cargo_install.rs

+        if dry_run {
+            match installable_pkg {
+                Some(installable_pkg) => dry_run_report(


See #11123 (comment)

If this is indeed required, I'll see what I can do to rework this and go into the more fine-grained compilation steps to have a proper dry-run. It might take some time though, as it is almost starting from scratch.

@rustbot author

bors · 2024-03-19T16:31:33Z

☔ The latest upstream changes (presumably #13602) made this pull request unmergeable. Please resolve the merge conflicts.

Add a `--dry-run` flag to the `install` command ### What does this PR try to resolve? This PR add the `--dry-run` flag to the `cargo install` command (see #11123). I've tried to do the bare minimal for this flag to work without changing anything in the output. In my opinion, the `--dry-run` flag should mimic as close as possible the behavior of the normal command to avoid missing potential issue in the normal execution. ~~Currently we're missing information about where the binary will be installed.~~ Unlike #13598 this PR: - Include as much of the compilation process as possible without actually compiling - use the information provided by `BuildContext` instead of `InstallablePackage::new` - in the same way as `unit_graph`, it add a `dry_run` to the `CompileOptions` and return a `Compilation::new` from the function `compile_ws` without actually compiling. - keeps the output the same rather than adding status messages indicating which very broad actions would be performed - ~~remove some warning not relevant in the case of a `--dry-run`~~ Like #13598, the version check and crate downloads still occur. ### How should we test and review this PR? The first commit include a unit tests to ensure that no binary is actually installed after the dry run. There is also a snapshot test that show the diff output of the `--help` flag. ### Additional information Tests and documentation done in #13598, may be cherry picked into this PR if needed.

PaulDance added 5 commits March 17, 2024 22:29

Feat(install/cli): Add new gated --dry-run option

cf2f775

Signed-off-by: Paul Mabileau <[email protected]>

Feat(install): Implement --dry-run

b56d71e

Signed-off-by: Paul Mabileau <[email protected]>

Feat(test/compare): Add new status headers to substitute_macros

f842f64

Signed-off-by: Paul Mabileau <[email protected]>

Test(install): Add tests for --dry-run

3b0852d

Signed-off-by: Paul Mabileau <[email protected]>

Docs(install): Add --dry-run

419d155

Signed-off-by: Paul Mabileau <[email protected]>

rustbot assigned epage Mar 17, 2024

rustbot assigned weihanglo and unassigned epage Mar 17, 2024

PaulDance commented Mar 18, 2024

View reviewed changes

epage reviewed Mar 18, 2024

View reviewed changes

epage mentioned this pull request Mar 18, 2024

cargo install --dry-run #11123

Open

epage reviewed Mar 18, 2024

View reviewed changes

rustbot added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2024

epage closed this Jun 5, 2024

knzai mentioned this pull request Jul 15, 2024

Support for idempotent installs/only install if missing? taiki-e/install-action#577

Open

Flowrey mentioned this pull request Jul 21, 2024

Add a --dry-run flag to the install command #14280

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic implementation for `cargo install --dry-run` #13598

Basic implementation for `cargo install --dry-run` #13598

PaulDance commented Mar 17, 2024

rustbot commented Mar 17, 2024

PaulDance commented Mar 17, 2024

PaulDance Mar 18, 2024

PaulDance commented Mar 18, 2024

epage Mar 18, 2024

PaulDance Mar 26, 2024

epage Mar 18, 2024

epage Mar 18, 2024

epage Mar 18, 2024

PaulDance Mar 26, 2024

epage Mar 26, 2024

PaulDance Mar 27, 2024

epage Mar 18, 2024

epage Mar 18, 2024

epage Mar 18, 2024

epage Mar 18, 2024

PaulDance Mar 27, 2024

bors commented Mar 19, 2024

	fn arg_dry_run(self, dry_run: &'static str) -> Self {
	self._arg(flag("dry-run", dry_run).short('n'))
	}

Basic implementation for cargo install --dry-run #13598

Basic implementation for cargo install --dry-run #13598

Conversation

PaulDance commented Mar 17, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rustbot commented Mar 17, 2024

PaulDance commented Mar 17, 2024

Choose a reason for hiding this comment

PaulDance commented Mar 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Mar 19, 2024

Basic implementation for `cargo install --dry-run` #13598

Basic implementation for `cargo install --dry-run` #13598