From mboxrd@z Thu Jan  1 00:00:00 1970
From: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Date: Sat, 17 Jun 2017 15:45:09 +0200
Subject: [Buildroot] [PATCH 1/1] v4l2loopback: new package
In-Reply-To: <1497640190-12668-1-git-send-email-alexandre.esse.dev@gmail.com>
References: <1497640190-12668-1-git-send-email-alexandre.esse.dev@gmail.com>
Message-ID: <20170617154509.446abdee@windsurf.lan>
List-Id: <buildroot.busybox.net>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: buildroot@busybox.net

Hello,

Thanks for this contribution! The general idea of the patch is good,
but there are a number of minor nits here and there that should be
fixed. See below for the detailed comments.

On Fri, 16 Jun 2017 21:09:50 +0200, Alexandre Esse wrote:
> This package provides a kernel module and utilities in order to use v4l2loopback virtual devices.
> This module allows you to create "virtual video devices" normal (v4l2) applications will read these devices as if they were ordinary video devices, but the video will not be read from e.g. a capture card but instead it is generated by another application.

Good to include such an explanation in the commit log. However, it
should be wrapped at 72 characters.

> diff --git a/package/Config.in b/package/Config.in
> index c997e2a..95604ed 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -49,6 +49,7 @@ menu "Audio and video applications"
>  	source "package/udpxy/Config.in"
>  	source "package/upmpdcli/Config.in"
>  	source "package/v4l2grab/Config.in"
> +	source "package/v4l2loopback/Config.in"
>  	source "package/vlc/Config.in"
>  	source "package/vorbis-tools/Config.in"
>  	source "package/wavpack/Config.in"
> diff --git a/package/v4l2loopback/Config.in b/package/v4l2loopback/Config.in
> new file mode 100644
> index 0000000..82ed0ed
> --- /dev/null
> +++ b/package/v4l2loopback/Config.in
> @@ -0,0 +1,23 @@
> +config BR2_PACKAGE_V4L2LOOPBACK
> +	bool "v4l2loopback"
> +	help
> +                This module allows you to create "virtual video devices".
> +                Normal (v4l2) applications will read these devices as if they
> +                were ordinary video devices, but the video will not be read from
> +                e.g. a capture card but instead it is generated by another
> +                application.

Indentation for the help text is one tab + two spaces.

Have you run ./support/scripts/check-package on your package ? It would
have detected this issue.

> +if BR2_PACKAGE_V4L2LOOPBACK
> +
> +config BR2_PACKAGE_V4L2LOOPBACK_UTILS
> +	bool "v4l2loopback-utils"
> +        select BR2_PACKAGE_BASH
> +        select BR2_PACKAGE_SUDO

Indentation is wrong here: should be one tab, not spaces.

Also, are these runtime dependencies? If so, you should indicate them
as such:

	select BR2_PACKAGE_BASH # runtime
	select BR2_PACKAGE_SUDO # runtime

so that we don't wonder why the .mk file does not reference them in the
<pkg>_DEPENDENCIES variable.

It's a bit of a pitty that the v4l2loopback-ctl script requires bash
and sudo, but OK, it's really the case, so no choice here.

> +	help
> +                This package contains applications to interact with
> +                v4l2-loopback devices ("virtual video devices").
> +                Currently there is only a single command line utility:
> +                        v4l2loopback-ctl: tool to set framerate, format and
> +                        timeout image

Indentation should be one tab + two spaces.

> diff --git a/package/v4l2loopback/v4l2loopback.hash b/package/v4l2loopback/v4l2loopback.hash
> new file mode 100644
> index 0000000..2678ca7
> --- /dev/null
> +++ b/package/v4l2loopback/v4l2loopback.hash
> @@ -0,0 +1,3 @@
> +sha256 9bb1e8d544019bead20813877415ae974fbc22f87c69772984a4abac433f36dd  v4l2loopback-v0.10.0.tar.gz
> +sha256 c8942b5cfb8d337ec0ef4668608facbf9c5aa28fb1cae38674194f6d698733b3  v4l2loopback-v0.9.1.tar.gz
> +sha256 8b4ad8b61d2333eca6f88d9e3060a27bcd45021469e8464082bdcbba5336211f  v4l2loopback-v0.9.0.tar.gz

Why define so many hashes? Just define the one of the version currently
used in Buildroot. Also please add a comment that indicate where the
hash is coming from.

> +V4L2LOOPBACK_VERSION = v0.10.0
> +V4L2LOOPBACK_SITE = $(call github,umlaeute,v4l2loopback,$(V4L2LOOPBACK_VERSION))
> +V4L2LOOPBACK_LICENSE = GPLv2

Please use GPL-2.0 instead.

> +V4L2LOOPBACK_LICENSE_FILES = COPYING
> +
> +ifeq ($(BR2_PACKAGE_V4L2LOOPBACK_UTILS),y)
> +

Remove this blank line.

> +define V4L2LOOPBACK_INSTALL_TARGET_CMDS
> +        $(INSTALL) -D -m 0755 $(@D)/utils/* $(TARGET_DIR)/usr/bin

Specify the full path:

	$(INSTALL) -D -m 0755 $(@D)/utils/v4l2loopback-ctl $(TARGET_DIR)/usr/bin/v4l2loopback-ctl

> +endef
> +

Remove this blank line.

> +endif
> +
> +$(eval $(kernel-module))
> +$(eval $(generic-package))

Could you fix the above issues, and submit an updated version of the
patch?

Thanks a lot!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com