[Buildroot] [PATCH v3] kmsxx: new package

[Buildroot] [PATCH v3] kmsxx: new package

[Maxime Ripard]

KMS++ is a suite of library and test tools to interact with KMS drivers in
the linux kernel.

Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

---
Changes from v2:
  - Dropped the python bindings
  - Rebased on top of master
  - Updated the version

 package/Config.in       |  1 +
 package/kmsxx/Config.in | 26 ++++++++++++++++++++++++++
 package/kmsxx/kmsxx.mk  | 45 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+)
 create mode 100644 package/kmsxx/Config.in
 create mode 100644 package/kmsxx/kmsxx.mk

diff --git a/package/Config.in b/package/Config.in
index 0f0c3769ab33..9867a84db06c 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -941,6 +941,7 @@ menu "Graphics"
 	source "package/imlib2/Config.in"
 	source "package/jasper/Config.in"
 	source "package/jpeg/Config.in"
+	source "package/kmsxx/Config.in"
 	source "package/lcms2/Config.in"
 	source "package/lesstif/Config.in"
 	source "package/libart/Config.in"
diff --git a/package/kmsxx/Config.in b/package/kmsxx/Config.in
new file mode 100644
index 000000000000..db3c74b45310
--- /dev/null
+++ b/package/kmsxx/Config.in
@@ -0,0 +1,26 @@
+config BR2_PACKAGE_KMSXX
+	bool "kmsxx"
+	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8
+	depends on BR2_INSTALL_LIBSTDCPP
+	depends on BR2_TOOLCHAIN_HAS_THREADS # libpthread-stubs
+	select BR2_PACKAGE_LIBDRM
+	help
+	  libkms++ is a C++11 library for kernel mode setting.
+
+	  Also included are simple test tools for KMS and python wrapper for
+	  libkms++.
+
+	  https://github.com/tomba/kmsxx
+
+if BR2_PACKAGE_KMSXX
+
+config BR2_PACKAGE_KMSXX_INSTALL_TESTS
+	bool "Install test programs"
+	help
+	  This option allows to install the kmsxx test programs.
+
+endif
+
+comment "kmsxx needs a toolchain w/ threads"
+        depends on !BR2_TOOLCHAIN_HAS_THREADS
+
diff --git a/package/kmsxx/kmsxx.mk b/package/kmsxx/kmsxx.mk
new file mode 100644
index 000000000000..6eaa85452dfb
--- /dev/null
+++ b/package/kmsxx/kmsxx.mk
@@ -0,0 +1,45 @@
+################################################################################
+#
+# kmsxx
+#
+################################################################################
+
+KMSXX_VERSION = a706f157b86e90696808025db01de99646d51a77
+KMSXX_SITE = $(call github,tomba,kmsxx,$(KMSXX_VERSION))
+KMSXX_LICENSE = MPLv2.0
+KMSXX_LICENSE_FILES = LICENSE
+
+KMSXX_DEPENDENCIES += libdrm
+KMSXX_CONF_OPTS += -DKMSXX_ENABLE_PYTHON=OFF
+
+ifeq ($(BR2_PACKAGE_KMSXX_INSTALL_TESTS),y)
+define KMSXX_INSTALL_TARGET_TESTS
+	$(INSTALL) -D -m 0755 $(@D)/bin/fbtestpat \
+		$(TARGET_DIR)/usr/bin/fbtestpat
+	$(INSTALL) -D -m 0755 $(@D)/bin/kmsblank \
+		$(TARGET_DIR)/usr/bin/kmsblank
+	$(INSTALL) -D -m 0755 $(@D)/bin/kmscapture \
+		$(TARGET_DIR)/usr/bin/kmscapture
+	$(INSTALL) -D -m 0755 $(@D)/bin/kmsprint \
+		$(TARGET_DIR)/usr/bin/kmsprint
+	$(INSTALL) -D -m 0755 $(@D)/bin/kmsview \
+		$(TARGET_DIR)/usr/bin/kmsview
+	$(INSTALL) -D -m 0755 $(@D)/bin/testpat \
+		$(TARGET_DIR)/usr/bin/testpat
+	$(INSTALL) -D -m 0755 $(@D)/bin/wbcap \
+		$(TARGET_DIR)/usr/bin/wbcap
+	$(INSTALL) -D -m 0755 $(@D)/bin/wbm2m \
+		$(TARGET_DIR)/usr/bin/wbm2m
+endef
+endif
+
+define KMSXX_INSTALL_TARGET_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++.so \
+		$(TARGET_DIR)/usr/lib/libkms++.so
+	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++util.so \
+		$(TARGET_DIR)/usr/lib/libkms++util.so
+
+	$(KMSXX_INSTALL_TARGET_TESTS)
+endef
+
+$(eval $(cmake-package))
-- 
2.9.2

