Skip to content

chore(scripts/run-docker): implement dry run simulation to skip launching container if no packages would have been built #24168

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

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

chore(scripts/run-docker): implement dry run simulation to skip launching container if no packages would have been built #24168

Show file tree
Hide file tree
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
87 changes: 87 additions & 0 deletions scripts/build-package-dry-run-simulation.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
#!/bin/sh

# 'exit 1' in subshells will exit this script with a return value of 1,
# but 'exit 0' in subshells will not
set -e -u

TERMUX_SCRIPTDIR=$(cd "$(realpath "$(dirname "$0")")"; cd ..; pwd)
. "$TERMUX_SCRIPTDIR/scripts/properties.sh"
. "$TERMUX_SCRIPTDIR/scripts/build/termux_step_setup_variables.sh"
. "$TERMUX_SCRIPTDIR/scripts/build/termux_error_exit.sh"
TERMUX_ON_DEVICE_BUILD=false
BUILDSCRIPT_NAME="build-package.sh"
TMPDIR=$(mktemp -d 2>/dev/null || mktemp -d -t 'mytmpdir')
NDK="$TMPDIR/dummy-ndk"

# Please keep synchronized with the logic of lines 468-547 of build-package.sh
declare -a PACKAGE_LIST=()
while (($# >= 1)); do
case "$1" in
*"/$BUILDSCRIPT_NAME") ;;
-a)
if [ $# -lt 2 ]; then
termux_error_exit "./build-package.sh: option '-a' requires an argument"
fi
shift 1
if [ -z "$1" ]; then
termux_error_exit "Argument to '-a' should not be empty."
fi
export TERMUX_ARCH="$1"
;;
-d) export TERMUX_DEBUG_BUILD=true ;;
-*) ;;
*) PACKAGE_LIST+=("$1") ;;
esac
shift 1
done

