Message-ID: <20240708165342.GA2417540@thelio-3990X>
Date: Mon, 8 Jul 2024 09:53:42 -0700
From: Nathan Chancellor <nathan@...nel.org>
To: Thomas Weißschuh <linux@...ssschuh.net>
Cc: Masahiro Yamada <masahiroy@...nel.org>,
	Nicolas Schier <nicolas@...sle.eu>,
	"Jan Alexander Steffens (heftig)" <heftig@...hlinux.org>,
	linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org
Subject: Re: [PATCH v2] kbuild: add script and target to generate pacman
 package

On Mon, Jul 08, 2024 at 05:56:44PM +0200, Thomas Weißschuh wrote:
> Hi Nathan,
> 
> On 2024-07-07 22:50:46+0000, Nathan Chancellor wrote:
> > On Sat, Jul 06, 2024 at 09:33:46AM +0200, Thomas Weißschuh wrote:
> > > pacman is the package manager used by Arch Linux and its derivates.
> > > Creating native packages from the kernel tree has multiple advantages:
> > > 
> > > * The package triggers the correct hooks for initramfs generation and
> > >   bootloader configuration
> > > * Uninstallation is complete and also invokes the relevant hooks
> > > * New UAPI headers can be installed without any manual bookkeeping
> > > 
> > > The PKGBUILD file is a simplified version of the one used for the
> > > downstream Arch Linux "linux" package.
> > > Extra steps that should not be necessary for a development kernel have
> > > been removed and an UAPI header package has been added.
> > > 
> > > Signed-off-by: Thomas Weißschuh <linux@...ssschuh.net>
> > 
> > Thanks a lot for addressing my comments. From a PKGBUILD perspective,
> > this looks good to me (I have a couple more comments below). I am not as
> > familiar with the Kbuild packaging infrastructure, so Masahiro might
> > have more comments on that, but it works for me in my basic testing so
> > consider it:
> > 
> > Reviewed-by: Nathan Chancellor <nathan@...nel.org>
> > Tested-by: Nathan Chancellor <nathan@...nel.org>
> 
> Thanks!
> 
> > 
> > > ---
> > > Changes in v2:
> > > - Replace ${MAKE} with $MAKE for consistency with other variables
> > > - Use $MAKE for "-s image_name"
> > > - Avoid permission warnings from build directory
> > > - Clarify reason for /build symlink removal
> > > - Install System.map and config
> > > - Install dtbs where available
> > > - Allow cross-build through arch=any
> > > - Sort Contributor/Maintainer chronologically
> > > - Disable some unneeded makepkg options
> > > - Use DEPMOD=true for consistency with rpm-package
> > > - Link to v1: https://lore.kernel.org/r/20240704-kbuild-pacman-pkg-v1-1-ac2f63f5fa7b@weissschuh.net
> > > ---
> > >  .gitignore               |  6 ++++
> > >  scripts/Makefile.package | 15 +++++++++
> > >  scripts/package/PKGBUILD | 83 ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 104 insertions(+)
> 
> <snip>
> 
> > > diff --git a/scripts/package/PKGBUILD b/scripts/package/PKGBUILD
> > > new file mode 100644
> > > index 000000000000..fe899c77a976
> > > --- /dev/null
> > > +++ b/scripts/package/PKGBUILD
> > > @@ -0,0 +1,83 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> > > +# Maintainer: Thomas Weißschuh <linux@...ssschuh.net>
> > > +# Contributor: Jan Alexander Steffens (heftig) <heftig@...hlinux.org>
> > > +
> > > +pkgbase=linux-upstream
> > > +pkgname=("$pkgbase" "$pkgbase-headers" "$pkgbase-api-headers")
> > > +pkgver="${KERNELRELEASE//-/_}"
> > > +pkgrel="$KBUILD_REVISION"
> > > +pkgdesc='Linux'
> > > +url='https://www.kernel.org/'
> > > +arch=(any)
> > 
> > I see why you went this way but this feels a little dangerous because
> > this means the package will be installable on architectures other than
> > the one that it is built for. I think a better solution for this problem
> > would be moving arch back to $UTS_MACHINE but setting CARCH to that same
> > value in scripts/Makefile.package above. This diff works for me,
> > allowing me to build an aarch64 package on x86_64:
> 
> This is what I used in v1 of the patch.

