feat: Clean cache command#1394
Conversation
d3xter666
force-pushed
the
feat-clean-cache
branch
2 times, most recently
from
83f2262 to
d52c4ad
Compare
RandomByte
force-pushed
the
feat/incremental-build-4
branch
from
7c59782 to
444977d
Compare
RandomByte
force-pushed
the
feat/incremental-build-4
branch
from
c2dc7b8 to
1041695
Compare
d3xter666
force-pushed
the
feat-clean-cache
branch
2 times, most recently
from
dc31834 to
f5def12
Compare
RandomByte
force-pushed
the
feat/incremental-build-4
branch
from
1041695 to
66296d5
Compare
d3xter666
force-pushed
the
feat-clean-cache
branch
2 times, most recently
from
77f7320 to
aa280da
Compare
d3xter666
force-pushed
the
feat-clean-cache
branch
from
34fd55f to
7473bd4
Compare
d3xter666
force-pushed
the
feat-clean-cache
branch
2 times, most recently
from
7b17cc0 to
569ff71
Compare
Provide common interface for cache cleanup, but distribute the real cleanup among the respective destinations
d3xter666
force-pushed
the
feat-clean-cache
branch
from
569ff71 to
36fd376
Compare
| * @returns {Promise<boolean>} True if user confirmed | ||
| */ | ||
| async function confirm(question) { | ||
| const rl = readline.createInterface({ |
There was a problem hiding this comment.
In the server we use a library for such questions:
cli/packages/server/lib/sslUtil.js
Line 74 in 2655ce8
Not sure which way is better, but ideally we use the same approach in both places
| .command("clean", "Remove all cached UI5 data", { | ||
| handler: handleCache, | ||
| builder: function(yargs) { | ||
| return yargs.option("interactive", { |
There was a problem hiding this comment.
From my experience, --yes or --force is more commonly used for this kind of option. E.g. apt get upgrade --yes or docker system prune -f
| return { | ||
| path: `buildCache/${CACHE_VERSION}`, | ||
| size, | ||
| type: "database" |
There was a problem hiding this comment.
It seems like the type attribute is never used (besides in tests). Do we need it?
| @@ -0,0 +1,80 @@ | |||
| import path from "node:path"; | |||
There was a problem hiding this comment.
It seems odd that this file is placed next to the other files responsible for managing the framework packages but doesn't use any of them. I would expect better integration here, e.g. no hardcoded assumptions like the framework/ directory name as well as checks for existing lockfiles to prevent deleting files while a download is running
| try { | ||
| const size = await getDirectorySize(frameworkDir); | ||
| if (size > 0) { | ||
| await fs.rm(frameworkDir, {recursive: true, force: true}); |
There was a problem hiding this comment.
Do we actually need to specify force?
| path: "framework", | ||
| type: "framework", |
There was a problem hiding this comment.
Why is both path and type different here from getCacheInfo? (as als mentioned in CacheManager: do we actually need type?)
| * @param {string} dirPath Absolute path to directory | ||
| * @returns {Promise<number>} Total size in bytes | ||
| */ | ||
| async function getDirectorySize(dirPath) { |
There was a problem hiding this comment.
On my system this results in a different number than du:
~/.ui5
❯ du -sh framework
37G framework
❯ ui5 cache clean
The following items from cache will be removed:
• framework/ (31.9 GB)
• buildCache/v0_7 (70.7 MB)
Total: 32.0 GBIn my case, it also takes over a minute to calculate the size, which makes me wonder whether we should really do that or rather list the number of artifacts. Something like "this will delete 500 packages across 40 versions of UI5" would be much faster to calculate - not sure yet.
| export async function cleanCache(ui5DataDir) { | ||
| const frameworkDir = path.join(ui5DataDir, "framework"); | ||
| try { | ||
| const size = await getDirectorySize(frameworkDir); |
There was a problem hiding this comment.
Since it takes a long time to calculate the size of a large cache, could we skip calculating it again during the cleaning?
| const dbDir = path.join(buildCacheDir, CACHE_VERSION); | ||
|
|
||
| try { | ||
| const storage = new BuildCacheStorage(dbDir); |
There was a problem hiding this comment.
Could we skip this if no database exists yet? Otherwise this would end up creating a new db file
3 participants
The command completely cleans the cache by removing the cache files as well as cleaning up the SQLite records.
It does not wipe out the SQLite DB file(s)
JIRA: CPOUI5FOUNDATION-891