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

[Masahiro Yamada]

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







-- 
Best Regards
Masahiro Yamada