这是indexloc提供的服务，不要输入任何密码
Skip to content

enhancement: allow expanding environment variables in patches. #24641

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
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

enhancement: allow expanding environment variables in patches. #24641

wants to merge 1 commit into from
+49 −18

Conversation

twaik
Copy link
Member

@twaik twaik commented May 7, 2025 •
edited
Loading

Filter patch lines and expand environment variables enclosed within @@ literals.
Also scripts are a bit refactored for consistency with other recent changes.

This enhancement allows us write patches with lines like @@TERMUX_ARCH@@ to be automatically expanded from build-time variables.

It should help us avoid writing simple but long heredocs or long regular escaped strings inside buildscripts (like here) or define *.in files (like here) for subsequent variable expansion with writing to target directory. Everything can be done in a regular *.patch* file.

Additional variables can be assigned in termux_step_post_get_source (if you need additional variables or contextual data).

Example (just for reference, not for real use):

+++ b/Makefile
@@ -6,13 +6,10 @@
 #	Last Modified: 2019/03/19
 #==========================================
 
-CC=gcc
-CFLAGS=-O3 -Wall
-
 all: sl
 
 sl: sl.c sl.h
-	$(CC) $(CFLAGS) -o sl sl.c -lncurses
+	$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -o sl sl.c -lncurses @@TERMUX_ARCH@@
 
 clean:
 	rm -f sl

And this

+	$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -o sl sl.c -lncurses @@TERMUX_ARCH@@

is being expanded to

+	$(CC) $(CFLAGS) $(CPPFLAGS) $(LDFLAGS) -o sl sl.c -lncurses aarch64

@twaik twaik requested review from truboxl, robertkirkman and licy183 May 7, 2025 10:27
@twaik
Copy link
Member Author

twaik commented May 7, 2025

I checked all our patches with grep and got these results.

$ grep -nr '^[+].*@@.*@@' --include="*.patch*"
packages/netcat-openbsd/debian-patches-port-to-linux-with-libbsd.patch:27:+@@ -38,7 +38,8 @@

$

Since we do not have variable named -38,7 +38,8 defined anywhere we should be fine with this and I consider we should not implement any way for escaping @@.*@@ patterns.

@licy183
Copy link
Member

licy183 commented May 7, 2025

This will be useful for autotools, but as for chromium's gn, I think it is more readable and more maintainable to use a .in file rather than store them in *.patch.

@twaik
enhancement: allow expanding environment variables in patches.
c5b29ea 
Filter patch lines and expand environment variables enclosed within `@@` literals
@twaik twaik force-pushed the enhance-patching branch from ca5a2d6 to c5b29ea Compare May 7, 2025 12:19
robertkirkman
robertkirkman approved these changes May 7, 2025
View reviewed changes
Copy link
Contributor

@robertkirkman robertkirkman left a comment

Choose a reason for hiding this comment

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

I am not sure how many specific use cases this might be helpful for right now, but I think it does not break any existing builds, so it should be fine to add this.

It might be helpful to somewhat simplify the build.sh of vim in a future version. I force-cross-compiled the perl, ruby and tcl bindings of vim using a convoluted patch, and this feature might help reduce the length of that code in a future version.

termux-packages/packages/vim/build.sh

Lines 94 to 110 in 3f5084b

# Vim doesn't support cross-compilation for Perl, Ruby and Tcl
# out of the box, so we need to patch the configure script to make it work.
local perl_version ruby_major_version tcl_major_version
perl_version="$(. "$TERMUX_SCRIPTDIR/packages/perl/build.sh"; echo "${TERMUX_PKG_VERSION[0]}")"
ruby_major_version="$(. "$TERMUX_SCRIPTDIR/packages/ruby/build.sh"; echo "${TERMUX_PKG_VERSION%\.*}")"
tcl_major_version="$(. "$TERMUX_SCRIPTDIR/packages/tcl/build.sh"; echo "${TERMUX_PKG_VERSION%\.*}")"
patch="$TERMUX_PKG_BUILDER_DIR/configure-perl-ruby-tcl-cross-compiling.diff"
echo "Applying patch: $(basename "$patch")"
test -f "$patch" && sed \
-e "s%\@PERL_VERSION\@%${perl_version}%g" \
-e "s%\@RUBY_MAJOR_VERSION\@%${ruby_major_version}%g" \
-e "s%\@TCL_MAJOR_VERSION\@%${tcl_major_version}%g" \
-e "s%\@PERL_PLATFORM\@%${TERMUX_ARCH}-android%g" \
-e "s%\@RUBY_PLATFORM\@%${TERMUX_HOST_PLATFORM}%g" \
-e "s%\@TERMUX_PREFIX\@%${TERMUX_PREFIX}%g" \
"$patch" | patch --silent -p1

