diff mbox

[v3] kmsxx: new package

Message ID 20160728193825.9199-1-maxime.ripard@free-electrons.com
State Accepted
Headers show

Commit Message

Maxime Ripard July 28, 2016, 7:38 p.m. UTC 
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

Comments

Yann E. MORIN July 28, 2016, 7:49 p.m. UTC | #1 
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@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Yann E. MORIN July 28, 2016, 8:05 p.m. UTC | #2 
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@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni July 28, 2016, 8:17 p.m. UTC | #3 
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
Yann E. MORIN July 28, 2016, 8:22 p.m. UTC | #4 
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.
Thomas Petazzoni July 28, 2016, 9:34 p.m. UTC | #5 
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
diff mbox

Patch

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))