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.

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

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
+64 −10
Draft

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 64 additions & 10 deletions scripts/generate-bootstraps.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,11 @@ trap 'rm -rf $BOOTSTRAP_TMPDIR' EXIT
# and <10.
BOOTSTRAP_ANDROID10_COMPATIBLE=false

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

Choose a reason for hiding this comment

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

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"

Copy link
Member

Choose a reason for hiding this comment

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

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


# By default, bootstrap archives will be built for all architectures
# supported by Termux application.
# Override with option '--architectures'.
Expand Down Expand Up @@ -118,9 +123,10 @@ pull_package() {
local package_tmpdir="${BOOTSTRAP_PKGDIR}/${package_name}"
mkdir -p "$package_tmpdir"

if [ ${TERMUX_PACKAGE_MANAGER} = "apt" ]; then
local package_url
if [ "$TERMUX_PACKAGE_MANAGER" = apt ]; then
local package_url package_file
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|')
Copy link
Member

Choose a reason for hiding this comment

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

Using basename should work fine.

if [ "${package_url}" = "$REPO_BASE_URL" ] || [ "${package_url}" = "${REPO_BASE_URL}/" ]; then
echo "[!] Failed to determine URL for package '$package_name'."
exit 1
Expand All @@ -144,10 +150,31 @@ pull_package() {
unset dep
fi

if [ "$TERMUX_PACKAGE_DOWNLOAD_CACHE" = true ]; then
Copy link
Member

Choose a reason for hiding this comment

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

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.

# since each generate-bootstraps.sh run will have different package_tmpdir
# we dont have to check existence of both files and even so prioritise the cached copy
# also use symbolic link to save space and I/O
echo "[*] CACHE: Checking for cached copy of '$package_file' for package '$package_name'..."
if [ -e "$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR/$package_file" ]; then
echo "[*] CACHE: Found cached copy '$package_file'! Symlinking..."
rm -f "$package_tmpdir/package.deb"
ln -fs "$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR/$package_file" "$package_tmpdir/package.deb"
else
echo "[*] CACHE: Cached copy of '$package_file' not found. Skipping..."
fi
fi

if [ ! -e "$package_tmpdir/package.deb" ]; then
echo "[*] Downloading '$package_name'..."
curl --fail --location --output "$package_tmpdir/package.deb" "$package_url"

if [ "$TERMUX_PACKAGE_DOWNLOAD_CACHE" = true ]; then
echo "[*] CACHE: Storing '$package_name' package.deb at '$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR/$package_file'..."
cp -f "$package_tmpdir/package.deb" "$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR/$package_file"
fi
fi

if [ -e "$package_tmpdir/package.deb" ]; then
echo "[*] Extracting '$package_name'..."
(cd "$package_tmpdir"
ar x package.deb
Expand Down Expand Up @@ -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"
Copy link
Member

Choose a reason for hiding this comment

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

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.

echo " stored at output directory unless specified another"
echo " path. The files can be reused at later runs."
echo " This option is disabled by default and for apt only."
echo
echo " --pm MANAGER Set up a package manager in bootstrap."
echo " It can only be pacman or apt (the default is apt)."
echo
Expand All @@ -311,7 +343,7 @@ show_usage() {
echo "Architectures: ${TERMUX_ARCHITECTURES[*]}"
echo "Repository Base Url: ${REPO_BASE_URL}"
echo "Prefix: ${TERMUX_PREFIX}"
echo "Package manager: ${TERMUX_PACKAGE_MANAGER}"
echo "Package manager: ${TERMUX_PACKAGE_MANAGER}"
echo
}

Expand Down Expand Up @@ -343,7 +375,7 @@ while (($# > 0)); do
REPO_BASE_URL="${REPO_BASE_URLS[${TERMUX_PACKAGE_MANAGER}]}"
shift 1
else
echo "[!] Option '--pm' requires an argument." 1>&2
echo "[!] Option '--pm' requires an argument." >&2
show_usage
exit 1
fi
Expand Down Expand Up @@ -372,6 +404,13 @@ while (($# > 0)); do
exit 1
fi
;;
-c|--cache)
if [ $# -gt 1 ] && [ -n "$2" ] && [[ $2 != -* ]]; then
TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR="$2"
shift 1
fi
TERMUX_PACKAGE_DOWNLOAD_CACHE=true
;;
*)
echo "[!] Got unknown option '$1'"
show_usage
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

echo "Supported package managers: '${TERMUX_PACKAGE_MANAGERS[*]}'" >&2
exit 1
fi

if [ -z "$REPO_BASE_URL" ]; then
echo "[!] The repository base url is not set." 1>&2
echo "[!] The repository base url is not set." >&2
exit 1
fi

if [ "$TERMUX_PACKAGE_DOWNLOAD_CACHE" = true ]; then
if [ "$TERMUX_PACKAGE_MANAGER" != apt ]; then
echo "[!] Option '--cache' does not support '$TERMUX_PACKAGE_MANAGER' yet." >&2
exit 1
fi
echo "[*] CACHE: Caching enabled. Cached packages will be stored and reused."
echo "[*] CACHE: Cache directory = $TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR"
echo "[*] CACHE: Temporary directory = $BOOTSTRAP_TMPDIR"
mkdir -p "$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR"
if [ ! -d "$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR" ]; then
echo "[!] CACHE: Failed to create cache directory '$TERMUX_PACKAGE_DOWNLOAD_CACHE_DIR'." >&2
exit 1
fi
fi

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
Copy link
Member

Choose a reason for hiding this comment

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

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

mkdir -p "${BOOTSTRAP_ROOTFS}/${TERMUX_PREFIX}/etc/apt/apt.conf.d"
mkdir -p "${BOOTSTRAP_ROOTFS}/${TERMUX_PREFIX}/etc/apt/preferences.d"
mkdir -p "${BOOTSTRAP_ROOTFS}/${TERMUX_PREFIX}/var/lib/dpkg/info"
Expand All @@ -420,7 +474,7 @@ for package_arch in "${TERMUX_ARCHITECTURES[@]}"; do
# Read package metadata.
unset PACKAGE_METADATA
declare -A PACKAGE_METADATA
if [ ${TERMUX_PACKAGE_MANAGER} = "apt" ]; then
if [ "$TERMUX_PACKAGE_MANAGER" = apt ]; then
read_package_list_deb "$package_arch"
else
read_package_list_pac "$package_arch"
Expand Down Expand Up @@ -460,7 +514,7 @@ for package_arch in "${TERMUX_ARCHITECTURES[@]}"; do

# Additional.
pull_package ed
if [ ${TERMUX_PACKAGE_MANAGER} = "apt" ]; then
if [ "$TERMUX_PACKAGE_MANAGER" = apt ]; then
pull_package debianutils
fi
pull_package dos2unix
Expand Down