@twaik
Copy link
Member Author

twaik commented May 7, 2025

Actually I see 2 ways to improve current PR and make its usage more convenient.

  1. I've found ~120 usages of patch invocations, with using sed to replace termux-specific variables and without them. We can create a separate function for processing patches (to be used by these packages) and invoke this function from both termux_step_handle_host_build.sh and termux_step_patch_package.sh.
  2. There are *.in files and cat + sed invocations with variable expanding. We can make similar function for copying *.in with expanding variables which will be a bit more convenient than patches.

TomJo2000
TomJo2000 reviewed May 14, 2025
View reviewed changes
Copy link
Member

@TomJo2000 TomJo2000 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I'm spread a bit thin right now with PRs and reviews, but how is this different from the existing behavior in termux_step_patch_package?

termux-packages/scripts/build/termux_step_patch_package.sh

Lines 22 to 34 in 7b2747b

test -f "$patch" && sed \
-e "s%\@TERMUX_APP_PACKAGE\@%${TERMUX_APP_PACKAGE}%g" \
-e "s%\@TERMUX_BASE_DIR\@%${TERMUX_BASE_DIR}%g" \
-e "s%\@TERMUX_CACHE_DIR\@%${TERMUX_CACHE_DIR}%g" \
-e "s%\@TERMUX_HOME\@%${TERMUX_ANDROID_HOME}%g" \
-e "s%\@TERMUX_PREFIX\@%${TERMUX_PREFIX}%g" \
-e "s%\@TERMUX_PREFIX_CLASSICAL\@%${TERMUX_PREFIX_CLASSICAL}%g" \
-e "s%\@TERMUX_ENV__S_TERMUX\@%${TERMUX_ENV__S_TERMUX}%g" \
-e "s%\@TERMUX_ENV__S_TERMUX_APP\@%${TERMUX_ENV__S_TERMUX_APP}%g" \
-e "s%\@TERMUX_ENV__S_TERMUX_API_APP\@%${TERMUX_ENV__S_TERMUX_API_APP}%g" \
-e "s%\@TERMUX_ENV__S_TERMUX_ROOTFS\@%${TERMUX_ENV__S_TERMUX_ROOTFS}%g" \
-e "s%\@TERMUX_ENV__S_TERMUX_EXEC\@%${TERMUX_ENV__S_TERMUX_EXEC}%g" \
"$patch" | patch --silent -p1

One thing I want to make absolutely sure we do not add here is the ability to inject arbitrary environment variable expansion into patches.
That's user controlled input and should not be trusted.

@robertkirkman
Copy link
Contributor

robertkirkman commented May 14, 2025 •
edited
Loading

One thing I want to make absolutely sure we do not add here is the ability to inject arbitrary environment variable expansion into patches.
That's user controlled input and should not be trusted.

Could you describe an example of what the worst-case scenario for that would be, as you imagine it?
Patches are a build-time tool. After the package reaches the users, patches have become part of the code of the program and are normally no longer distinguishable from the rest of the code of the program from the user perspective.

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

@TomJo2000 TomJo2000 TomJo2000 left review comments

@robertkirkman robertkirkman robertkirkman approved these changes

@Grimler91 Grimler91 Awaiting requested review from Grimler91 Grimler91 is a code owner

@thunder-coding thunder-coding Awaiting requested review from thunder-coding thunder-coding is a code owner

@truboxl truboxl Awaiting requested review from truboxl

@licy183 licy183 Awaiting requested review from licy183

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@twaik @licy183 @robertkirkman @TomJo2000