Summary

Review of the Ubuntu Kernel package.

This is the beginning of a general discussion of Ubuntu kernel packaging, mostly centered on the number of kernel packages and the differentiation between them.

Preliminary suggestions for topics of discussion are:

Are we splitting into too many packages?

Should LBM (linux backports modules) be split into more packages divided by driver type?

Here are some specific issues to be discussed during the UDS session:

==============================

If you force a clean without the build-deps installed (which is strictly
speaking incorrect but often done to trigger the update of debian/
autogenerated files -- especially when you're working from a checkout before
kicking e.g. a pbuilder build or an upload), you'll end up with an empty contrl
file and your tree is broken (and you don't know which build-deps you need
anymore!); that's inconvenient, failure shouldn't overwrite control

==============================

debian/rules is plenty of inelegant handling:
# dpkg-buildpackage passes options that are incomptatible
# with the kernel build.
unexport CFLAGS
unexport LDFLAGS

=> we should override or filter out unsupported flags but aim at supporting as
many of them as possible

BenC: Why? The kernel provides very specific compiler options. It is impossible for us to know which ld/cc options are incompatible, and the kernel even overrides many itself. We don't want this is our build. If a user wants to compile their own kernel, they will install linux-source-x.x.x, and that will honor whatever cflags/ldflags they wish to add. There's no reason for us to support this in our build tree.

==============
export DH_COMPAT=4

=> debhelper >= 5 should be used (needs porting), and this should be set in
debian/compat

BenC: Point taken. We should review this and work to update our debhelper usage.

==============
export LC_ALL=C
export SHELL=/bin/bash -e

=> these changes break expectations of all called commands and subshells, and
get passed down to all subcommands; that's a really intrusive change; I believe
that:
a) each command for which we require english output (e.g. we parse the output)
we should set LC_ALL, but nothing more
b) setting SHELL to bash (but no export) is fine, but -e is too intrusive as a
default; when people write makefiles they might write rules like:
        rm foo; do_stuff
these constructs should be fixed to not fail under -e, but I still thing it's
wrong to assume all constructs will be -e compliant, as this isn't the default
in any makefile usually; it would be like changing the system-wide shell to
enforce -e on all scripts

BenC: No, it's not wrong. We want -e so we show all failures. If we don't, then we'll miss failures we should catch. False failures should be fixed, not papered over. LC_ALL is not a bad default. It means we have no surprises. The kernel has no international support, so there's no need for us to concern ourselves. Plus, this more closely duplicates what will happen on the build daemons.

==============
include debian/rules.d/$(arch).mk

this breaks when the tree doesn't have support for the current arch; this is
painful when e.g. you're trying to build a source package of a tree not
supporting your arch (e.g. I'm on amd64 and am trying to create a source
package from the lpia source kernel tree) because it fails make.  This is
required when one tries to pbuild a package because pbuilder builds a source
package, then copies to build env, then builds, then retrieves results from
build env.

This is also ugly because it doesn't have any number; it's hardcoded that this
happens after 0-common-vars and before 1-maintainer...
=> make that 10-common-vars, 20-arch, 30-maintainer and put:
-include debian/rules.$(arch)
in 20-arch, or just use 20-arch for a list of if (arch==lpia) / endif
constructs

BenC: We're going to ditch the numbers. They are meaningless. So ordering aside, the problem is that you can't build the source on an architecture that isn't supported. Easiest thing to do is add a '-' before this include.

==============

fix debian/rules.d handling so that it includes all rules.d files as sorted by
their number instead of hardcoding which to include...
something like $(foreach $(shell ls debian/rules.d | sort -n),include $(1)) or
something similar.

BenC: We'll get rid of the numbers.

==============

cleanup:
   # This gets rid of the d-i packages in control
   cp -f debian/control.stub debian/control
is weird; it's not clear to me why it's done

BenC: It's because the d-i packages are different per architecture. So the control file would look different depending on where debian/control is built. We don't want this.
LoicMinier: We need to fix kernel-wedge to output all control entries (for all udebs on all arches).

