termux / termux-packages

A package build system for Termux.
https://termux.dev
Other
12.8k stars 2.94k forks source link

[Bug]: Broken indentation for package maintainer scripts and other files using cat heredoc #20533

Open agnostic-apollo opened 1 month ago

agnostic-apollo commented 1 month ago

Problem description

I was looking at bootstraps and the indentation of files is broken that are generated during build time where a cat <<- EOF heredoc used to generate the file in indented with a tab in the build.sh file, and the content itself is also indented with tabs, like for nested conditionals, or line is continued with a trailing slash \. This affects packages that generate maintainer scripts like postinst, like editor packages e.g nano that run update-alternatives --install. Other packages generating other files are affected too.

For example, for the nano package, the build.sh file currently contains following.

termux_step_create_debscripts() {
    cat <<- EOF > ./postinst
    #!$TERMUX_PREFIX/bin/sh
    if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" = "configure" ] || [ "\$1" = "abort-upgrade" ]; then
        if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
            update-alternatives --install \
                $TERMUX_PREFIX/bin/editor editor $TERMUX_PREFIX/bin/nano 20
        fi
    fi
    EOF

    cat <<- EOF > ./prerm
    #!$TERMUX_PREFIX/bin/sh
    if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" != "upgrade" ]; then
        if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
            update-alternatives --remove editor $TERMUX_PREFIX/bin/nano
        fi
    fi
    EOF
}

The actual generated file in the package contains following. Note the lack of indents and tab characters between update-alternatives --install /data/... where trailing slash \ was used without escaping it \\ as it got processed while heredoc was read.

#!/data/data/com.termux/files/usr/bin/sh
if [ "debian" = "pacman" ] || [ "$1" = "configure" ] || [ "$1" = "abort-upgrade" ]; then
if [ -x "/data/data/com.termux/files/usr/bin/update-alternatives" ]; then
update-alternatives --install               /data/data/com.termux/files/usr/bin/editor editor /data/data/com.termux/files/usr/bin/nano 20
fi
fi

The lack of indents is because of usage of <<-. According to bash documentation.

If the redirection operator is <<-, then all leading tab characters are stripped from input lines and the line containing delimiter. This allows here-documents within shell scripts to be indented in a natural fashion.

Basically this means that tab indent of the cat herdoc itself are removed and also the tab indents of the content.

To fix this, the leading indents in the heredoc should be removed completely and trailing \ should be escaped like \\. The variables that are to be expanded at build time should ideally be quoted.

termux_step_create_debscripts() {
cat <<EOF > ./postinst
#!$TERMUX_PREFIX/bin/sh
if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" = "configure" ] || [ "\$1" = "abort-upgrade" ]; then
    if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
        update-alternatives --install \\
            "$TERMUX_PREFIX/bin/editor" editor "$TERMUX_PREFIX/bin/nano" 20
    fi
fi
EOF

cat <<EOF > ./prerm
#!$TERMUX_PREFIX/bin/sh
if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" != "upgrade" ]; then
    if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
        update-alternatives --remove editor "$TERMUX_PREFIX/bin/nano"
    fi
fi
EOF
}

The heredoc variant cat << ' EOF' | sed -E 's/^[\t]//' > ./foo suggested here will work too while keeping indents in the build file, but due to literal heredoc usage, it will require exporting all the environment variables in a subshell that are to be expanded in the content at build time and replaced manually with envsubst, in case not all variables in the content need to be expanded at build time. That gets a bit verbose though.

termux_step_create_debscripts() {
    (
    export TERMUX_PACKAGE_FORMAT TERMUX_PREFIX

    cat << '    EOF' | sed -E 's/^[\t]//' | \
        envsubst '$TERMUX_PACKAGE_FORMAT,$TERMUX_PREFIX' > ./postinst
    #!$TERMUX_PREFIX/bin/sh
    if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" = "configure" ] || [ "\$1" = "abort-upgrade" ]; then
        if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
            update-alternatives --install \
                "$TERMUX_PREFIX/bin/editor" editor "$TERMUX_PREFIX/bin/nano" 20
        fi
    fi
    EOF

    cat << '    EOF' | sed -E 's/^[\t]//' | \
        envsubst '$TERMUX_PACKAGE_FORMAT,$TERMUX_PREFIX' > ./prerm
    #!$TERMUX_PREFIX/bin/sh
    if [ "$TERMUX_PACKAGE_FORMAT" = "pacman" ] || [ "\$1" != "upgrade" ]; then
        if [ -x "$TERMUX_PREFIX/bin/update-alternatives" ]; then
            update-alternatives --remove editor "$TERMUX_PREFIX/bin/nano"
        fi
    fi
    EOF
    )

}

I personally won't be fixing this, package maintainers should do it. Searching for <<- can be used to find potentially affected packages.

What steps will reproduce the bug?

.

What is the expected behavior?

No response

System information

.
truboxl commented 1 month ago

For me that use this feature I don't see this as an issue. The indents bloat the file size and scripts are still working. Though the \\ is indeed needed for that particular case.

agnostic-apollo commented 1 month ago

A few bytes bloat the file size? We should remove indents for all the termux-tools and other packages too then. ;)

Scripts published to users should have proper formatting.

Grimler91 commented 1 month ago

Moving the postinst/prerm script into a hooks subfolder like a few packages have done already (and @Maxython proposed for openjdk recently) would be a better fix instead of trying to sed in build scripts I think

agnostic-apollo commented 1 month ago

Yeah, that would be better.

Biswa96 commented 1 month ago

Shouldn't the postinst/prerm scripts be added automatically instead of adding it in termux_step_create_debscripts in individual build scripts?

truboxl commented 1 month ago

Not all packages need postinst prerm scripts, and the scripts are in different places (in / out of subfolders), need to be standardized

Changing this a tracker

agnostic-apollo commented 1 month ago

Also note that other dynamically generated files are affected too in some cases instead of just maintainer scripts.