Infra, Build Order and Build Bootstraps Improvements #24647
Draft
+1,972
−578
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agnostic-apollo
requested review from
Grimler91,
thunder-coding and
TomJo2000
as code owners
TomJo2000
reviewed
May 7, 2025
There was a problem hiding this comment.
I'm gonna just have to stop my review here.
This is 2500LOCs of wide ranging build script changes.
I have serious stylistic concerns about the scripts/utils subdirectory.
And there are a good amount of seemingly unrelated packaging changes wrapped up in this PR.
This is practically unreviewable.
I hope you've crossed your t's and dotted your i's because I cannot check your work on this.
scripts/utils/utils.sh
Outdated
There was a problem hiding this comment.
What's the high level goal with the entire scripts/utils directory?
This seems rather clunky and doesn't really fit the style of the rest of the build scripts.
There was a problem hiding this comment.
Those scripts are from my shell libraries that will be released soon in a separate org. termux-packages will use need-only functions/files from those libraries. The final changes would use some scripts/lib/<lib_name> format for external libraries.
| for subpackage in $TERMUX_PKG_BUILDER_DIR/*.subpackage.sh $TERMUX_PKG_TMPDIR/*subpackage.sh; do | ||
| [[ -f "$subpackage" ]] || continue | ||
| for subpackage in "${TERMUX_PKG_SUBPACKAGES_LIST[@]}"; do | ||
| if [ ! -f "$subpackage" ]; then |
There was a problem hiding this comment.
Wouldn't this be a more concise version of that change?
Suggested change
| if [ ! -f "$subpackage" ]; then | |
| [[ -f "$subpackage" ]] || termux_error_exit "Failed to find subpackage build file \"$subpackage\" of package \"$TERMUX_PKG_NAME\"" |
I spent a lot of time and effort overhauling the subpackages scripts for readability.
There was a problem hiding this comment.
We need to know if deb file for package and all its subpackages actually exists in the output directory before deciding whether package should be rebuilt or not. If any deb is missing, package must be rebuilt. Currently, there are no such validations.
There was a problem hiding this comment.
That part was moved to a separate function due to duplicated code in debian and pacman.
| for subpackage in $TERMUX_PKG_BUILDER_DIR/*.subpackage.sh $TERMUX_PKG_TMPDIR/*subpackage.sh; do | ||
| [[ -f "$subpackage" ]] || continue | ||
| for subpackage in "${TERMUX_PKG_SUBPACKAGES_LIST[@]}"; do | ||
| if [ ! -f "$subpackage" ]; then |
There was a problem hiding this comment.
Same as in the Apt case.
Suggested change
| if [ ! -f "$subpackage" ]; then | |
| [[ -f "$subpackage" ]] || termux_error_exit "Failed to find subpackage build file \"$subpackage\" of package \"$TERMUX_PKG_NAME\"" |
1This PR adds draft build infrastructure changes are fixes To build bootstraps with default packages, run following. It takes about 20-25min on my laptop to build bootstrap zip of one architecture. Do not pass
For For The files being added by Diff generate and build bootstraps of single archget_packages() { unzip -p "$1" var/lib/dpkg/status | grep '^Package: ' | sed -E 's/Package: //g' | sort; }
get_file_list() { unzip -Z -1 "$1" | sort; }
get_file_sizes() { unzip -vqq "$1" | awk '{$2=""; $3=""; $4=""; $5=""; $6=""; $7=""; $1 = sprintf("%-10s", $1); print}' | sort -k2; }
BOOTSTRAP_A=output/generate/bootstrap-aarch64.zip BOOTSTRAP_B=output/build/bootstrap-aarch64.zip
diff -y --suppress-common-lines --width=160 -t <(get_file_sizes "$BOOTSTRAP_A") <(get_file_sizes "$BOOTSTRAP_B") > diff.txt
(diff -u <(get_packages "$BOOTSTRAP_A") <(get_packages "$BOOTSTRAP_B")) > diff.txt
(diff -u <(get_file_list "$BOOTSTRAP_A") <(get_file_list "$BOOTSTRAP_B")) > diff.txtDiff generate and build bootstraps of multiple arches and generate single markdown reportget_packages() { unzip -p "$1" var/lib/dpkg/status | grep '^Package: ' | sed -E 's/Package: //g' | sort; }
get_file_list() { unzip -Z -1 "$1" | sort; }
get_file_sizes() { unzip -vqq "$1" | awk '{$2=""; $3=""; $4=""; $5=""; $6=""; $7=""; $1 = sprintf("%-10s", $1); print}' | sort -k2; }
BOOTSTRAPS_A_DIR="output/generate"
BOOTSTRAPS_B_DIR="output/build"
DIFF_FILE="diff.md"
printf "%s\n\n" "# Bootstraps Comparison" > "$DIFF_FILE"
for arch in "aarch64" "arm" "x86_64" "i686"; do
BOOTSTRAP_A="$BOOTSTRAPS_A_DIR/bootstrap-${arch}.zip"
BOOTSTRAP_B="$BOOTSTRAPS_B_DIR/bootstrap-${arch}.zip"
if [[ -f "$BOOTSTRAP_A" ]] && [[ -f "$BOOTSTRAP_B" ]]; then
printf "%s\n\n" "## \`${BOOTSTRAP_A}\` vs \`${BOOTSTRAP_B}\`" >> "$DIFF_FILE"
printf "%s\n\n" "## Packages Comparison" >> "$DIFF_FILE"
printf "%s\n" '```diff' >> "$DIFF_FILE"
(diff --label="$BOOTSTRAP_A" --label="$BOOTSTRAP_B" -u <(get_packages "$BOOTSTRAP_A") <(get_packages "$BOOTSTRAP_B")) >> "$DIFF_FILE"
printf "%s\n" '```' >> "$DIFF_FILE"
printf "\n\n\n%s\n\n" "## File List Comparison" >> "$DIFF_FILE"
printf "%s\n" '```diff' >> "$DIFF_FILE"
(diff --label="$BOOTSTRAP_A" --label="$BOOTSTRAP_B" -u <(get_file_list "$BOOTSTRAP_A") <(get_file_list "$BOOTSTRAP_B")) >> "$DIFF_FILE"
printf "%s\n" '```' >> "$DIFF_FILE"
printf "\n\n\n%s\n\n" "## File Size Comparison" >> "$DIFF_FILE"
printf "%s\n" '```diff' >> "$DIFF_FILE"
diff --label="$BOOTSTRAP_A" --label="$BOOTSTRAP_B" -y --suppress-common-lines --width=160 -t <(get_file_sizes "$BOOTSTRAP_A") <(get_file_sizes "$BOOTSTRAP_B") >> "$DIFF_FILE"
printf "%s\n" '```' >> "$DIFF_FILE"
printf "\n---\n\n\n\n\n" >> "$DIFF_FILE"
fi
done |
2This PR changes the target build order algorithm to use topological sorting to generate the order. This is necessary because if The |
3The The I have temporarily removed the The |
Build order cycle before `python-tkinter` removal with `-I`
|
Build order cycle before `python-tkinter` removal without `-I`
|
Build order after `python-tkinter` removal
|
agnostic-apollo
referenced
this pull request
in agnostic-apollo/termux-packages
May 7, 2025
twaik
reviewed
May 7, 2025
| declare -a PACKAGES_LIST=() | ||
|
|
||
| for i in "${PACKAGES_ARGS_LIST[@]}"; do | ||
| if [[ -z "${i:-}" ]]; then |
There was a problem hiding this comment.
AFAIK it is not needed to do :- inside [[, the test will not cause bailing because of set -u. Same applies to all places you used this.
Also AFAIK this causes some speed hit.
|
|
||
| build_bootstrap_killtree() { | ||
|
|
||
| local signal="$1"; local pid="$2"; local cpid |
There was a problem hiding this comment.
It can be collapsed to a single local expression if you do not mind.
| if [[ " ${TERMUX_DEFAULT_ARCHITECTURES[*]} " != *" $TERMUX_ARCH "* ]]; then | ||
| echo "Unsupported architecture '$TERMUX_ARCH' for in architectures list: '${TERMUX_ARCHITECTURES[*]}'" 1>&2 | ||
| echo "Supported architectures: '${TERMUX_DEFAULT_ARCHITECTURES[*]}'" 1>&2 | ||
| if [[ "$TERMUX_PACKAGE_MANAGER" == *" "* ]] || [[ " ${TERMUX_SUPPORTED_PACKAGE_MANAGERS[*]} " != *" $TERMUX_PACKAGE_MANAGER "* ]]; then |
There was a problem hiding this comment.
Can be collapsed to a single [[ ... || ... ]] expression.
Comment on lines
+574
to
587
| if [[ $NO_CLEAN_BEFORE_BUILD != "true" ]]; then | ||
| # The "-f" flag must never be passed to `build-package.sh` | ||
| # with `BUILD_PACKAGE_OPTIONS` as otherwise if a package | ||
| # in `PACKAGES` list first gets built as a dependency of | ||
| # another package, and then its build is manually | ||
| # started as part of bootstrap packages, then it will | ||
| # get built again, which may also fail or create an | ||
| # inconsistent state for certain packages as prefix | ||
| # wouldn't have been wiped. So all packages are manually | ||
| # cleaned before building bootstrap. | ||
|
|
||
| # Remove `$HOME/.termux-build` and termux prefix. | ||
| "$TERMUX_SCRIPTDIR/clean.sh" | ||
| fi |
There was a problem hiding this comment.
Probably can be
Suggested change
| if [[ $NO_CLEAN_BEFORE_BUILD != "true" ]]; then | |
| # The "-f" flag must never be passed to `build-package.sh` | |
| # with `BUILD_PACKAGE_OPTIONS` as otherwise if a package | |
| # in `PACKAGES` list first gets built as a dependency of | |
| # another package, and then its build is manually | |
| # started as part of bootstrap packages, then it will | |
| # get built again, which may also fail or create an | |
| # inconsistent state for certain packages as prefix | |
| # wouldn't have been wiped. So all packages are manually | |
| # cleaned before building bootstrap. | |
| # Remove `$HOME/.termux-build` and termux prefix. | |
| "$TERMUX_SCRIPTDIR/clean.sh" | |
| fi | |
| # The "-f" flag must never be passed to `build-package.sh` | |
| # with `BUILD_PACKAGE_OPTIONS` as otherwise if a package | |
| # in `PACKAGES` list first gets built as a dependency of | |
| # another package, and then its build is manually | |
| # started as part of bootstrap packages, then it will | |
| # get built again, which may also fail or create an | |
| # inconsistent state for certain packages as prefix | |
| # wouldn't have been wiped. So all packages are manually | |
| # cleaned before building bootstrap. | |
| # Remove `$HOME/.termux-build` and termux prefix. | |
| [[ $NO_CLEAN_BEFORE_BUILD != "true" ]] && "$TERMUX_SCRIPTDIR/clean.sh" |
There was a problem hiding this comment.
If some other code would need to be added with no clean enabled, then would have to readd the if condition, make changes to indent, which would also mess with git line history. Additionally, my newer build-bootstraps.sh script does not use set -e and can't use above directly with || return $?.
|
|
||
| # Case statements are slow for long strings | ||
| # 1000 is arbitrarily selected and needs testing | ||
| if [ "${#1}" -lt 1000 ]; then |
There was a problem hiding this comment.
Suggested change
| if [ "${#1}" -lt 1000 ]; then | |
| if (( ${#1} < 1000 )); then |
| # start `^` and end `$` anchors | ||
| # We manually add everything to pattern space and print only | ||
| # if it matches the regex | ||
| [ -n "$(data__printf_string "${1}x" | sed -r -n -e '$!{:a;N;$!ba;}; /^[a-zA-Z_][a-zA-Z0-9_]+$/p')" ] |
There was a problem hiding this comment.
In the case if this function is intensively used probably you want to use only bash builtins here to avoid performance hit.
There was a problem hiding this comment.
Read the if condition above, its only used if a variable name length exceeds 1000 characters, that normally does not happen, other than some of my crazy projects stuff where in rare conditions it can exceed depending on user input.
Comment on lines
+108
to
+117
| if [ -z "$curr_value" ]; then | ||
| if [ -n "${2-}" ]; then | ||
| logger__log_errors "The $1 is not set while running \"${2-}\"${3-}" | ||
| else | ||
| logger__log_errors "The $1 is not set" | ||
| fi | ||
| return 1 | ||
| else | ||
| return 0 | ||
| fi |
There was a problem hiding this comment.
Suggested change
| if [ -z "$curr_value" ]; then | |
| if [ -n "${2-}" ]; then | |
| logger__log_errors "The $1 is not set while running \"${2-}\"${3-}" | |
| else | |
| logger__log_errors "The $1 is not set" | |
| fi | |
| return 1 | |
| else | |
| return 0 | |
| fi | |
| [[ -n "$curr_value" ]] && return 0 | |
| if [ -n "${2-}" ]; then | |
| logger__log_errors "The $1 is not set while running \"${2-}\"${3-}" | |
| else | |
| logger__log_errors "The $1 is not set" | |
| fi | |
| return 1 |
Comment on lines
+124
to
+140
| shell__validate_variable_unset() { | ||
|
|
||
| local curr_value | ||
| shell__copy_variable curr_value "${1-}" || return $? | ||
|
|
||
| if [ -n "$curr_value" ]; then | ||
| if [ -n "${2-}" ]; then | ||
| logger__log_errors "The $1 is set while running \"${2-}\"${3-}" | ||
| else | ||
| logger__log_errors "The $1 is set" | ||
| fi | ||
| return 1 | ||
| else | ||
| return 0 | ||
| fi | ||
|
|
||
| } |
| package_dir="$TERMUX_SCRIPTDIR/$packages_dir/$package_name" | ||
|
|
||
| # Search for virtual static subpackage | ||
| elif [[ "$package_name" == *"-static" ]] && [ -f "$TERMUX_SCRIPTDIR/$packages_dir/${package_name%-static}/build.sh" ]; then |
There was a problem hiding this comment.
Suggested change
| elif [[ "$package_name" == *"-static" ]] && [ -f "$TERMUX_SCRIPTDIR/$packages_dir/${package_name%-static}/build.sh" ]; then | |
| elif [[ "$package_name" == *"-static" && -f "$TERMUX_SCRIPTDIR/$packages_dir/${package_name%-static}/build.sh" ]]; then |
Yeah, thanks for looking into it, but I wasn't looking for a serious review right now, other than maybe buildorder. These changes are not meant to be merged, as I commented above. The build dependency changes would get pushed to master, the buildorder likely finalized in a separate PR. Python one for whoever decides to pick it up. The bootstrap and other infra changes can be merged from here, but that's long ahead. I don't have time right now, I just pushed this now for users and forkers who may want to build bootstraps for their apps, as that has been broken for years, and it will still be a few weeks/months before my bootstrap changes are finalized. |
are these results from a recent version of the repository, or are they more of a test from an older version of the repository? If they are from an older version and this will eventually be updated to match the current repository, that is completely ok. It is my understanding from testing that after it is no longer be possible to build |
|
The tests were run after rebasing this branch against upstream master 2 days ago, including bootstrap zips diff. https://github.com/agnostic-apollo/termux-packages/commits/infra-improvs/ Ah, then easier to revert that than moving python-tkinter to a separate package. I wasn't seeing the cycle a few months ago, and surely that's the cause. |
agnostic-apollo
force-pushed
the
infra-improvs
branch
from
63cfe26 to
87a9ccb
Compare
…e did not need to be built, then do not check if `_tkinter` module was built
agnostic-apollo
force-pushed
the
infra-improvs
branch
from
87a9ccb to
c35a832
Compare
|
I dropped 63cfe26 in my last push as the cycle got fixed in d09a983 and it readded the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.