[Buildroot] [PATCH v3] kmsxx: new package

[Yann E. MORIN]

Maxime, All,

On 2016-07-28 21:38 +0200, Maxime Ripard spake thusly:
> KMS++ is a suite of library and test tools to interact with KMS drivers in
> the linux kernel.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> ---
[--SNIP--]
> diff --git a/package/kmsxx/Config.in b/package/kmsxx/Config.in
> new file mode 100644
> index 000000000000..db3c74b45310
> --- /dev/null
> +++ b/package/kmsxx/Config.in
> @@ -0,0 +1,26 @@
> +config BR2_PACKAGE_KMSXX
> +	bool "kmsxx"
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_8
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_HAS_THREADS # libpthread-stubs

The comment "# libpthread-stubs" seems to imply that the dependency on
C++ is ihnerited from libpthread-stubs, but it is neither selected not
depended on; it is also not in the build dependency. Is it an oversight?

However, given the name of the package, I guess it needs C++ for itself.
So, whether libpthread-stubs is needed or not, you should drop the
comment altogether: C++ *is* needed by kms++.

> +	select BR2_PACKAGE_LIBDRM
> +	help
> +	  libkms++ is a C++11 library for kernel mode setting.
> +
> +	  Also included are simple test tools for KMS and python wrapper for
> +	  libkms++.

Drop the comment about the Python bindings, since we don't support them
(you dropped them sinc e v2 of the patch).

> +	  https://github.com/tomba/kmsxx
> +
> +if BR2_PACKAGE_KMSXX
> +
> +config BR2_PACKAGE_KMSXX_INSTALL_TESTS
> +	bool "Install test programs"
> +	help
> +	  This option allows to install the kmsxx test programs.
> +
> +endif