You had $UTS_MACHINE as arch but I don't see where CARCH was set for
makepkg? If you tried to cross compile with v1, there was an error
because the default CARCH value (the host) does not match the arch
value, but explicitly passing CARCH to makepkg with $UTS_MACHINE should
allow makepkg to build a package that is only installable on that
machine?

> But I felt that this only works by pure chance.

I don't think it is by pure chance, it should be entirely based off of the
builder's ARCH value, no? I might be misunderstanding something though.

> IMO users of this feature should know what they are doing.
> 
> That said, if anybody has strong opinions on this, I'll be happy to change it.

I don't feel strongly about it but I think this is different from pretty
much all of the other package builds, which only build a package that is
installable/usable on one archictecture, and the solution seems
simple/robust enough.

> > diff --git a/scripts/Makefile.package b/scripts/Makefile.package
> > index 8c0c80f8bec0..a5b5b899d90c 100644
> > --- a/scripts/Makefile.package
> > +++ b/scripts/Makefile.package
> > @@ -151,6 +151,7 @@ pacman-pkg:
> >  		srctree="$(realpath $(srctree))" \
> >  		objtree="$(realpath $(objtree))" \
> >  		BUILDDIR="$(realpath $(objtree))/pacman" \
> > +		CARCH="$(UTS_MACHINE)" \
> >  		KBUILD_MAKEFLAGS="$(MAKEFLAGS)" \
> >  		KBUILD_REVISION="$(shell $(srctree)/init/build-version)" \
> >  		makepkg
> > diff --git a/scripts/package/PKGBUILD b/scripts/package/PKGBUILD
> > index fe899c77a976..7f1a4588c3d3 100644
> > --- a/scripts/package/PKGBUILD
> > +++ b/scripts/package/PKGBUILD
> > @@ -8,7 +8,7 @@ pkgver="${KERNELRELEASE//-/_}"
> >  pkgrel="$KBUILD_REVISION"
> >  pkgdesc='Linux'
> >  url='https://www.kernel.org/'
> > -arch=(any)
> > +arch=($UTS_MACHINE)
> >  options=(!debug !strip !buildflags !makeflags)
> >  license=(GPL-2.0-only)
> >  
> > 
> > > +options=(!debug !strip !buildflags !makeflags)
> > > +license=(GPL-2.0-only)
> > > +
> 
> <snip>
> 
> > > +
> > > +package_linux-upstream-headers() {
> > > +  pkgdesc="Headers and scripts for building modules for the $pkgdesc kernel"
> > > +
> > > +  export MAKEFLAGS="${KBUILD_MAKEFLAGS}"
> > > +  cd "$objtree"
> > > +  local builddir="$pkgdir/usr/$MODLIB/build"
> > > +
> > > +  echo "Installing build files..."
> > > +  "$srctree/scripts/package/install-extmod-build" "$builddir"
> > > +
> > > +  echo "Installing System.map and config..."
> > > +  cp System.map "$builddir/System.map"
> > > +  cp .config "$builddir/.config"
> > 
> > Remove the dot on the installation location so that it is more visible.
> 
> This is the location used by the downstream linux-headers package.

Ah, I did not realize that!

> I can add a symlink for better visibility, though.

It probably isn't that big of a deal if it is in the package somewhere,
I don't feel strongly enough about it.

> > > +  echo "Adding symlink..."
> > > +  mkdir -p "$pkgdir/usr/src"
> > > +  ln -sr "$builddir" "$pkgdir/usr/src/$pkgbase"
> > > +}
> > > +
> 
> <snip>

Powered by blists - more mailing lists