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

cargo install --dry-run #11123

Open
epage opened this issue Sep 22, 2022 · 23 comments
Open

cargo install --dry-run #11123

epage opened this issue Sep 22, 2022 · 23 comments
Assignees
Labels
C-enhancement Category: enhancement call-for-testing Marks issues that require broader testing from the community, e.g. before stabilization. Command-install E-medium Experience: Medium S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.

Comments

@epage
Copy link
Contributor

epage commented Sep 22, 2022

--dry-run is an important feature for commands with side effects to help

  • Give users confidence in what the commands will do (e.g. what binaries will be installed)
  • Offer a tool for users and developers to debug subsets of the functionality
  • Allow scripts to check the status of the system

Testing instructions: #11123 (comment)

@epage epage added Command-install C-enhancement Category: enhancement labels Sep 22, 2022
@weihanglo weihanglo added the S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. label May 24, 2023
@weihanglo
Copy link
Member

This is a feature to pursue. We can make it as an unstable feature behind -Zunstale-options. However, it is not really in a high priority, so label it as S-needs-mentor for now.

@PaulDance
Copy link
Contributor

Just thought about this and it would seem nice indeed. My personal use case would be to be able to detect whether a package needs:

  • an install: Would install <pkg>@<version>;
  • an update: Would update <pkg> from <old-ver> to <new-ver>;
  • or nothing: the same Already up-to-date.

or something along those lines, as long as it has the necessary information, at least these.

Indeed, if I'm not mistaken, the install command currently performs its duties linearly one package at a time if multiple are given. Therefore, when giving a good bunch of them that are already installed, the version check can take quite a bit of time and end up not starting any kind of actual installation if all are already up-to-date for example. As this specific part is mostly IO-bound, then performing it concurrently gives an easy performance boost. The option would thus enable that easily as well.

Although it not high priority for me either, as the buggy version check that I currently have based on cargo search will call cargo install on some packages just for nothing but without any consequence, I would therefore be pleased to at least attempt some things towards implementing this. The mentoring part would definitely be of use to me however, so if you have time in the near future, it would be great.

@weihanglo
Copy link
Member

Hey @PaulDance. Thanks for being interested in this.

Thinking backwards from what users want, here is my idea that the initial version of --dry-run could have:

  • How: Is it an update (replace), a fresh install, or no-op?
  • What: which binary from which source is going to be installed
  • Where: to where the binary will be installed

There might be somethings hard to do in the first PR, like programming interface for scripting, or checking if yanked versions will block the installation. That's fine, since we plan to put it behind -Zunstable-options (the process of adding it can be found here). It is benefical for maintainers to review the implementation, if we can break down a feature a into several small PRs. It also increases the chance to merge them sooner.

For the implementation, it might not be as easy as it looks like at first glance. It depends on how acurate we want for cargo install --dry-run. The best option we have is collecting the info after Cargo computing all the necessary build tasks.

  • In cargo install, the build task computation starts from here. Down from there, the actual compilation starts from this line. After that Cargo collects the compilation results into the Compilation struct. These steps are all in one function Context::compile(). If you have an idea of refactoring, Don't hesitate. But beware, the Context::compile() function is already complicated. We should keep it simple as possible as we can.
  • You might want to take a look at build-plans and unit graph features. They might be good references for this feature, as they do similar stuff: computing build tasks without excuting them.
  • We care only BuildContext::roots units, as they are bins/libs requested from the command line to install.

If you have any question, find us on Zulip t-cargo or in the office hours. Cheers!

@weihanglo weihanglo added S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review E-medium Experience: Medium and removed S-needs-mentor Status: Issue or feature is accepted, but needs a team member to commit to helping and reviewing. labels Jan 25, 2024
@epage
Copy link
Contributor Author

epage commented Jan 25, 2024

@weihanglo I think I'm missing where you are going with talking about Context::compile. Could you clarify the connection?

@epage
Copy link
Contributor Author

epage commented Jan 25, 2024

My personal use case would be to be able to detect whether a package needs:

For querying the state (needs install, needs update, updated), we have the data (crates2.json) but we a cargo install --dry-run wouldn't have the programmatic API to access that information except through whatever errors via combinations of existing flags we already provide. Waiting on #4379 for more programmatic output might take a while. I wonder if the better route would be to tie these questions into more general version management in #9527

@weihanglo
Copy link
Member

@weihanglo I think I'm missing where you are going with talking about Context::compile. Could you clarify the connection?

To learn where Cargo will install the binaries, we need output infos that are computed in Context::compile.

@LuuuXXX
Copy link
Contributor

LuuuXXX commented Feb 28, 2024

claim first, question it later~ @rustbot claim

@PaulDance
Copy link
Contributor