==============

 you'll probably see why I'm unhappy about handling of the numbering:

include debian/rules.d/2-binary-arch.mk
include debian/rules.d/5-udebs.mk
include debian/rules.d/3-binary-indep.mk
include debian/rules.d/4-checks.mk

 *cough*

BenC: Repetition...we'll remove the numbers.

==============

# Misc stuff

debian/control.stub is not the real target; this rule generates
debian/d-i/kernel-versions; in fact I recommend you write a generic file.in ->
file rule which substitutes anything you'd like to substitute; think
config.status.  It makes it:
  a) easier to add/remove variables which affect all files
  b) easier to read
  c) easier to rely on new generated files

would probably look like (taken from the gtk+ I wrote):
debian/%: debian/%.in
        dh_testdir
        sed \
                -e "s#@SONAME@#$(SONAME)#g" \
                -e "s#@APIVER@#$(APIVER)#g" \
                [...]
                $@.in > $@

then make your other targets depend on the files they need (aka real makefile
deps  :) 

This will also get us rid of ugly things like:
new=`echo $$i | sed 's/\.in$$//'`;

Or:
"cat $$i | sed ... > $$new"
(you can replace cat foo | sed > bar with sed foo > bar)

BenC: Send a patch for review.

==============

.PHONY: debian/control

Hmm.  There are sometimes good reasons to do this, but I'm tempted to think
it's not the case here.  One reason I sometimes invoke is that the timestamp of
control might be skewed when you're using a .diff.gz (it might be the same time
as the source files, even if it's older), but you're using native packages, so
it's not a good explanation.  My guess is that you don't have proper deps on
the files generating control.

I'd rather have a gencontrol or something target which is forced in this case
than using debian/control as a target and then claiming it's a .PHONY one...

BenC: We've never had issue with this before. You're assuming that native tarball is our default, and it isn't. We switch to .orig.tar.gz when upstream releases.

==============

I find it less readable to have constructs in the middle of rules to compute
values; for example:
        flavours="$(wildcard debian/control.d/vars.* debian/sub-flavours/*.vars)";\
=> this should really be a make variable...

BenC: I find it easier to not have to find a variable that is only used once, just to read the rule that uses it.

==============================

Consider offering git-buildpackage support (debian/gbp.conf config); it's an
excellent frontend to enforce common practice among team members; for example
you have a setting for the format of tags (in your case "Ubuntu-%(version)") so
that one can simply run git-buildpackage to tag the tree and it will do the
right thing.

BenC: We've never used this, so an intro into this tool would be helpful.

==============================
debian/rules.d/0-common-vars.mk

-include $(CURDIR)/../.intrepid-env

That's not very elegant... I'd recommend using explicit variables from the env
for this all the time.

BenC: Tim added this to make his life easier. I'll leave it up to him whether to remove it.
LoicMinier: will be taken out

==============

gitver=$(shell if test -f .git/HEAD; then cat .git/HEAD; else uuidgen; fi)
gitverpre=$(shell echo $(gitver) | cut -b -3)
gitverpost=$(shell echo $(gitver) | cut -b 38-40)

a) I think you want git-symbolic-ref
b) don't use "=" make constructs, they require make to execute this statement
every time it's used; if you use ":=" it will be computed only once
c) use dh_testroot to protect from the makefile being used from debian/ or a
higher level dir than the source dir

BenC: Patches welcome...

==============

ifeq ($(wildcard /CurrentlyBuilding),)

WTF

BenC: I wont even go into this one much. On the build daemon, we produce .ddeb's, and we don't want to create these monstrous packages during out test builds.

==============

release := $(shell sed -n '1s/^.*(\(.*\)-.*).*$$/\1/p' debian/changelog)

use dpkg-parsechangelog
DEBVERSION := $(shell dpkg-parsechangelog | sed -n -e 's/^Version: //p')
VERSION := $(shell echo $(DEBVERSION) | sed -e 's/-[^-]*$$//')

would be even better to deal with epochs, oh well

BenC: Again, patches welcome.

