[libcamera-devel] [PATCH] utils: Add kernel headers update script

[Laurent Pinchart]

Hi Kieran,

On Mon, Mar 08, 2021 at 01:53:32PM +0000, Kieran Bingham wrote:
> On 08/03/2021 13:06, Laurent Pinchart wrote:
> > On Mon, Mar 08, 2021 at 12:23:19PM +0000, Kieran Bingham wrote:
> >> On 03/03/2021 17:39, Laurent Pinchart wrote:
> >>> Add a script to update the local copy of kernel headers (in
> >>> include/linux/) from a Linux kernel git tree. The script handles header
> >>> installation, manual processing of the IPU3 header that is still in
> >>> staging, and update of the README file.
> >>>
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >>> ---
> >>>  utils/update-kernel-headers.sh | 78 ++++++++++++++++++++++++++++++++++
> >>>  1 file changed, 78 insertions(+)
> >>>  create mode 100755 utils/update-kernel-headers.sh
> >>>
> >>> diff --git a/utils/update-kernel-headers.sh b/utils/update-kernel-headers.sh
> >>> new file mode 100755
> >>> index 000000000000..f5d91c8a8c08
> >>> --- /dev/null
> >>> +++ b/utils/update-kernel-headers.sh
> >>> @@ -0,0 +1,78 @@
> >>> +#!/bin/sh
> >>> +
> >>> +# SPDX-License-Identifier: GPL-2.0-or-later
> >>> +# Update the kernel headers copy from a kernel source tree
> >>> +
> >>> +if [ $# != 1 ] ; then
> >>> +	echo "Usage: $0 <kernel dir>"
> >>> +	exit 1
> >>> +fi
> >>> +
> >>> +header_dir="$(dirname "$(realpath "$0")")/../include/linux"
> >>> +kernel_dir="$1"
> >>> +
> >>> +# Bail out if the directory doesn't contain kernel sources
> >>> +line=$(head -3 "${kernel_dir}/Kbuild" 2>/dev/null | tail -1)
> >>> +if [ "$line" != "# Kbuild for top-level directory of the kernel" ] ; then
> >>> +	echo "Directory ${kernel_dir} doesn't contain a kernel source tree"
> >>> +	exit 1
> >>> +fi
> >>> +
> >>> +if [ ! -d "${kernel_dir}/.git" ] ; then
> >>> +	echo "Directory ${kernel_dir} doesn't contain a git tree"
> >>> +	exit 1
> >>> +fi
> >>> +
> >>> +# Install the headers to a temporary directory
> >>> +install_dir=$(mktemp -d)
> >>> +if [ ! -d "${install_dir}" ] ; then
> >>> +	echo "Failed to create temporary directory"
> >>> +	exit 1
> >>> +fi
> >>> +
> >>> +trap "rm -rf ${install_dir}" EXIT
> >>> +
> >>> +set -e
> >>> +make -C "${kernel_dir}" O="${install_dir}" headers_install
> >>> +set +e
> >>> +
> >>> +# Copy the headers
> >>> +headers="
> >>> +	drm/drm_fourcc.h
> >>> +	linux/dma-buf.h
> >>> +	linux/dma-heap.h
> >>> +	linux/media-bus-format.h
> >>> +	linux/media.h
> >>> +	linux/rkisp1-config.h
> >>> +	linux/v4l2-common.h
> >>> +	linux/v4l2-controls.h
> >>> +	linux/v4l2-mediabus.h
> >>> +	linux/v4l2-subdev.h
> >>> +	linux/videodev2.h
> >>> +"
> >>> +
> >>> +for header in $headers ; do
> >>> +	name=$(basename "${header}")
> >>> +	cp "${install_dir}/usr/include/${header}" "${header_dir}/${name}"
> >>> +done
> >>> +
> >>> +# The IPU3 header is a special case, as it's stored in staging. Handle it
> >>> +# manually.
> >>> +(cd "${install_dir}" ; "${kernel_dir}scripts/headers_install.sh" \
> >>> +	"${kernel_dir}/drivers/staging/media/ipu3/include/intel-ipu3.h" \
> >>> +	"${header_dir}/intel-ipu3.h")
> >>> +
> >>> +# Update the README file
> >>> +version=$(git --git-dir="${kernel_dir}/.git" describe)
> >>
> >> Should this have any other parameters?
> >>
> >>  --always --tags --dirty ?
> > 
> > We're expected to run this on a kernel git tree, if it doesn't have any
> > tag, there's something clearly wrong :-) --always shouldn't be needed.
> > Similarly, for --tags, as we're tracking mainline tags are annotated.
> > If there are local tags in a kernel tree on top a mailine tag, ignoring
> > them would be less confusing I think.
> 
> Are we always guaranteed to have pull tags locally?
> 
> Aren't there conditions where the local git tree might not have the
> tags? (Perhaps not, but that's why I suggested the always).

It's technically possible, but then I think it makes it a fairly bad
candidate as a source of upstream kernel headers. It's important to
record the exact version where the headers come from, and most of the
time that should be an upstream tag.

> > --dirty sounds good, and we should actually error out in that case. I'll
> > submit a v2.
> 
> Erroring out in that case sounds best indeed.
> 
> >>> +
> >>> +cat <<EOF > "${header_dir}/README"
> >>> +# SPDX-License-Identifier: CC0-1.0
> >>> +
> >>> +Files in this directory are imported from ${version} of the Linux kernel. Do not
> >>> +modify them manually.
> >>> +EOF
> >>> +
> >>> +# Cleanup
> >>> +rm -rf "${install_dir}"
> >>> +
> >>> +echo "Kernel headers updated"
> >>
> >> Are there any other manual steps required which should be mentioned at
> >> the end of the script so they do not get missed by someone running it?
> > 
> > How about this ?
> > 
> > diff --git a/utils/update-kernel-headers.sh b/utils/update-kernel-headers.sh
> > index f5d91c8a8c08..bb045d395359 100755
> > --- a/utils/update-kernel-headers.sh
> > +++ b/utils/update-kernel-headers.sh
> > @@ -75,4 +75,9 @@ EOF
> >  # Cleanup
> >  rm -rf "${install_dir}"
> > 
> > -echo "Kernel headers updated"
> > +cat <<EOF
> > +----------------------------------------------------------------------
> > +Kernel headers updated. Please review and up-port local changes before
> > +committing.
> > +----------------------------------------------------------------------
> > +EOF
> > 
> >> Otherwise
> >>
> >> Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>

-- 
Regards,

Laurent Pinchart