Usually, when there is a single option to a package, we do not use an
if-block, but just make that option depend on the package. But I'm fine
with an if-block (I prefer it; it's just not what we usually do).

> +comment "kmsxx needs a toolchain w/ threads"
> +        depends on !BR2_TOOLCHAIN_HAS_THREADS
> +
> diff --git a/package/kmsxx/kmsxx.mk b/package/kmsxx/kmsxx.mk
> new file mode 100644
> index 000000000000..6eaa85452dfb
> --- /dev/null
> +++ b/package/kmsxx/kmsxx.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# kmsxx
> +#
> +################################################################################
> +
> +KMSXX_VERSION = a706f157b86e90696808025db01de99646d51a77
> +KMSXX_SITE = $(call github,tomba,kmsxx,$(KMSXX_VERSION))
> +KMSXX_LICENSE = MPLv2.0
> +KMSXX_LICENSE_FILES = LICENSE
> +
> +KMSXX_DEPENDENCIES += libdrm
> +KMSXX_CONF_OPTS += -DKMSXX_ENABLE_PYTHON=OFF
> +
> +ifeq ($(BR2_PACKAGE_KMSXX_INSTALL_TESTS),y)
> +define KMSXX_INSTALL_TARGET_TESTS
> +	$(INSTALL) -D -m 0755 $(@D)/bin/fbtestpat \
> +		$(TARGET_DIR)/usr/bin/fbtestpat
> +	$(INSTALL) -D -m 0755 $(@D)/bin/kmsblank \
> +		$(TARGET_DIR)/usr/bin/kmsblank
> +	$(INSTALL) -D -m 0755 $(@D)/bin/kmscapture \
> +		$(TARGET_DIR)/usr/bin/kmscapture
> +	$(INSTALL) -D -m 0755 $(@D)/bin/kmsprint \
> +		$(TARGET_DIR)/usr/bin/kmsprint
> +	$(INSTALL) -D -m 0755 $(@D)/bin/kmsview \
> +		$(TARGET_DIR)/usr/bin/kmsview
> +	$(INSTALL) -D -m 0755 $(@D)/bin/testpat \
> +		$(TARGET_DIR)/usr/bin/testpat
> +	$(INSTALL) -D -m 0755 $(@D)/bin/wbcap \
> +		$(TARGET_DIR)/usr/bin/wbcap
> +	$(INSTALL) -D -m 0755 $(@D)/bin/wbm2m \
> +		$(TARGET_DIR)/usr/bin/wbm2m

    KMSXX_TESTS = fbtestpat kmsblank kmscapture ...
    define KMSXX_INSTALL_TARGET_TESTS
        $(foreach t,$(KMSXX_TESTS),\
            $(INSTALL) -D -m 0755 $(@D)/bin/$(t) $(TARGET_DIR/usr/bin/$(t))
         )
    endef

> +endef
> +endif
> +
> +define KMSXX_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++.so \
> +		$(TARGET_DIR)/usr/lib/libkms++.so
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++util.so \
> +		$(TARGET_DIR)/usr/lib/libkms++util.so
> +

No empty line above.

> +	$(KMSXX_INSTALL_TARGET_TESTS)
> +endef

Since it installs libraries, maybe it should be installed in staging as
well?

Regards,
Yann E. MORIN.

> +$(eval $(cmake-package))
> -- 
> 2.9.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3] kmsxx: new package

[Yann E. MORIN]

Maxime, All,

Well, I forgot a few nts in the previous mail... Here they are. ;-)

On 2016-07-28 21:38 +0200, Maxime Ripard spake thusly:
> KMS++ is a suite of library and test tools to interact with KMS drivers in
> the linux kernel.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> ---
> diff --git a/package/kmsxx/Config.in b/package/kmsxx/Config.in
> new file mode 100644
> index 000000000000..db3c74b45310
> --- /dev/null
> +++ b/package/kmsxx/Config.in
> @@ -0,0 +1,26 @@
> +config BR2_PACKAGE_KMSXX
> +	bool "kmsxx"

We can call it by its real name in the prompt: kms++ .

You should also provide a .hash file: Github provides reproducible
archives.

> diff --git a/package/kmsxx/kmsxx.mk b/package/kmsxx/kmsxx.mk
> new file mode 100644
> index 000000000000..6eaa85452dfb
> --- /dev/null
> +++ b/package/kmsxx/kmsxx.mk
> @@ -0,0 +1,45 @@
> +################################################################################
> +#
> +# kmsxx
> +#
> +################################################################################
> +
> +KMSXX_VERSION = a706f157b86e90696808025db01de99646d51a77
> +KMSXX_SITE = $(call github,tomba,kmsxx,$(KMSXX_VERSION))
> +KMSXX_LICENSE = MPLv2.0
> +KMSXX_LICENSE_FILES = LICENSE
> +
> +KMSXX_DEPENDENCIES += libdrm
> +KMSXX_CONF_OPTS += -DKMSXX_ENABLE_PYTHON=OFF

No need for "+=" here, just a plain "=".

> +define KMSXX_INSTALL_TARGET_CMDS
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++.so \
> +		$(TARGET_DIR)/usr/lib/libkms++.so
> +	$(INSTALL) -D -m 0755 $(@D)/lib/libkms++util.so \
> +		$(TARGET_DIR)/usr/lib/libkms++util.so

Aren't the libraries versioned? If they are, don't we need to provide
the versioning symlinks? (Not that it matters, we're not supposed to
have multiple versions of the same libs in Buildroot; it's just for
consistency and customs.)