As long as it ends up being implemented with things I care for or that I could add easily, I don't mind, please go ahead. I simply didn't find the time for it yet.

@LuuuXXX
Copy link
Contributor

LuuuXXX commented Feb 29, 2024

hi~, If there are any good implementations and plans, I'd love to see them happen.

@LuuuXXX LuuuXXX removed their assignment Feb 29, 2024
@PaulDance
Copy link
Contributor

but a cargo install --dry-run wouldn't have the programmatic API to access that information except through whatever errors via combinations of existing flags we already provide.

Wouldn't the standard error stream's content be enough to achieve this for now, even if there wouldn't be any stability guarantee?

Waiting on #4379 for more programmatic output might take a while. I wonder if the better route would be to tie these questions into more general version management in #9527.

These would be better indeed, but as you said, they might take a while. I'm therefore looking for some middle ground that could be easier and quicker to integrate in the meantime.

@PaulDance
Copy link
Contributor

@rustbot claim

@epage
Copy link
Contributor Author

epage commented Mar 18, 2024

Was going to comment on #13598 but as this is on more fundamental requirements, posting it here.

We have a couple different points where we can stub out behavior for dry-run

Install is more complete at the cost of more time to perform (and less invasive). The challenge is each user might come to dry-run with different needs. In terms of precedence, we do the full work in cargo publish --dry-run.

@epage
Copy link
Contributor Author

epage commented Jun 5, 2024

@PaulDance are you still working on this?

@PaulDance
Copy link
Contributor

Not at the moment, no, sorry. You can probably unassign me from here and close the PR since a full rewrite is required anyway.

@Flowrey
Copy link
Contributor

Flowrey commented Jul 21, 2024

@rustbot claim

bors added a commit that referenced this issue Sep 21, 2024
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.
@epage
Copy link
Contributor Author

epage commented Nov 6, 2024

Call for testing

cargo install --dry-run is now available and we're looking for feedback to see if this can be stabilized!

Basic steps:

Requirements:

Try cargo install --dry-run on the various cargo install workflows you use

In particular, consider

  • Testing under different circumstances (e.g. sources, upgrade, etc)
  • If this provides the right level of fidelity you are looking for (e.g. is doing the compile helpful feedback or slow you down from what you are looking for)
  • If this changed anything it shouldn't

Please leave feedback on this issue

@weihanglo weihanglo added the S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns. label Nov 6, 2024
@PaulDance
Copy link
Contributor

From the few simple tests I conducted, things seem to work fine 🙂

@PaulDance
Copy link
Contributor

In the case where a package needs to actually be installed or updated, the crates are in fact really downloaded and not just printed as such, right? If so, would it be possible to make it so that the same lines would be printed without actually downloading the crates or would it be too complicated to achieve in this iteration? Just looking to add a little speed without sacrificing end result information.

@epage
Copy link
Contributor Author

epage commented Nov 30, 2024

We need to download to discover what bins are present.

A general question though is what level of fidelity/reporting do people want from this? For example, yank or msrv messages will only show with a compilation.

@PaulDance
Copy link
Contributor

We need to download to discover what bins are present.

For the top crate or for all? I would have guessed that after downloading the requested crate and locking the dependencies, downloading the dependencies would be optional.

@epage
Copy link
Contributor Author

epage commented Nov 30, 2024

True but that still runs into the second half of my comment.

@PaulDance
Copy link
Contributor

My personal use case would be to determine whether something needs to be done or not. What exactly needs to happen is not of importance for me, in this first iteration at least. Fidelity is not that important for me in this regard and I would therefore be fine with Cargo only printing that it downloaded some crates while it actually did not, which should be compatible with the notion of dry run. This way, the level of reported information should stay the same while it just goes a little bit quicker.

I'll let others comment on what exactly they would need, though. For example, what happens after the downloads is not simulated at all. That part may be easier to achieve once the build plan feature lands, if I remember correctly at least.

@epage epage added the call-for-testing Marks issues that require broader testing from the community, e.g. before stabilization. label Dec 13, 2024
@epage
Copy link
Contributor Author

epage commented Dec 16, 2024

A middle-ground solution could be that we switch cargo install to do a cargo check --profile dev instead of cargo build --profile release. Dry-run mode will be faster while we still get all of the error reporting. Care will need to be taken in case we rely on the full results of cargo build to male sure everything still works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: enhancement call-for-testing Marks issues that require broader testing from the community, e.g. before stabilization. Command-install E-medium Experience: Medium S-accepted Status: Issue or feature is accepted, and has a team member available to help mentor or review S-waiting-on-feedback Status: An implemented feature is waiting on community feedback for bugs or design concerns.
Projects
None yet
Development

No branches or pull requests

5 participants