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

[robertkirkman]

• Fixes [CI]: worker should perform package skip BEFORE docker image is being pulled. #24116

• adds a new script to scripts, build-package-dry-run-simulation.sh, which is used to test whether the entire build-package.sh command would have both proceeded beyond this point,

termux-packages/scripts/build/termux_step_start_build.sh

Lines 13 to 16 in 040710f

	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
	fi

• and also proceeded beyond this point,

termux-packages/scripts/build/termux_step_start_build.sh

Lines 42 to 47 in 040710f

	if [ "$TERMUX_PKG_HAS_DEBUG" = "true" ]; then
	DEBUG="-dbg"
	else
	echo "Skipping building debug build for $TERMUX_PKG_NAME"
	exit 0
	fi

• or if it would have skipped all packages in the list of packages to build without building anything.

• if the build would have continued, a non-zero value is returned, but if all packages in the list of packages to build would have been skipped, zero is returned.

• in run-docker.sh, runs the command passed to run-docker.sh in build-package-dry-run-simulation.sh before doing anything else only if the command passed has build-package.sh in its first argument, and only continues if a non-zero value is returned by build-package-dry-run-simulation.sh, which indicates either that at least one package will be built, or that the passed command is empty or not build-package.sh.

[robertkirkman]

• I am not sure whether this is fully compatible with the case of running the scripts/run-docker.sh command on a MacOS or BSD computer that has a VM-based docker command installed. This scripts/run-docker.sh has been tested on a GNU/Linux computer with a traditional native-container docker command installed. If someone who has MacOS or BSD regularly uses scripts/run-docker.sh locally on their computer, it might be convenient if they could help me test this to implement it fully for their device.

[twaik]

LGTM.

[agnostic-apollo]

Add a check for is $1 ends with /build-package.sh before running the command. You should not be passing --dummy arg to random commands and see if they succeed.

[agnostic-apollo]

Additionally, I am not sure if running the build-package.sh command outside the docker container on host system is safe, you would have to ensure no creation/deletion of files/directories is done on the host system. Such code paths may not run now, but someone could add them without knowing in future if not careful.

[robertkirkman]

Add a check for is $1 ends with /build-package.sh before running the command. You should not be passing --dummy arg to random commands and see if they succeed.

I agree, I put it like this because I can't actually think of any way to create a scenario in testing this version where something bad happens that wouldn't have already been bad before this, but explicitly checking instead of explaining it in a comment might be slightly more readable and also would prevent undesired behavior in the edge case of someone running a different command through run-docker.sh that isn't build-package.sh but which does already have a --dummy command (when researching I couldn't find any existing commands that have that argument, but anyone could create a new one at any time and then it would exist, and if I change it to --dry-run like you suggested, there do already exist other commands which have that argument)

Additionally, I am not sure if running the build-package.sh command outside the docker container on host system is safe, you would have to ensure no creation/deletion of files/directories is done on the host system.

I checked as carefully as I can, and I am pretty sure that the only remaining file creation outside of Docker in this codepath on GNU/Linux computers is the touching of several empty, unique files in /tmp outside of docker here:

termux-packages/build-package.sh

Lines 15 to 16 in f255467

	echo -n " " > "$TERMUX_BUILD_PACKAGE_CALL_BUILT_PACKAGES_LIST_FILE_PATH"
	touch "$TERMUX_BUILD_PACKAGE_CALL_BUILDING_PACKAGES_LIST_FILE_PATH"

and here:

termux-packages/build-package.sh

Line 58 in f255467

touch "$TERMUX_BUILD_LOCK_FILE"

which do not seem particularly harmful. I believe I disabled all others, but "pretty sure" is not as strong of a guarantee as "absolutely impossible and cannot ever happen under any circumstances period".

Such code paths may not run now, but someone could add them without knowing in future if not careful.

I agree, this makes build-package.sh very slightly more complicated because if this is added, any future editors of build-package.sh or the scripts called from it must be aware of one additional variable, and it is completely true that someone could accidentally add additional file or folder creation or deletion commands in a section of the code that would also run in this mode, without adding any condition to check for the new variable.

