Skip to content

Commit

Permalink
Auto merge of #14280 - Flowrey:dry-run-install, r=weihanglo
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bors committed Sep 21, 2024
2 parents 6469a3e + b0e08fc commit d7bffc3
Show file tree
Hide file tree
Showing 11 changed files with 324 additions and 91 deletions.
6 changes: 5 additions & 1 deletion src/bin/cargo/commands/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ pub fn cli() -> Command {
)
.arg(opt("root", "Directory to install packages into").value_name("DIR"))
.arg(flag("force", "Force overwriting existing crates or binaries").short('f'))
.arg_dry_run("Perform all checks without installing (unstable)")
.arg(flag("no-track", "Do not save tracking information"))
.arg(flag(
"list",
Expand Down Expand Up @@ -200,7 +201,9 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {

compile_opts.build_config.requested_profile =
args.get_profile_name("release", ProfileChecking::Custom)?;

if args.dry_run() {
gctx.cli_unstable().fail_if_stable_opt("--dry-run", 11123)?;
}
if args.flag("list") {
ops::install_list(root, gctx)?;
} else {
Expand All @@ -213,6 +216,7 @@ pub fn exec(gctx: &mut GlobalContext, args: &ArgMatches) -> CliResult {
&compile_opts,
args.flag("force"),
args.flag("no-track"),
args.dry_run(),
)?;
}
Ok(())
Expand Down
3 changes: 3 additions & 0 deletions src/cargo/core/compiler/build_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ pub struct BuildConfig {
pub build_plan: bool,
/// Output the unit graph to stdout instead of actually compiling.
pub unit_graph: bool,
/// `true` to avoid really compiling.
pub dry_run: bool,
/// An optional override of the rustc process for primary units
pub primary_unit_rustc: Option<ProcessBuilder>,
/// A thread used by `cargo fix` to receive messages on a socket regarding
Expand Down Expand Up @@ -112,6 +114,7 @@ impl BuildConfig {
force_rebuild: false,
build_plan: false,
unit_graph: false,
dry_run: false,
primary_unit_rustc: None,
rustfix_diagnostic_server: Rc::new(RefCell::new(None)),
export_dir: None,
Expand Down
74 changes: 49 additions & 25 deletions src/cargo/core/compiler/build_runner/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,27 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
})
}

/// Dry-run the compilation without actually running it.
///
/// This is expected to collect information like the location of output artifacts.
/// Please keep in sync with non-compilation part in [`BuildRunner::compile`].
pub fn dry_run(mut self) -> CargoResult<Compilation<'gctx>> {
let _lock = self
.bcx
.gctx
.acquire_package_cache_lock(CacheLockMode::Shared)?;
self.lto = super::lto::generate(self.bcx)?;
self.prepare_units()?;
self.prepare()?;
self.check_collisions()?;

for unit in &self.bcx.roots {
self.collect_tests_and_executables(unit)?;
}

Ok(self.compilation)
}

/// Starts compilation, waits for it to finish, and returns information
/// about the result of compilation.
///
Expand Down Expand Up @@ -214,31 +235,7 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {

// Collect the result of the build into `self.compilation`.
for unit in &self.bcx.roots {
// Collect tests and executables.
for output in self.outputs(unit)?.iter() {
if output.flavor == FileFlavor::DebugInfo || output.flavor == FileFlavor::Auxiliary
{
continue;
}

let bindst = output.bin_dst();

if unit.mode == CompileMode::Test {
self.compilation
.tests
.push(self.unit_output(unit, &output.path));
} else if unit.target.is_executable() {
self.compilation
.binaries
.push(self.unit_output(unit, bindst));
} else if unit.target.is_cdylib()
&& !self.compilation.cdylibs.iter().any(|uo| uo.unit == *unit)
{
self.compilation
.cdylibs
.push(self.unit_output(unit, bindst));
}
}
self.collect_tests_and_executables(unit)?;

// Collect information for `rustdoc --test`.
if unit.mode.is_doc_test() {
Expand Down Expand Up @@ -307,6 +304,33 @@ impl<'a, 'gctx> BuildRunner<'a, 'gctx> {
Ok(self.compilation)
}

fn collect_tests_and_executables(&mut self, unit: &Unit) -> CargoResult<()> {
for output in self.outputs(unit)?.iter() {
if output.flavor == FileFlavor::DebugInfo || output.flavor == FileFlavor::Auxiliary {
continue;
}

let bindst = output.bin_dst();

if unit.mode == CompileMode::Test {
self.compilation
.tests
.push(self.unit_output(unit, &output.path));
} else if unit.target.is_executable() {
self.compilation
.binaries
.push(self.unit_output(unit, bindst));
} else if unit.target.is_cdylib()
&& !self.compilation.cdylibs.iter().any(|uo| uo.unit == *unit)
{
self.compilation
.cdylibs
.push(self.unit_output(unit, bindst));
}
}
Ok(())
}

/// Returns the executable for the specified unit (if any).
pub fn get_executable(&mut self, unit: &Unit) -> CargoResult<Option<PathBuf>> {
let is_binary = unit.target.is_executable();
Expand Down
6 changes: 5 additions & 1 deletion src/cargo/ops/cargo_compile/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,11 @@ pub fn compile_ws<'a>(
}
crate::core::gc::auto_gc(bcx.gctx);
let build_runner = BuildRunner::new(&bcx)?;
build_runner.compile(exec)
if options.build_config.dry_run {
build_runner.dry_run()
} else {
build_runner.compile(exec)
}
}

/// Executes `rustc --print <VALUE>`.
Expand Down
67 changes: 43 additions & 24 deletions src/cargo/ops/cargo_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ impl<'gctx> InstallablePackage<'gctx> {
Ok(duplicates)
}

fn install_one(mut self) -> CargoResult<bool> {
fn install_one(mut self, dry_run: bool) -> CargoResult<bool> {
self.gctx.shell().status("Installing", &self.pkg)?;

let dst = self.root.join("bin").into_path_unlocked();
Expand All @@ -321,6 +321,7 @@ impl<'gctx> InstallablePackage<'gctx> {
self.check_yanked_install()?;

let exec: Arc<dyn Executor> = Arc::new(DefaultExecutor);
self.opts.build_config.dry_run = dry_run;
let compile = ops::compile_ws(&self.ws, &self.opts, &exec).with_context(|| {
if let Some(td) = td_opt.take() {
// preserve the temporary directory, so the user can inspect it
Expand Down Expand Up @@ -419,13 +420,15 @@ impl<'gctx> InstallablePackage<'gctx> {
let staging_dir = TempFileBuilder::new()
.prefix("cargo-install")
.tempdir_in(&dst)?;
for &(bin, src) in binaries.iter() {
let dst = staging_dir.path().join(bin);
// Try to move if `target_dir` is transient.
if !self.source_id.is_path() && fs::rename(src, &dst).is_ok() {
continue;
if !dry_run {
for &(bin, src) in binaries.iter() {
let dst = staging_dir.path().join(bin);
// Try to move if `target_dir` is transient.
if !self.source_id.is_path() && fs::rename(src, &dst).is_ok() {
continue;
}
paths::copy(src, &dst)?;
}
paths::copy(src, &dst)?;
}

let (to_replace, to_install): (Vec<&str>, Vec<&str>) = binaries
Expand All @@ -441,11 +444,13 @@ impl<'gctx> InstallablePackage<'gctx> {
let src = staging_dir.path().join(bin);
let dst = dst.join(bin);
self.gctx.shell().status("Installing", dst.display())?;
fs::rename(&src, &dst).with_context(|| {
format!("failed to move `{}` to `{}`", src.display(), dst.display())
})?;
installed.bins.push(dst);
successful_bins.insert(bin.to_string());
if !dry_run {
fs::rename(&src, &dst).with_context(|| {
format!("failed to move `{}` to `{}`", src.display(), dst.display())
})?;
installed.bins.push(dst);
successful_bins.insert(bin.to_string());
}
}

// Repeat for binaries which replace existing ones but don't pop the error
Expand All @@ -456,10 +461,12 @@ impl<'gctx> InstallablePackage<'gctx> {
let src = staging_dir.path().join(bin);
let dst = dst.join(bin);
self.gctx.shell().status("Replacing", dst.display())?;
fs::rename(&src, &dst).with_context(|| {
format!("failed to move `{}` to `{}`", src.display(), dst.display())
})?;
successful_bins.insert(bin.to_string());
if !dry_run {
fs::rename(&src, &dst).with_context(|| {
format!("failed to move `{}` to `{}`", src.display(), dst.display())
})?;
successful_bins.insert(bin.to_string());
}
}
Ok(())
};
Expand All @@ -476,9 +483,14 @@ impl<'gctx> InstallablePackage<'gctx> {
&self.rustc.verbose_version,
);

if let Err(e) =
remove_orphaned_bins(&self.ws, &mut tracker, &duplicates, &self.pkg, &dst)
{
if let Err(e) = remove_orphaned_bins(
&self.ws,
&mut tracker,
&duplicates,
&self.pkg,
&dst,
dry_run,
) {
// Don't hard error on remove.
self.gctx
.shell()
Expand Down Expand Up @@ -515,7 +527,10 @@ impl<'gctx> InstallablePackage<'gctx> {
}
}

if duplicates.is_empty() {
if dry_run {
self.gctx.shell().warn("aborting install due to dry run")?;
Ok(true)
} else if duplicates.is_empty() {
self.gctx.shell().status(
"Installed",
format!(
Expand Down Expand Up @@ -620,6 +635,7 @@ pub fn install(
opts: &ops::CompileOptions,
force: bool,
no_track: bool,
dry_run: bool,
) -> CargoResult<()> {
let root = resolve_root(root, gctx)?;
let dst = root.join("bin").into_path_unlocked();
Expand Down Expand Up @@ -654,7 +670,7 @@ pub fn install(
)?;
let mut installed_anything = true;
if let Some(installable_pkg) = installable_pkg {
installed_anything = installable_pkg.install_one()?;
installed_anything = installable_pkg.install_one(dry_run)?;
}
(installed_anything, false)
} else {
Expand Down Expand Up @@ -705,7 +721,7 @@ pub fn install(

let install_results: Vec<_> = pkgs_to_install
.into_iter()
.map(|(krate, installable_pkg)| (krate, installable_pkg.install_one()))
.map(|(krate, installable_pkg)| (krate, installable_pkg.install_one(dry_run)))
.collect();

for (krate, result) in install_results {
Expand Down Expand Up @@ -857,6 +873,7 @@ fn remove_orphaned_bins(
duplicates: &BTreeMap<String, Option<PackageId>>,
pkg: &Package,
dst: &Path,
dry_run: bool,
) -> CargoResult<()> {
let filter = ops::CompileFilter::new_all_targets();
let all_self_names = exe_names(pkg, &filter);
Expand Down Expand Up @@ -894,8 +911,10 @@ fn remove_orphaned_bins(
old_pkg
),
)?;
paths::remove_file(&full_path)
.with_context(|| format!("failed to remove {:?}", full_path))?;
if !dry_run {
paths::remove_file(&full_path)
.with_context(|| format!("failed to remove {:?}", full_path))?;
}
}
}
}
Expand Down
4 changes: 4 additions & 0 deletions src/doc/man/cargo-install.md
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,10 @@ Filesystem path to local crate to install from.
List all installed packages and their versions.
{{/option}}

{{#option "`-n`" "`--dry-run`" }}
(unstable) Perform all checks without installing.
{{/option}}

{{#option "`-f`" "`--force`" }}
Force overwriting existing crates or binaries. This can be used if a package
has installed a binary with the same name as another package. This is also
Expand Down
3 changes: 3 additions & 0 deletions src/doc/man/generated_txt/cargo-install.txt
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ OPTIONS
--list
List all installed packages and their versions.

-n, --dry-run
(unstable) Perform all checks without installing.

-f, --force
Force overwriting existing crates or binaries. This can be used if a
package has installed a binary with the same name as another
Expand Down
5 changes: 5 additions & 0 deletions src/doc/src/commands/cargo-install.md
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,11 @@ treated as a caret requirement like Cargo dependencies are.</dd>
<dd class="option-desc">List all installed packages and their versions.</dd>


<dt class="option-term" id="option-cargo-install--n"><a class="option-anchor" href="#option-cargo-install--n"></a><code>-n</code></dt>
<dt class="option-term" id="option-cargo-install---dry-run"><a class="option-anchor" href="#option-cargo-install---dry-run"></a><code>--dry-run</code></dt>
<dd class="option-desc">(unstable) Perform all checks without installing.</dd>


<dt class="option-term" id="option-cargo-install--f"><a class="option-anchor" href="#option-cargo-install--f"></a><code>-f</code></dt>
<dt class="option-term" id="option-cargo-install---force"><a class="option-anchor" href="#option-cargo-install---force"></a><code>--force</code></dt>
<dd class="option-desc">Force overwriting existing crates or binaries. This can be used if a package
Expand Down
6 changes: 6 additions & 0 deletions src/etc/man/cargo-install.1
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ Filesystem path to local crate to install from.
List all installed packages and their versions.
.RE
.sp
\fB\-n\fR,
\fB\-\-dry\-run\fR
.RS 4
(unstable) Perform all checks without installing.
.RE
.sp
\fB\-f\fR,
\fB\-\-force\fR
.RS 4
Expand Down
Loading

0 comments on commit d7bffc3

Please sign in to comment.