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

feat: cache get-app artifacts by commit_hash #1519

Merged
merged 12 commits into from
Jan 23, 2024
Merged

feat: cache get-app artifacts by commit_hash #1519

merged 12 commits into from
Jan 23, 2024

Conversation

18alantom
Copy link
Member

@18alantom 18alantom commented Jan 15, 2024

TLDR: Stores get-app artifacts in .cache by commit hash cache key and reuses them when applicable.

Partially Fixes: frappe/press#1242

TODO:

  • Cache output and restore cache if present.
  • Add explicit compress_artifacts flag.
  • Update frappe/esbuild/esbuild.js to account for cached get-app. feat(Bench): add using-cached flag frappe#24412
  • Complete installation from cache.
  • Ensure $BENCH/sites is being updated properly.
  • Get rid of node_modules for apps with built frontends (gameplan, insights, etc).
  • Commands to clear, and view app cache.
  • Check if it works.

Usage

Adds two new options to bench get-app:

  • --cache-key: Used to store tarred app folder in ~/.cache/bench/apps with the following format "{app}-{cache-key[:10]}.tar"
  • --compress-artifacts: Optional flag that causes artifacts to be compressed using gzip, extension will then be .tgz.

Examples:

# Get from local repo, apps/gampelan will be cached if not present
bench get-app file:///home/frappe/gameplan --cache-key 87d4aa3b10aa23a620d5a51a968ae47110c43dd1

# Get from remote frappe repo, apps/erpnext will be compressed and cached if not present
bench get-app erpnext --cache-key 7bcea6099da1a41ad530bdd85ff337f627039b80 --compress-artifacts

For either of the above if app is present in cache it will be fetched from there, else it will be stored in cache. While installing, cache is checked for both compressed and non compressed tar files.

Utility Commands

bench app-cache Screenshot 2024-01-19 at 19 10 18
bench app-cache --remove-app APP_NAME Screenshot 2024-01-19 at 19 10 40
bench app-cache --remove-key CACHE_KEY Screenshot 2024-01-19 at 19 11 23

Why?

When running get-app bench spends significant amount of time in git clone, yarn install, pip install, building frontend dependencies. If it's running get-app on the same code (same app, same repo, same commit hash), get-app artifacts from the previous run can be reused.

How?

After get-app has completed for some $APP the relevant folders that change are:

  • $BENCH/apps/$APP: cloned source files, and built frontend files and dependencies.
  • $BENCH/env: python dependencies for the get-app'd $APP.
  • $BENCH/sites: link to the assets/$APP folder, update assets.json, apps.json, apps.txt.

Caching the contents of $BENCH/apps/$APP covers repo cloning, yarn install and yarn build. That's what this PR facilitates.

Reference

File system changes after running get-app for frappe/hrms.

Screenshot 2024-01-15 at 11 49 18

Regarding $BENCH/env

Pip install does take a considerable amount of time but since it isn't contained in the app folder it won't be included for now. Future updates might have python dependencies stored inside the app folder. Out of scope for now.

@ankush
Copy link
Member

ankush commented Jan 17, 2024

Get rid of node_modules for apps with built frontends (gameplan, insights, etc).

This will not allow rebuild btw (we allow doing it from FC dashboard 😬 )

message += " (compressed)"
click.secho(message)

os.chdir(app_path.parent)
Copy link
Member

@ankush ankush Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If possible avoid changing directories. This has caused problems in past as bench assumes a certain directory to be cwd always.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it's required cause of how tar functions, it stores the entire path and untarring requires moving the file out of nested directories. I've wrapped tar in a try finally so cwd should be restored irrespective: 30f301e

@18alantom
Copy link
Member Author

Get rid of node_modules for apps with built frontends (gameplan, insights, etc).

This will not allow rebuild btw (we allow doing it from FC dashboard 😬 )

Ah, I didn't know we allowed rebuilds. I suppose we can trigger yarn install if node_modules not found. It will increase the first rebuild time, but seems like a reasonable tradeoff. What do you think?

@18alantom 18alantom requested a review from ankush January 19, 2024 14:21
@18alantom
Copy link
Member Author

18alantom commented Jan 23, 2024

Note: ignoring SonarCloud Quality Gate

It's failing due to potential tar bombs. It's failed in two places when creating the archive and when extracting it.

When creating an archive: check is not necessary here cause the archive file is newly created and will be empty. Else that stack of code would not have run.

When extracting from an archive: archive being extracted from is created by bench, and not user submitted, well the app repo could be, but in the given context it seems highly unlikely.

(that being said I might add an extractall filter in a subsequent PR, it is not going to appease SonarCloud but will address the issue)

Edit: filter added in #1523

@18alantom 18alantom marked this pull request as ready for review January 23, 2024 09:36
Copy link

sonarcloud bot commented Jan 23, 2024

Quality Gate Failed Quality Gate failed

Failed conditions

2 Security Hotspots

See analysis details on SonarCloud

@18alantom 18alantom merged commit 36c3cf4 into develop Jan 23, 2024
13 of 14 checks passed
@ankush ankush deleted the get-app-cache branch January 24, 2024 07:04
Copy link

🎉 This PR is included in version 5.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

perf(build): Better cache pip and yarn packages
2 participants