The alternative is to implement the fake build by creating a duplicated replica of all of the logic that's used to calculate whether packages will be built or skipped, and the tradeoff of that would be that instead of the risk being someone accidentally adding code that creates or deletes files that could run on GNU/Linux outside of Docker, the risk would be that someone might accidentally change the logic that's used to calculate whether packages will be built or skipped without also making the equivalent changes in the duplicated replica logic at the same time, which would cause the duplicated logic to become out of sync and no longer correctly skip the exact same packages that would be skipped by the normal mode.

[agnostic-apollo]

Looking more into your problem, if I am not mistaken, you don't need to replicate entire functionality. Just create a new script that reads -a flag and then loops on package name args and, finds each build.sh file and sources it in a subshell and then checks if arch passed is in TERMUX_PKG_EXCLUDED_ARCHES, and its NOT, then exit early from script as at least one package will need to get built. If loop ends, then no package will be built, and you can skip running docker command.

[agnostic-apollo]

Instead of a loop, some find command shenanigans can be used too.

termux-packages/scripts/bin/test-buildorder-random

Line 13 in d549cdf

filter_packages=$(find packages -maxdepth 2 -type f -name build.sh | xargs -P$(nproc) -i sh -c "if ! grep -E "^TERMUX_PKG_EXCLUDED_ARCHES=.*${TERMUX_ARCH}" {} >/dev/null; then dirname {}; fi" | sort)

[robertkirkman]

Ok I have put a version in this PR that I think matches what you described as your method.

It does not modify any part of the original build-package.sh, instead it modifies only run-docker.sh and the new script.

I believe everything here is the bare minimum of what is required to do it this way. If I remove anything, errors happen with some edge case inputs.

If you believe that this way is the best, then we can use this method.

[robertkirkman]

There is a way I can avoid being forced to use /tmp or mktemp, which is by falsely setting TERMUX_ON_DEVICE_BUILD=true inside the script as a form of forcing the codepath of least resistance, but that might make the script significantly more confusing for others to try to read than it already is. Another way is by duplicating almost the entire contents of the termux_step_setup_variables() function into the script, but that would balloon the length of the script to far beyond even 100 lines.

[agnostic-apollo]

Ah, NDK will need to be set too. This looks good and what I had in mind.

The primary concern with your way of directly modifying build-package.sh was the danger of running commands on the host due to build-package.sh code paths including due to future changes. However, that should get solved by gating it with $TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES as per my code above, so your way should be fine too. Anyone who will be passing --dry-run should accept any dangers it may bring, and that should be added as warning to the help output.

So to decide I guess the more important question is whether we want --dry-run to test other things as well in future, and if so, then should go with that way, as maintaining a single script would be easier and would actually test build-package.sh script itself.

[robertkirkman]

It's possible that the relevant areas of the build-package.sh and termux_step_start_build() might not change rapidly, and could remain the same for a long time, in which case the way this is currently should be sufficient for a while. It's hard to predict exactly what changes will happen in the future, but I have put comments in as reminders for the future that if build-package.sh changes too much, the changes can be transferred to build-package-dry-run-simulation.sh by checking the relevant portions at a commit date close to the commit date of this PR, then comparing them to the same portions at a future commit date. I would suggest that we can go with the way this is currently, and only switch it back to the other way if too many problems arise with this way in the future.

[agnostic-apollo]

Okay, we will go with current way then for now. The run-docker.sh code must still be gated with $TERMUX_DOCKER__CONTAINER_EXEC_COMMAND__PRE_CHECK_IF_WILL_BUILD_PACKAGES as you are running properties.sh and other build scripts like termux_step_setup_variables, which should not be run on the host unless explicitly enabled.

I have made fixes in other comments.

[agnostic-apollo]

And you likely meant --dry-run instead of --dummy.

[robertkirkman]

And you likely meant --dry-run instead of --dummy.

I do like that name better for this also. I will change it to that.

[agnostic-apollo]

continue

[agnostic-apollo]

continue

[agnostic-apollo]

exit 85 # C_EX__NOOP

[agnostic-apollo]

echo ...
exit 0

[agnostic-apollo]

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

[twaik]

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?

[agnostic-apollo]

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

[robertkirkman]

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.

[agnostic-apollo]

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.

[agnostic-apollo]

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.

[agnostic-apollo]

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

[robertkirkman]

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.

[agnostic-apollo]

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.

[robertkirkman]

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.