Regards,
Yann E. MORIN.

> +	$(KMSXX_INSTALL_TARGET_TESTS)
> +endef
> +
> +$(eval $(cmake-package))
> -- 
> 2.9.2
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3] kmsxx: new package

[Thomas Petazzoni]

Hello,

On Thu, 28 Jul 2016 21:49:27 +0200, Yann E. MORIN wrote:

> > +	depends on BR2_INSTALL_LIBSTDCPP
> > +	depends on BR2_TOOLCHAIN_HAS_THREADS # libpthread-stubs  
> 
> The comment "# libpthread-stubs" seems to imply that the dependency on
> C++ is ihnerited from libpthread-stubs, but it is neither selected not
> depended on; it is also not in the build dependency. Is it an oversight?
> 
> However, given the name of the package, I guess it needs C++ for itself.
> So, whether libpthread-stubs is needed or not, you should drop the
> comment altogether: C++ *is* needed by kms++.

There is no comment on the C++ dependency, so I'm not sure what you
mean here.

The BR2_TOOLCHAIN_HAS_THREADS dependency is inherited from libdrm,
which itself inherits it from libpthread-stubs. So the way we typically
do it is:

	depends on BR2_TOOLCHAIN_HAS_THREADS # libdrm -> libpthread-stubs

Or just:

	depends on BR2_TOOLCHAIN_HAS_THREADS # libdrm

Thanks,

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

[Buildroot] [PATCH v3] kmsxx: new package

[Yann E. MORIN]

Thomas, All,

On 2016-07-28 22:17 +0200, Thomas Petazzoni spake thusly:
> On Thu, 28 Jul 2016 21:49:27 +0200, Yann E. MORIN wrote:
> > > +	depends on BR2_INSTALL_LIBSTDCPP
> > > +	depends on BR2_TOOLCHAIN_HAS_THREADS # libpthread-stubs  
> > 
> > The comment "# libpthread-stubs" seems to imply that the dependency on
> > C++ is ihnerited from libpthread-stubs, but it is neither selected not
> > depended on; it is also not in the build dependency. Is it an oversight?
> > 
> > However, given the name of the package, I guess it needs C++ for itself.
> > So, whether libpthread-stubs is needed or not, you should drop the
> > comment altogether: C++ *is* needed by kms++.
> 
> There is no comment on the C++ dependency, so I'm not sure what you
> mean here.

Grr... I read in diagonal, and in my eyes, the comment was on the
BR2_INSTALL_LIBSTDCPP line... :-/

> The BR2_TOOLCHAIN_HAS_THREADS dependency is inherited from libdrm,
> which itself inherits it from libpthread-stubs. So the way we typically
> do it is:
> 
> 	depends on BR2_TOOLCHAIN_HAS_THREADS # libdrm -> libpthread-stubs
> 
> Or just:
> 
> 	depends on BR2_TOOLCHAIN_HAS_THREADS # libdrm

Indeed.

Sorry for the noise.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v3] kmsxx: new package

[Thomas Petazzoni]

Hello,

On Thu, 28 Jul 2016 21:38:25 +0200, Maxime Ripard wrote:
> KMS++ is a suite of library and test tools to interact with KMS drivers in
> the linux kernel.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

I've applied, after doing numerous updates:

    [Thomas:
     - rename prompt to kms++, suggested by Yann E. Morin
     - fixup the thread dependency comment
     - remove the mention of the python wrapper in the Config.in help
       text, since they are not installed
     - fix the Config.in comment to mention the C++ and gcc >= 4.8
       dependencies
     - use = instead of += when appropriate
     - use a loop to install the test programs
     - use a loop to install the libraries
     - add installation to staging as well, both the libraries and header
       files
     - add missing dependency on host-pkgconf
     - add hash file.]

Also, it would be nice to tell upstream that CMake can not only build
stuff, but also install stuff. It is a bit silly to be using a
well-known build system, but not use it to provide proper install
targets.

Thanks a lot!

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