Re: [PATCH v2] kbuild: deb-pkg: remove the fakeroot builds support

[Ben Hutchings <ben@decadent.org.uk>]

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

On Wed, 2023-11-29 at 00:38 +0900, Masahiro Yamada wrote:
> In 2017, the dpkg suite introduced the rootless builds support with the
> following commits:
> 
>   - 2436807c87b0 ("dpkg-deb: Add support for rootless builds")
>   - fca1bfe84068 ("dpkg-buildpackage: Add support for rootless builds")
> 
> This feature is available in the default dpkg on Debian 10 and Ubuntu
> 20.04.
> 
> Remove the old method.

This seems reasonable.


> Additionally, export DEB_RULES_REQUIRES_ROOT=no in case debian/rules is
> invoked without dpkg-buildpackage. This change aligns with the Debian
> kernel commit 65206e29f378 ("Allow to run d/rules.real without root").

The Debian linux package has multiple makefiles used recursively
(rather than included).  The referenced commit is kind of a hack to
make rootless builds of a subset of binary packages work when invoking
one of the lower-level makefiles directly.

It works because the package runs dh_builddeb, which checks
DEB_RULES_REQUIRES_ROOT.  But setting DEB_RULES_REQUIRES_ROOT has
absolutely zero effect on dpkg-deb or other low-level tools.

> While the upstream kernel currently does not run dh_testroot, it may
> be useful in the future.

We can do one of:

1. Ignore DEB_RULES_REQUIRES_ROOT, assume that dpkg-deb supports
   --root-owner-group and use it unconditionally (your v1).
2. Check DEB_RULES_REQUIRES_ROOT, do either fakeroot and chown or
   dpkg-deb --root-owner-group (current behaviour), and maybe also do
   the equivalent of dh_testroot.
3. Delegate this to dh_builddeb.  Since we use dh_listpackages now,
   debhelper is already required and this would make things a lot
   simpler.

But the combination of changes in v2 does not make sense to me.

Ben.

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
> Changes in v2:
>   - add DEB_RULES_REQUIRES_ROOT=no to debian/rules
> 
>  scripts/Makefile.package     | 4 +---
>  scripts/package/builddeb     | 8 +-------
>  scripts/package/debian/rules | 2 ++
>  3 files changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/scripts/Makefile.package b/scripts/Makefile.package
> index 0c3adc48dfe8..a81dfb1f5181 100644
> --- a/scripts/Makefile.package
> +++ b/scripts/Makefile.package
> @@ -109,8 +109,6 @@ debian-orig: linux.tar$(debian-orig-suffix) debian
>  		cp $< ../$(orig-name); \
>  	fi
>  
> -KBUILD_PKG_ROOTCMD ?= 'fakeroot -u'
> -
>  PHONY += deb-pkg srcdeb-pkg bindeb-pkg
>  
>  deb-pkg:    private build-type := source,binary
> @@ -125,7 +123,7 @@ deb-pkg srcdeb-pkg bindeb-pkg:
>  	$(if $(findstring source, $(build-type)), \
>  		--unsigned-source --compression=$(KDEB_SOURCE_COMPRESS)) \
>  	$(if $(findstring binary, $(build-type)), \
> -		-R'$(MAKE) -f debian/rules' -j1 -r$(KBUILD_PKG_ROOTCMD) -a$$(cat debian/arch), \
> +		-R'$(MAKE) -f debian/rules' -j1 -a$$(cat debian/arch), \
>  		--no-check-builddeps) \
>  	$(DPKG_FLAGS))
>  
> diff --git a/scripts/package/builddeb b/scripts/package/builddeb
> index d7dd0d04c70c..2fe51e6919da 100755
> --- a/scripts/package/builddeb
> +++ b/scripts/package/builddeb
> @@ -36,19 +36,13 @@ create_package() {
>  	sh -c "cd '$pdir'; find . -type f ! -path './DEBIAN/*' -printf '%P\0' \
>  		| xargs -r0 md5sum > DEBIAN/md5sums"
>  
> -	# Fix ownership and permissions
> -	if [ "$DEB_RULES_REQUIRES_ROOT" = "no" ]; then
> -		dpkg_deb_opts="--root-owner-group"
> -	else
> -		chown -R root:root "$pdir"
> -	fi
>  	# a+rX in case we are in a restrictive umask environment like 0077
>  	# ug-s in case we build in a setuid/setgid directory
>  	chmod -R go-w,a+rX,ug-s "$pdir"
>  
>  	# Create the package
>  	dpkg-gencontrol -p$pname -P"$pdir"
> -	dpkg-deb $dpkg_deb_opts ${KDEB_COMPRESS:+-Z$KDEB_COMPRESS} --build "$pdir" ..
> +	dpkg-deb --root-owner-group ${KDEB_COMPRESS:+-Z$KDEB_COMPRESS} --build "$pdir" ..
>  }
>  
>  install_linux_image () {
> diff --git a/scripts/package/debian/rules b/scripts/package/debian/rules
> index 3dafa9496c63..f23d97087948 100755
> --- a/scripts/package/debian/rules
> +++ b/scripts/package/debian/rules
> @@ -5,6 +5,8 @@ include debian/rules.vars
>  
>  srctree ?= .
>  
> +export DEB_RULES_REQUIRES_ROOT := no
> +
>  ifneq (,$(filter-out parallel=1,$(filter parallel=%,$(DEB_BUILD_OPTIONS))))
>      NUMJOBS = $(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
>      MAKEFLAGS += -j$(NUMJOBS)

-- 
Ben Hutchings
For every complex problem
there is a solution that is simple, neat, and wrong.


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]