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.

Sign up for GitHub

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

Jump to bottom

enhance(scripts/generate-bootstraps.sh): add caching support #11491

Draft

truboxl wants to merge 1 commit into termux:master from truboxl:generate-bootstraps.sh-cache

Contributor

truboxl commented Aug 6, 2022

Since build-bootstraps.sh is out of commission and pending a rewrite (#10462 (comment)), I decided to just improve generate-bootstraps.sh which already does not include extra packages when generating bootstraps to begin with.

With the optional caching turned on, it gained a tiny bit of speed up.
Time lost most of which is spend on downloading the same *_all.deb files again thrice.
But subsequent runs will show massive speed up since files are stored and cached.

Without caching all arch (default)
real	20m57.026s
user	0m0.515s
sys	0m0.870s
With caching all arch (1st run)
real	19m14.773s
user	0m0.508s
sys	0m0.972s
With caching all arch (after 1st run)
real	0m54.656s
user	0m0.084s
sys	0m0.100s

With caching also allows building for custom arches.
For my arm vfpv3-d16 project, I can imagine triggering multiple parallel CI running
./scripts/run-docker.sh ./build-package.sh -a arm one-bootstrap-package-per-CI
and then pass all deb files to
./scripts/run-docker.sh ./scripts/generate-bootstraps.sh -c --architectures arm
for processing and not rely on build-bootstraps.sh anymore...

Note that file lists are not cached.

truboxl requested a review from Grimler91 as a code owner

August 6, 2022 08:55

truboxl requested a review from agnostic-apollo

August 6, 2022 08:56

truboxl force-pushed the generate-bootstraps.sh-cache branch 2 times, most recently from 5297acc to 60a4818 Compare

August 16, 2022 16:22

truboxl mentioned this pull request

[Bug]: Building apt fails with configure: exit code 77 #11713

Closed

truboxl force-pushed the generate-bootstraps.sh-cache branch from 60a4818 to 50e1526 Compare

September 9, 2022 20:48

truboxl mentioned this pull request

[Bug]: build-package.sh: cannot compile subpackages #10462

Open

truboxl marked this pull request as draft

October 17, 2022 22:54

truboxl force-pushed the generate-bootstraps.sh-cache branch 3 times, most recently from a897335 to d34339b Compare

October 22, 2022 15:14


          enhance(scripts/generate-bootstraps.sh): add caching support

5367b1d

* format the script too

truboxl force-pushed the generate-bootstraps.sh-cache branch from d34339b to 5367b1d Compare

October 22, 2022 15:19

truboxl marked this pull request as ready for review

October 24, 2022 09:33

Contributor Author

truboxl commented Oct 24, 2022

Will merge this soon if no objection
Caching is disabled by default. The caching feature is actively being tested over at my repo https://github.com/truboxl/termux-packages-altarch

If we do enable caching in generate bootstrap ci, we should be able to cut down fetching the same *_all.deb 3 times. From my testing, they are:

Package: termux-am
Download-Size: 17.1 kB
Package: ca-certificates
Download-Size: 234 kB
Package: termux-keyring
Download-Size: 39.2 kB
Package: termux-licenses
Download-Size: 52.2 kB
Package: termux-tools
Download-Size: 27.9 kB

which should save 371.4kB x 3 = 1111.2kB bandwidth if enabled, probably too small to notice
Might want to explore more methods to cut down bandwidth to the server in the future

Member

agnostic-apollo commented Oct 24, 2022

After months of commits, are you sure you are done? ;)

It's a good idea to cache for people who have space, so thanks.

But there are changes required.

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

+              # Optional caching of downloaded packages in output directory
+              # Apt only for now
+              TERMUX_PACKAGE_DOWNLOAD_CACHE=false
+              TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR="$PWD/output"

Member

agnostic-apollo Oct 24, 2022

This should be named cache instead of output.

The TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR does not include TERMUX_APP_PACKAGE, which will result in mixing of packages with different prefixes if cache dir is not cleaned manually if repo url is changed.

Moreover, the generate-bootstrap.sh script is not checking if TERMUX_REPO_PACKAGE equals TERMUX_APP_PACKAGE. It would be better to replace REPO_BASE_URLS and -r format to <package_name>:<repo_url> so that caching is correctly done as you want.

