Re: [PATCH] kbuild: give up untracked files for source package builds

[Nicolas Schier <nicolas@fjasle.eu>]

[-- Attachment #1: Type: text/plain, Size: 10131 bytes --]

On Tue, Apr 04, 2023 at 11:17:58AM +0900 Masahiro Yamada wrote:
> When the source tree is dirty and contains untracked files, package
> builds may fail. For example, when a broken symlink exists, a file
> path contains whitespaces, etc.
> 
> Since commit 05e96e96a315 ("kbuild: use git-archive for source package
> creation"), the source tarball only contains committed files because
> it is created by 'git archive'. scripts/package/gen-diff-patch tries
> to address the diff from HEAD, but including untracked files by the
> hand-crafted script introduces more complexity. I wrote a patch [1] to
> make it work in most cases, but still wonder if this is what we should
> aim for.
> 
> This patch just gives up untracked files. Going forward, it is your
> responsibility to do 'git add' for what you want in the source package.
> The script shows a warning just in case you forgot to do so. It should
> be checked only when building source packages.
> 
> [1]: https://lore.kernel.org/all/CAK7LNAShbZ56gSh9PrbLnBDYKnjtTkHMoCXeGrhcxMvqXGq9=g@mail.gmail.com/2-0001-kbuild-make-package-builds-more-robust.patch

With regard to your question concerning [1], I was thinking about
gbp-buildpackage's default behaviour: it denies package builds when untracked
files are found.  I think, you chose a good compromise.

> 
> Fixes: 05e96e96a315 ("kbuild: use git-archive for source package creation")
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/Makefile.package       |  3 +-
>  scripts/package/gen-diff-patch | 57 +++++++++++++----------------
>  scripts/package/mkdebian       | 66 +++++++++++++++++++---------------
>  scripts/package/mkspec         | 11 ++----
>  4 files changed, 67 insertions(+), 70 deletions(-)
> 
> diff --git a/scripts/Makefile.package b/scripts/Makefile.package
> index 61f72eb8d9be..49aff12cb6ab 100644
> --- a/scripts/Makefile.package
> +++ b/scripts/Makefile.package
> @@ -94,7 +94,7 @@ binrpm-pkg:
>  		$(UTS_MACHINE)-linux -bb $(objtree)/binkernel.spec
>  
>  quiet_cmd_debianize = GEN     $@
> -      cmd_debianize = $(srctree)/scripts/package/mkdebian
> +      cmd_debianize = $(srctree)/scripts/package/mkdebian $(mkdebian-opts)
>  
>  debian: FORCE
>  	$(call cmd,debianize)
> @@ -103,6 +103,7 @@ PHONY += debian-orig
>  debian-orig: private source = $(shell dpkg-parsechangelog -S Source)
>  debian-orig: private version = $(shell dpkg-parsechangelog -S Version | sed 's/-[^-]*$$//')
>  debian-orig: private orig-name = $(source)_$(version).orig.tar.gz
> +debian-orig: mkdebian-opts = --need-source
>  debian-orig: linux.tar.gz debian
>  	$(Q)if [ "$(df  --output=target .. 2>/dev/null)" = "$(df --output=target $< 2>/dev/null)" ]; then \
>  		ln -f $< ../$(orig-name); \
> diff --git a/scripts/package/gen-diff-patch b/scripts/package/gen-diff-patch
> index f842ab50a780..a180ff94f655 100755
> --- a/scripts/package/gen-diff-patch
> +++ b/scripts/package/gen-diff-patch
> @@ -2,43 +2,36 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  
>  diff_patch="${1}"
> -untracked_patch="${2}"
>  srctree=$(dirname $0)/../..
>  
> -rm -f ${diff_patch} ${untracked_patch}
> -
> -if ! ${srctree}/scripts/check-git; then
> -	exit
> -fi
> -
> -mkdir -p "$(dirname ${diff_patch})" "$(dirname ${untracked_patch})"
> +mkdir -p "$(dirname ${diff_patch})"

shellcheck complains about missing quotes around "${diff_patch}".

>  
>  git -C "${srctree}" diff HEAD > "${diff_patch}"
>  
> -if [ ! -s "${diff_patch}" ]; then
> -	rm -f "${diff_patch}"
> +if [ ! -s "${diff_patch}" ] ||
> +   [ -z "$(git -C "${srctree}" ls-files --other --exclude-standard | head -n1)" ]; then

Did you leave out the 'rm -f "${diff_patch}"' to have a more static mkspec
output?  

>  	exit
>  fi
>  
> -git -C ${srctree} status --porcelain --untracked-files=all |
> -while read stat path
> -do
> -	if [ "${stat}" = '??' ]; then
> -
> -		if ! diff -u /dev/null "${srctree}/${path}" > .tmp_diff &&
> -			! head -n1 .tmp_diff | grep -q "Binary files"; then
> -			{
> -				echo "--- /dev/null"
> -				echo "+++ linux/$path"
> -				cat .tmp_diff | tail -n +3
> -			} >> ${untracked_patch}
> -		fi
> -	fi
> -done
> -
> -rm -f .tmp_diff
> -
> -if [ ! -s "${diff_patch}" ]; then
> -	rm -f "${diff_patch}"
> -	exit
> -fi
> +# The source tarball, which is generated by 'git archive', contains everything
> +# you committed in the repository. If you have local diff ('git diff HEAD'),
> +# it will go into ${diff_patch}. If untracked files are remaining, the resulting
> +# source package may not be correct.
> +#
> +# Examples:
> +#  - You modified a source file to add #include <linux/new-header.h>
> +#    but forgot to add include/linux/new-header.h
> +#  - You modified a Makefile to add 'obj-$(CONFIG_FOO) += new-dirver.o'
> +#    but you forgot to add new-driver.c
> +#
> +# You need to commit them, or at least stage them by 'git add'.

making the file(s) known to git by 'git add -N' would be sufficient; but that's
probably too much detail here.  Nevertheless, I think the explanation is
valueable!

> +#
> +# This script does not take care of untracked files because doing so would
> +# introduce additional complexity. Instead, print a warning message here if
> +# untracked files are found.
> +# If all untracked files are just garbage, you can ignore this warning.
> +echo >&2 "============================ WARNING ============================"
> +echo >&2 "Your working tree has diff from HEAD, and also untracked file(s)."
> +echo >&2 "Please make sure you did 'git add' for all new files you need in"
> +echo >&2 "the source package."
> +echo >&2 "================================================================="
> diff --git a/scripts/package/mkdebian b/scripts/package/mkdebian
> index e20a2b5be9eb..220b5e35fc13 100755
> --- a/scripts/package/mkdebian
> +++ b/scripts/package/mkdebian
> @@ -84,7 +84,45 @@ set_debarch() {
>  	fi
>  }
>  
> +# Create debian/source/ if it is a source package build
> +gen_source ()
> +{
> +	mkdir -p debian/source
> +
> +	echo "3.0 (quilt)" > debian/source/format
> +
> +	{
> +		echo "diff-ignore"
> +		echo "extend-diff-ignore = .*"
> +	} > debian/source/local-options
> +
> +	# Add .config as a patch
> +	mkdir -p debian/patches
> +	{
> +		echo "Subject: Add .config"
> +		echo "Author: ${maintainer}"
> +		echo
> +		echo "--- /dev/null"
> +		echo "+++ linux/.config"
> +		diff -u /dev/null "${KCONFIG_CONFIG}" | tail -n +3
> +	} > debian/patches/config
> +	echo config > debian/patches/series

I'd named it config.patch, but actually it is just a config, so this makes
sense to me as well.

> +
> +	$(dirname $0)/gen-diff-patch debian/patches/diff.patch

"${0%/*}" instead of $(dirname $0) would also be possible.

> +	if [ -s debian/patches/diff.patch ]; then
> +		echo diff.patch >> debian/patches/series
> +	else
> +		rm -f debian/patches/diff.patch
> +	fi
> +}
> +
>  rm -rf debian
> +mkdir debian
> +
> +if [ "$1" = --need-source ]; then
> +	gen_source
> +	shift

Might you want to remove the 'shift'?  It looks like mkdebian handles more
command line arguments but it doesn't, as far as I can see.  And in case it
will do in some future, argument handling had to be revised nevertheless.

> +fi
>  
>  # Some variables and settings used throughout the script
>  version=$KERNELRELEASE
> @@ -132,34 +170,6 @@ else
>          echo >&2 "Install lsb-release or set \$KDEB_CHANGELOG_DIST explicitly"
>  fi
>  
> -mkdir -p debian/source/
> -echo "3.0 (quilt)" > debian/source/format
> -
> -{
> -	echo "diff-ignore"
> -	echo "extend-diff-ignore = .*"
> -} > debian/source/local-options
> -
> -# Add .config as a patch
> -mkdir -p debian/patches
> -{
> -	echo "Subject: Add .config"
> -	echo "Author: ${maintainer}"
> -	echo
> -	echo "--- /dev/null"
> -	echo "+++ linux/.config"
> -	diff -u /dev/null "${KCONFIG_CONFIG}" | tail -n +3
> -} > debian/patches/config
> -echo config > debian/patches/series
> -
> -$(dirname $0)/gen-diff-patch debian/patches/diff.patch debian/patches/untracked.patch
> -if [ -f debian/patches/diff.patch ]; then
> -	echo diff.patch >> debian/patches/series
> -fi
> -if [ -f debian/patches/untracked.patch ]; then
> -	echo untracked.patch >> debian/patches/series
> -fi
> -
>  echo $debarch > debian/arch
>  extra_build_depends=", $(if_enabled_echo CONFIG_UNWINDER_ORC libelf-dev:native)"
>  extra_build_depends="$extra_build_depends, $(if_enabled_echo CONFIG_SYSTEM_TRUSTED_KEYRING libssl-dev:native)"
> diff --git a/scripts/package/mkspec b/scripts/package/mkspec
> index b7d1dc28a5d6..b45020d64218 100755
> --- a/scripts/package/mkspec
> +++ b/scripts/package/mkspec
> @@ -19,8 +19,7 @@ else
>  	mkdir -p rpmbuild/SOURCES
>  	cp linux.tar.gz rpmbuild/SOURCES
>  	cp "${KCONFIG_CONFIG}" rpmbuild/SOURCES/config
> -	$(dirname $0)/gen-diff-patch rpmbuild/SOURCES/diff.patch rpmbuild/SOURCES/untracked.patch
> -	touch rpmbuild/SOURCES/diff.patch rpmbuild/SOURCES/untracked.patch
> +	$(dirname $0)/gen-diff-patch rpmbuild/SOURCES/diff.patch

Possibly change to "${0%/*}/gen-diff-patch", cp. above?

Thanks for the patch(es) and kind regards,
Nicolas

Reviewed-by: Nicolas Schier <nicolas@fjasle.eu>

>  fi
>  
>  if grep -q CONFIG_MODULES=y include/config/auto.conf; then
> @@ -56,7 +55,6 @@ sed -e '/^DEL/d' -e 's/^\t*//' <<EOF
>  $S	Source0: linux.tar.gz
>  $S	Source1: config
>  $S	Source2: diff.patch
> -$S	Source3: untracked.patch
>  	Provides: $PROVIDES
>  $S	BuildRequires: bc binutils bison dwarves
>  $S	BuildRequires: (elfutils-libelf-devel or libelf-devel) flex
> @@ -94,12 +92,7 @@ $S$M
>  $S	%prep
>  $S	%setup -q -n linux
>  $S	cp %{SOURCE1} .config
> -$S	if [ -s %{SOURCE2} ]; then
> -$S		patch -p1 < %{SOURCE2}
> -$S	fi
> -$S	if [ -s %{SOURCE3} ]; then
> -$S		patch -p1 < %{SOURCE3}
> -$S	fi
> +$S	patch -p1 < %{SOURCE2}
>  $S
>  $S	%build
>  $S	$MAKE %{?_smp_mflags} KERNELRELEASE=$KERNELRELEASE KBUILD_BUILD_VERSION=%{release}
> -- 
> 2.37.2

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]