feat(clean): `deno clean --except <paths>`, remove all cache data except what's needed to run `paths` #28424

nathanwhit · 2025-03-07T18:07:42Z

TODO:

Tests
Make some changes to deno_cache_dir so we can get the paths for the local http cache
Right now this leaves the node modules setup cache in an incorrect state (removes the symlinks, but doesn't update the setup cache)
~~Handle code cache and other sqlite caches?~~

Copilot

Pull Request Overview

This PR adds support for an entrypoint-based clean subcommand in Deno by extending the clean functionality to accept new clean flags. Key changes include:

Modifying the Clean subcommand in cli/main.rs to accept a CleanFlags argument and await the clean operation.
Introducing a new CleanFlags struct and updating the clean_parse function in cli/args/flags.rs to extract flags for the clean command.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
cli/main.rs	Updates the Clean subcommand to forward clean_flags and await the clean function.
cli/args/flags.rs	Adds the CleanFlags struct and updates the clean_parse function to extract and handle new flags.

This backs out commit eedebb9.

This backs out commit 903fe38.

This backs out commit c6e20b7.

cli/args/flags.rs

cli/tools/clean.rs

bartlomieju · 2025-04-29T20:31:13Z

cli/tools/clean.rs

+
+  let npm_cache = factory.npm_cache()?;
+  let snap = npm_resolver.as_managed().unwrap().resolution().snapshot();
+  // TODO(nathanwhit): remove once we don't need packuments for creating the snapshot from lockfile


Isn't this the case with lockfile v5?

yes, but we don't know if it's actually lockfile v5. they could still be using v4

we could maybe add an API to deno_lockfile to get the version of the original lockfile text, but there isn't one atm

Ah, good point 👍 let's keep it for now

Oh, would be sweet to add this. I think it would help netlify too.

bartlomieju · 2025-04-29T20:32:40Z

cli/tools/clean.rs

+    .map(deno_resolver::npm::get_package_folder_id_folder_name)
+    .collect::<HashSet<_>>();
+
+  // TODO(nathanwhit): this probably shouldn't reach directly into this code


I'm not sure I understand what's the problem here - is this touching some internal implementation details?

sort of. it's just everywhere else in the code we expose more generic methods on e.g. NpmInstaller. This just uses some of the code from the local installer directly. It's not incorrect, but it feels sort of wrong to me.

We could expose it as a public API down the road?

yeah that's pretty much what the todo is, just to make/use a more public api

dsherret · 2025-04-29T21:02:52Z

cli/npm/installer/mod.rs

@@ -12,6 +12,7 @@ use deno_npm::NpmSystemInfo;
 use deno_resolver::npm::managed::NpmResolutionCell;
 use deno_runtime::colors;
 use deno_semver::package::PackageReq;
+pub(crate) use local::SetupCache;


Nitpick because this is the cli crate:

Suggested change

pub(crate) use local::SetupCache;

pub use local::SetupCache;

dsherret · 2025-04-29T21:11:21Z

cli/tools/clean.rs

+#[derive(Clone, Copy, PartialEq, Eq, Debug)]
+enum Found {
+  Match,
+  Prefix,
+}


Nitpick: Maybe move this about PathTree so that the PathTrie struct w/ impl is beside each other?

dsherret · 2025-04-29T21:13:42Z

cli/tools/clean.rs

+    .build_graph_with_npm_resolution(
+      graph,
+      CreateGraphOptions {
+        // loader: Some(&mut NoLoader),


dsherret · 2025-04-29T21:16:04Z

cli/tools/clean.rs

+
+  let npm_cache = factory.npm_cache()?;
+  let snap = npm_resolver.as_managed().unwrap().resolution().snapshot();
+  // TODO(nathanwhit): remove once we don't need packuments for creating the snapshot from lockfile


Oh, would be sweet to add this. I think it would help netlify too.

dsherret · 2025-04-29T21:17:01Z

cli/tools/clean.rs

+      continue;
+    }
+    if !entry.path().starts_with(base) {
+      panic!("VERY BAD");


Better message? 😄

dsherret · 2025-04-29T21:18:40Z

cli/tools/clean.rs

+      if dry_run {
+        #[allow(clippy::print_stderr)]
+        {
+          eprintln!("would remove dir: {}", entry.path().display());


Should we use a tree view here similar to deno compile/info's output? This prefix text for each entry seems a little repetitive btw. Maybe we should just list the paths?

I'll just list the paths for now

dsherret · 2025-04-29T21:21:12Z

cli/tools/clean.rs

+        if meta.is_symlink() {
+          std::fs::remove_dir(path).with_context(|| {
+            format!("Failed to remove symlink: {}", path.display())
+          })?;
+          return Ok(());
+        }


Does remove_dir work for file symlinks on windows?

nope, only directory symlinks AFAICT. that's why it tries remove_file, and falls back to remove_dir if that failed

dsherret · 2025-04-29T21:22:28Z

cli/tools/clean.rs

+  dry_run: bool,
+) -> Result<(), AnyError> {
+  if !dir.ends_with("node_modules") || !dir.is_dir() {
+    bail!("not a node_modules directory");


Include the path in the error message?

dsherret · 2025-04-29T21:23:46Z

cli/tools/clean.rs

+  if !base.exists() {
+    return Ok(());
+  }


Not like it will ever happen, but perhaps we should handle this in the std::fs::read_dir(&base)? error case and also handle when ErrorKind::NotADirectory.

dsherret · 2025-04-29T21:25:10Z

cli/tools/clean.rs

+    } else if dry_run {
+      #[allow(clippy::print_stderr)]
+      {
+        eprintln!(
+          "would remove dir from node modules: {}",
+          entry.path().display()
+        );
+      }
+    } else {
+      rm_rf(state, &entry.path())?;
+    }


Could use strategy pattern here in the future (reporting strategy and remove strategy).

dsherret

LGTM!

nathanwhit added 9 commits February 27, 2025 16:38

wip

f22dad2

Merge main into clean-entrypoint-take2

31a5404

getting there

9b3701d

cleanup

238a956

cleanup, simplify, print results

5c6ba37

more cleanup

a41703e

cleanup

6b03b0b

test path trie, accept vendor flag

470eae6

refactor for vendor dir

82ebcd3

bartlomieju added this to the 2.3.0 milestone Apr 22, 2025

nathanwhit requested review from dsherret and Copilot April 22, 2025 16:07

bartlomieju assigned dsherret and bartlomieju Apr 22, 2025

nathanwhit requested a review from bartlomieju April 22, 2025 16:07

Copilot AI reviewed Apr 22, 2025

View reviewed changes

nathanwhit added 6 commits April 23, 2025 18:49

Merge upstream/main into clean-entrypoint-take2

f9636d1

update deno_cache_dir

b2670b8

wire up removing local cache

1ee2781

start adding tests

060c3a0

more tests

360c864

Merge main into clean-entrypoint-take2

c93fbc2

nathanwhit marked this pull request as ready for review April 24, 2025 21:41

nathanwhit added 7 commits April 24, 2025 15:04

unpatch

634b94e

appease clippy

c9ad9ca

fix test on windows

976afaf

fix test

bc10666

fix text again

e60f6a1

fix for real

a376141

fix again

64b1d56

debugging ci

903fe38

nathanwhit force-pushed the clean-entrypoint-take2 branch from 4cabc70 to 903fe38 Compare April 28, 2025 23:12

nathanwhit added 2 commits April 28, 2025 16:28

normalize paths

4e0480f

debug

eedebb9

nathanwhit force-pushed the clean-entrypoint-take2 branch from 2af5b12 to eedebb9 Compare April 29, 2025 00:19

nathanwhit added 8 commits April 28, 2025 17:23

canonicalize root path

4ce5477

Back out "debug"

206869e

This backs out commit eedebb9.

Back out "debugging ci"

7567d5d

This backs out commit 903fe38.

use sys traits canonicalize

b6d995b

appease clippy

a3fd5c0

canonicalize deno dir path

c6e20b7

Back out "canonicalize deno dir path"

f5c97e2

This backs out commit c6e20b7.

rewrite base paths

6ffac29

nathanwhit force-pushed the clean-entrypoint-take2 branch from 697096e to 2dd71a8 Compare April 29, 2025 15:45

Merge main into clean-entrypoint-take2

7d66ed3

nathanwhit force-pushed the clean-entrypoint-take2 branch from 2dd71a8 to 7d66ed3 Compare April 29, 2025 15:53

dsherret reviewed Apr 29, 2025

View reviewed changes

cli/args/flags.rs Show resolved Hide resolved

bartlomieju reviewed Apr 29, 2025

View reviewed changes

nathanwhit added 2 commits April 29, 2025 14:03

fix test

c8b50fe

address comments

696f457

nathanwhit force-pushed the clean-entrypoint-take2 branch from 5c8e5d9 to 696f457 Compare April 29, 2025 21:03

dsherret reviewed Apr 29, 2025

View reviewed changes

nathanwhit added 2 commits April 29, 2025 14:57

appease clippy

8b23905

address comments

6f5433f

nathanwhit force-pushed the clean-entrypoint-take2 branch from 8c030e3 to 6f5433f Compare April 29, 2025 21:57

Merge main into clean-entrypoint-take2

77167a6

dsherret approved these changes Apr 29, 2025

View reviewed changes

nathanwhit enabled auto-merge (squash) April 29, 2025 22:09

nathanwhit merged commit 6d00354 into denoland:main Apr 29, 2025
18 checks passed

nathanwhit deleted the clean-entrypoint-take2 branch April 29, 2025 22:50

feat(clean): deno clean --except <paths>, remove all cache data except what's needed to run paths #28424

feat(clean): deno clean --except <paths>, remove all cache data except what's needed to run paths #28424

Uh oh!

Conversation

nathanwhit commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanwhit Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nathanwhit Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dsherret left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

feat(clean): `deno clean --except <paths>`, remove all cache data except what's needed to run `paths` #28424

feat(clean): `deno clean --except <paths>`, remove all cache data except what's needed to run `paths` #28424

nathanwhit commented Mar 7, 2025 •

edited

Loading

nathanwhit Apr 29, 2025 •

edited

Loading

nathanwhit Apr 29, 2025 •

edited

Loading