==============

ifeq ($(CONCURRENCY_LEVEL),)

=> there are standard ways to do this now; dpkg-buildpackage -j (debuild -j) andDEB_BUILD_OPTIONS_PARALLEL ?= $(patsubst parallel=%,%,$(filter parallel=%,$(DEB_BUILD_OPTIONS)))
MAKEFLAGS += $(if $(DEB_BUILD_OPTIONS_PARALLEL),-j$(DEB_BUILD_OPTIONS_PARALLEL))

(I think it's in policy now)

BenC: We support what policy says. We leave CONCURRENCY_LEVEL in for historical reasons (we supported concurrent builds before policy stated it).

==============

LOCAL_ENV_CC should just be CC; people can just pass that through

BenC: I don't have an opinion on this.

==============

ccache shouldn't be handled specially; just put it in your PATH and preserve
PATH

BenC: We don't have anything about ccache except a comment. We all use ccache, and rely on PATH to handle it.

==============================
debian/rules.d/1-maintainer.mk

printchanges, insertchanges, diffupstream: any good reason to force hiding of
the commands which are run?

BenC: Because the output is piped into files.

==============================
debian/rules.d/2-binary-arch.mk

# We don't want make removing intermediary stamps
.SECONDARY :

=> this looks intrusive; would like some rationale; probably only affects this
   makefile though

BenC: Already discussed. Even more important is there is already a comment.

==============

install-%: pkgdir = $(CURDIR)/debian/linux-image-$(abi_release)-$*
install-%: dbgpkgdir = $(CURDIR)/debian/linux-image-debug-$(abi_release)-$*
install-%: basepkg = linux-headers-$(abi_release)
install-%: hdrdir = $(CURDIR)/debian/$(basepkg)-$*/usr/src/$(basepkg)-$*
install-%: target_flavour = $*

 You don't need to set these in install-%: prefix; you can set them outside of
 the target definition.  My conventions in my debian/rules is UPPERCASE vars
 are computed once and constant and lowercase ones are "dynamic" (typically $*
 based or macros)

BenC: How can they be set globally when the vars use $*, and as such are based specifically on the target being built?

==============

        # The main image
        install -m644 -D $(builddir)/build-$*/$(kernel_file) \
                $(pkgdir)/boot/$(install_file)-$(abi_release)-$*

        install -m644 $(builddir)/build-$*/.config \
                $(pkgdir)/boot/config-$(abi_release)-$*
        install -m644 $(abidir)/$* \
                $(pkgdir)/boot/abi-$(abi_release)-$*
        ...
=> long list of install calls; why not dh_install?

BenC: Why should we?

==============

ifneq ($(skipdbg),true)
        dh_installchangelogs -p$(dbgpkg)
        dh_installdocs -p$(dbgpkg)
        dh_compress -p$(dbgpkg)
        dh_fixperms -p$(dbgpkg)
        dh_installdeb -p$(dbgpkg)
        dh_gencontrol -p$(dbgpkg)
        dh_md5sums -p$(dbgpkg)

 => Use -N$(dbgpkg) in DH_OPTIONS; but I don't see why it should all be
    conditionalized...


        # Hokay...here's where we do a little twiddling...
        mv ../$(dbgpkg)_$(release)-$(revision)_$(arch).deb \
                ../$(dbgpkg)_$(release)-$(revision)_$(arch).ddeb

don't mess with files in the parent dir urgh

BenC: We have to. Telling us not to mess with them and not giving an alternative, is not very helpful.

LoicMinier: use dh_builddeb --filename

        grep -v '^$(dbgpkg)_.*$$' debian/files > debian/files.new
        mv debian/files.new debian/files

 That's going to cause surprizes (".*$" is also not very elegant but heh).

BenC: What sort of surprises do you mean? Examples of why something is bad would be helpful.
LoicMinier: dpkg-gencontrol and -genchanges can take a -f

==============================

debian/rules.d/5-udebs.mk

        for i in $$imagelist; do \
          dpkg -x $$(ls ../linux-image-$$i\_$(release)-$(revision)_${arch}.deb) \
                debian/d-i-${arch}; \
          /sbin/depmod -b debian/d-i-${arch} $$i; \
        done

 wow; just install the files you want in the udebs in the udebs trees with debhelper and the files you want in kernel trees in kernels; that's all

BenC: Not sure what you mean here. This isn't to build the package, it's to prep a dir so that kernel-wedge can pull the right things for the udeb's.

ifeq ($(disable_d_i),)
        debian/rules do-binary-udebs;
endif

 same thing; debhelper will only act on packages it needs to act on, this is
 defined in control and doesn't need to be reimplemented in the kernel's rules

BenC: This isn't the point. We like to have the ability to disable d-i when we are doing local builds to avoid the overhead.

==============================
* It takes long to clone
2008/12/10 19:08:34
Initialized empty Git repository in /home/lool/git/ubuntu/ubuntu/ubuntu-jaunty/.git/
remote: Counting objects: 981959, done.
remote: Compressing objects: 100% (175354/175354), done.
remote: Total 981959 (delta 812437), reused 973807 (delta 804385)
Receiving objects: 100% (981959/981959), 278.44 MiB | 541 KiB/s, done.
Resolving deltas: 100% (812437/812437), done.
Checking out files: 100% (25943/25943), done.
2008/12/10 19:22:04 | pts/4 | zsh/3 

2008/12/10 19:26:46
Initialized empty Git repository in /home/lool/git/kernel/linux-2.6/.git/
remote: Counting objects: 980636, done.
remote: Compressing objects: 100% (161484/161484), done.
remote: Total 980636 (delta 818505), reused 978938 (delta 816954)
Receiving objects: 100% (980636/980636), 236.14 MiB | 2678 KiB/s, done.
Resolving deltas: 100% (818505/818505), done.
Checking out files: 100% (25254/25254), done.
2008/12/10 19:34:51 | pts/4 | zsh/3 

 => too many objects; do we need the full history?

APW: there are two simple solutions here:
 1. shallow clone  (git clone --depth N)
 1. using --reference when cloning  (git clone --reference ../linus-upstream)

==============================

No documentation in debian/; what about a debian/README.Source pointing at the
wiki pages?

BenC: Sound idea.

==============================

native package; this makes it simple to push a tarball, but strictly speaking
it would be more elegant to use the upstream tarball; in practice it's probably
more work for little gain (the ability to use the upstream tarball) especially
in ubuntu, and lot of risk (diff is applied as a patch)

I think the pain of having to get the tarball would be alleviated by using
pristine-tar though and it would get us rid of:
% debuild -S -sa
This package has a Debian revision number but there does not seem to be
an appropriate original tar file or .orig directory in the parent directory;
(expected linux_2.6.28.orig.tar.gz or ubuntu-jaunty.orig)
continue anyway? (y/n)

BenC: We use native tarball until the kernel release we are working with goes final, then we use the upstream as a .orig.tar.gz.

==============================

Add Vcs-Git and Vcs-Browse control headers so that debcheckout works

BenC: Vcs-Git already exists.

Check for lintian warnings and fix them or add overrides

W: linux source: debian-rules-sets-DH_COMPAT line 16
W: linux source: ancient-standards-version 3.6.1 (current is 3.8.0)

W: linux source: changelog-should-mention-nmu
BenC: This one is false.

E: linux source: build-depends-on-obsolete-package build-depends-indep: gs
W: linux source: native-package-with-dash-version
BenC: Same here, false positive.

LoicMinier: this was only a first pass, there might be more; there are also control files which can be cleaned up of old deps (pre-dapper versions!) and other modules (meta notably).

===============================

Release Note

Rationale

Use Cases

Assumptions

Design

Implementation

UI Changes

Code Changes

Migration

Test/Demo Plan

Unresolved issues

BoF agenda and discussion

pgraner's notes


CategorySpec

KernelTeam/Specs/BootPerformance/JauntyKernelPackaging (last edited 2008-12-16 10:42:39 by lool)