kbuild: give up untracked files for source package builds

  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

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(-)
  

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
  

On Tue, Apr 4, 2023 at 5:05 PM Nicolas Schier <nicolas@fjasle.eu> wrote:
> >
> >  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}".


I will fix it.


>
> >
> >  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?


Yes.

I noticed it is ok to leave an empty patch file.

The 'patch' command exists with 0
when the input diff is empty.


$ patch -p1 < /dev/null
$ echo $?
0

However, dpkg-source warns of an empty patch file.
So, mkdebian should remove it when it gets empty.



> > +# 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!


Yeah, it is up to users how to remember to do 'git add'.
'git add -N' is a minute.



> > +
> > +     # 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.


I agree.
I will rename it in v2.


>
> > +
> > +     $(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.


OK. I will remove it.



> >  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?


dirname forks a process.

I will change it to ${srctree}/scripts/package/gen-diff-patch
so it works only with a variable expansion.