# please keep synchronized with the logic of lines 592-656 of build-package.sh
for ((i=0; i<${#PACKAGE_LIST[@]}; i++)); do
(
TERMUX_PKG_NAME=$(basename "${PACKAGE_LIST[i]}")
export TERMUX_PKG_BUILDER_DIR=
for package_directory in $TERMUX_PACKAGES_DIRECTORIES; do
if [ -d "${TERMUX_SCRIPTDIR}/${package_directory}/${TERMUX_PKG_NAME}" ]; then
export TERMUX_PKG_BUILDER_DIR=${TERMUX_SCRIPTDIR}/$package_directory/$TERMUX_PKG_NAME
break
fi
done
if [ -z "${TERMUX_PKG_BUILDER_DIR}" ]; then
termux_error_exit "No package $TERMUX_PKG_NAME found in any of the enabled repositories. Are you trying to set up a custom repository?"
fi
TERMUX_PKG_BUILDER_SCRIPT=$TERMUX_PKG_BUILDER_DIR/build.sh

mkdir -p "$NDK"
echo "Pkg.Revision = $TERMUX_NDK_VERSION_NUM" > "$NDK/source.properties"
termux_step_setup_variables

# Please keep synchronized with the logic of lines 2-50 of scripts/build/termux_step_start_build.sh
source "$TERMUX_PKG_BUILDER_SCRIPT"

if [ -n "${TERMUX_PKG_EXCLUDED_ARCHES:=""}" ] && [ "$TERMUX_PKG_EXCLUDED_ARCHES" != "${TERMUX_PKG_EXCLUDED_ARCHES/$TERMUX_ARCH/}" ]; then
echo "Skipping building $TERMUX_PKG_NAME for arch $TERMUX_ARCH"
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

continue

fi

if [ "$TERMUX_DEBUG_BUILD" = "true" ] && [ "$TERMUX_PKG_HAS_DEBUG" = "false" ]; then
echo "Skipping building debug build for $TERMUX_PKG_NAME"
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

continue

fi

termux_error_exit "Ending dry run simulation (a normal build would have continued)"
Copy link
Member

Choose a reason for hiding this comment

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

echo ...
exit 0

)
done

if [ ${#PACKAGE_LIST[@]} -gt 0 ]; then
# at least one package name was parsed, but none of them reached "exit 1",
# so exit with return value 0 to indicate that no packages would have been built
echo "Ending dry run simulation ($BUILDSCRIPT_NAME would not have built any packages)"
exit 0
Copy link
Member

Choose a reason for hiding this comment

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

exit 85 # C_EX__NOOP

fi

# if this point is reached, assume that a combination of arguments
# that is either invalid or is not implemented in this script
# has been used, and the real build-package.sh
# needs to be run so that its own parser can interpret the arguments
# and display the appropriate error message
termux_error_exit "Ending dry run simulation (unknown arguments, pass to the real $BUILDSCRIPT_NAME for more information)"
14 changes: 14 additions & 0 deletions scripts/run-docker.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@ set -e -u

TERMUX_SCRIPTDIR=$(cd "$(realpath "$(dirname "$0")")"; cd ..; pwd)

# If build-package-dry-run-simulation.sh returns a non-zero exit value, or if
# $1 (the first argument passed to this script which runs docker) does not contain
# $BUILDSCRIPT_NAME, this condition will evaluate false and this script which
# runs docker will continue.
BUILDSCRIPT_NAME="build-package.sh"
case "${1:-}" in
*"/$BUILDSCRIPT_NAME")
if "$TERMUX_SCRIPTDIR/scripts/build-package-dry-run-simulation.sh" "$@" > /dev/null 2>&1; then
echo "$0: Exiting since $BUILDSCRIPT_NAME would not have built any packages"
exit 0
fi
;;
esac

Copy link
Member

@agnostic-apollo agnostic-apollo Apr 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

if [ "${TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES:-}" = "true" ]; then
	case "${1:-}" in
		*"/build-package.sh")
			RETURN_VALUE=0
			OUTPUT="$("$TERMUX_SCRIPTDIR/scripts/build-package-dry-run-simulation.sh" "$@" 2>&1)" || RETURN_VALUE=$?
			if [ $RETURN_VALUE -ne 0 ]; then
				echo "$OUTPUT" 1>&2
				if [ $RETURN_VALUE -eq 85 ]; then # C_EX__NOOP
					echo "$0: Exiting since 'build-package.sh' would not have built any packages"
					exit 0
				fi
                                exit $RETURN_VALUE
			fi
			;;
	esac
fi

Copy link
Member

Choose a reason for hiding this comment

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

Why exactly do we need TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES check here if we do not have any other place where we set it?

Copy link
Member

Choose a reason for hiding this comment

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

It needs to be exported in workflows when running run-docker to build packages.

Copy link
Contributor Author

@robertkirkman robertkirkman Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Why exactly do we need TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES check here if we do not have any other place where we set it?

I have a plan to try to implement a slightly different change that might be ok to agnostic-apollo, without using that variable.

agnostic-apollo says that the reason that variable is required is because

  • sourcing of properties.sh is performed in the script
  • running of termux_step_setup_variables is performed in the script

so I am going to try to remove those two things from my script and try to get it to still work.

For the outcome I prefer, I think of using that variable as a last resort.

Copy link
Member

Choose a reason for hiding this comment

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

Removing the sourcing should be fine too, as my initial suggestion didn't require sourcing the scripts. But running a simulation every time build-package.sh command is run from outside the docker contains still feels like a "feature" that should be conditionally enabled, as it will have latency. Its not just workflows where it is run and users may be running it too, so keeping the variable still makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

Now you have added a dependency of jq being installed on the host system before the container even runs.

Additionally, if you plan on sourcing, instead of grepping relevant variables, then you will need to source properties.sh as lot of package build.sh files depend on variables set by it like termux-{exec,core}, etc and sourcing without them will fail with unbound variable errors due to set -u being set. Additionally, hardcoding $TERMUX_PREFIX might not be a good idea anyways.

Just gate it with a variable and use current logic, otherwise explain why you hate the variable, other than its lengthy name, its lengthy because there are tonne of other variables being set in my rewrite in addition to the 10 commands supported and so variables specific to each command need to be under their own scopes.

Copy link
Member

Choose a reason for hiding this comment

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

Technically sourcing properties.sh would require jq on host too.

Copy link
Contributor Author

@robertkirkman robertkirkman Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Now you have added a dependency of jq being installed on the host system before the container even runs.

It is essentially copied from properties.sh here, so that is an example of beginning to simplify away properties.sh, therefore the jq dependency was already there. Further simplifying away could result in additional removal of dependencies like jq or repo.json itself.

termux-packages/scripts/properties.sh

Line 2042 in 3d51f54

TERMUX_PACKAGES_DIRECTORIES=$(jq --raw-output 'del(.pkg_format) | keys | .[]' "$TERMUX_PKGS__BUILD__REPO_ROOT_DIR/repo.json")

if you plan on sourcing, instead of grepping relevant variables, then you will need to source properties.sh as lot of package build.sh files depend on variables set by it like termux-{exec,core}, etc and sourcing without them will fail with unbound variable errors due to set -u being set.

Yes, this is why I need to create a test rig that runs a series of loops to test this script with every possible combination of architecture, package and debug flags, then add definitions of only the variables that are required. Alternatively I could use grep instead of source, but I think there is a possibility that might have its own, other side effects (because grep is not a bash interpreter), so a test rig is necessary in either case.

Additionally, hardcoding $TERMUX_PREFIX might not be a good idea anyways.

I am not completely sure yet, but I think that is only required as long as termux_step_setup_variables is required, and if I successfully remove the call to termux_step_setup_variables, then that will be possible to remove.

explain why you hate the variable

I want this locally on any Docker computer without manually editing run-docker.sh every time I clone a clean repository and without having to type TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES=true. However if that's not wanted by most users, then I can continue just waiting for the docker container to create and launch every time I want to check whether a command would build any packages.

Copy link
Member

Choose a reason for hiding this comment

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

Should have mentioned that earlier so that I would have considered that, but why pull a repository if you don't intend to build any packages, our workflow is special case of time saving. Anyways, just instead check if first arg of run-docker.sh equals --pre-check-if-will-build-packages or -p, then shift first arg and run logic. grep -E should be supported on all platforms and would be simpler than all the issues of properties.sh, other steps and variables required by build.sh files, and its already used elsewhere. And you don't need TERMUX_PACKAGES_DIRECTORIES either with jq dependency, you can just use find to find <package_name>/build.sh file and then run grep on it if only one file is found.

Copy link
Contributor Author

@robertkirkman robertkirkman Apr 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Should have mentioned that earlier so that I would have considered that

sorry, it was discussed briefly in the issue linked, but I had not repeated the explanation of that in this thread yet. Passing an argument to run-docker.sh to activate it is fine and I like that, and I will keep your other ideas in mind while making the next commit of this.

why pull a repository if you don't intend to build any packages, our workflow is special case of time saving

It is for the case when I want to build a single package or a long list of packages on a clean computer, but I forget or don't manually check which architectures it is currently enabled for, or whether it supports Debug mode, or I accidentally mistype the package name so the package name does not match any existing package, and I have to either manually double check the command I typed very carefully before running it, or run it and then wait for the docker container to actually download, launch and set up before the real build-package.sh parses the command arguments.

I have encountered that situation before which is why I believe this would help me. One other change I want to make, is, since I have a check for whether the package names are valid here already anyway, I need to connect it to run-docker.sh in such a way that the container is not run if any package names are found to be invalid.

CONTAINER_HOME_DIR=/home/builder
UNAME=$(uname)
if [ "$UNAME" = Darwin ]; then
Expand Down