termux-packages/scripts/properties.sh

Lines 35 to 38 in 8e8b1a4

    
           # Package name for the packages hosted on the repo. 
        
           # This must only equal TERMUX_APP_PACKAGE if using custom repo that 
        
           # has packages that were built with same package name. 
        
           TERMUX_REPO_PACKAGE="com.termux"

Member

agnostic-apollo Oct 24, 2022

Also consider adding a --clean-cache-dir option which clears the cache dir for TERMUX_APP_PACKAGE and redownloads the packages.

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

		package_url="$REPO_BASE_URL/$(echo "${PACKAGE_METADATA[${package_name}]}" \| grep -i "^Filename:" \| awk '{ print $2 }')"
		package_file=$(echo "$package_url" \| sed -e 's\|\(.\)\/\(.\).deb$\|\2.deb\|')

Member

agnostic-apollo Oct 24, 2022

Using basename should work fine.

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

@@ @@ -295,6 +322,11 @@ show_usage() { @@
               	echo "                             Multiple packages should be passed as"
               	echo "                             comma-separated list."
               	echo
+              	echo " -c, --cache [DIR]           Enable caching. Downloaded package files will be"

Member

agnostic-apollo Oct 24, 2022

Ideally separate parameters should be used for enabling cache and passing custom cache dir in case other options need to be added in future, like -c/--cache and --cache-dir.

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

@@ @@ -382,23 +421,38 @@ while (($# > 0)); do @@
               done
               if [[ "$TERMUX_PACKAGE_MANAGER" == *" "* ]] || [[ " ${TERMUX_PACKAGE_MANAGERS[*]} " != *" $TERMUX_PACKAGE_MANAGER "* ]]; then
-              	echo "[!] Invalid package manager '$TERMUX_PACKAGE_MANAGER'" 1>&2
-              	echo "Supported package managers: '${TERMUX_PACKAGE_MANAGERS[*]}'" 1>&2
+              	echo "[!] Invalid package manager '$TERMUX_PACKAGE_MANAGER'" >&2

Member

agnostic-apollo Oct 24, 2022

No specific need to remove 1 here and other places. Current way makes it explicitly clear what is being redirected to what. Will also require changes in build-bootstrap.sh if you do.

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

    
              for package_arch in "${TERMUX_ARCHITECTURES[@]}"; do

              	BOOTSTRAP_ROOTFS="$BOOTSTRAP_TMPDIR/rootfs-${package_arch}"

              	BOOTSTRAP_PKGDIR="$BOOTSTRAP_TMPDIR/packages-${package_arch}"

              	# Create initial directories for $TERMUX_PREFIX

              	if ! ${BOOTSTRAP_ANDROID10_COMPATIBLE}; then

              		if [ ${TERMUX_PACKAGE_MANAGER} = "apt" ]; then

              		if [ "$TERMUX_PACKAGE_MANAGER" = apt ]; then

Member

agnostic-apollo Oct 24, 2022

Don't remove double quotes. apt is a string and not a keyword. It's convention to always use double quotes around variables and strings.

https://google.github.io/styleguide/shellguide.html#testing-strings

agnostic-apollo reviewed

View reviewed changes

scripts/generate-bootstraps.sh

@@ @@ -144,10 +150,31 @@ pull_package() { @@
               			unset dep
               		fi
+              		if [ "$TERMUX_PACKAGE_DOWNLOAD_CACHE" = true ]; then

Member

agnostic-apollo Oct 24, 2022

The rm -f "$package_tmpdir/package.deb" should be moved before this. Shouldn't attempt to use any existing deb file at location, unless a valid cached one is linked to it.

truboxl marked this pull request as draft

October 25, 2022 04:39

stale bot commented Dec 9, 2022

This issue/PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale bot added the wontfix label

agnostic-apollo removed the wontfix label

Grimler91 added the not stale label

xtkoba removed the not stale label

Member

twaik commented Oct 19, 2024

Is this still relevant/in progress? There are unresolved reviews and the branch is out of date.

ReeaoX mentioned this pull request

[Bug]: my custom config of Polybar. Reverted into the example config of Polybar. After receive upgrading Polybar (Pacman). #22276

Closed

31 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet