[Buildroot] [PATCH] sdl2: new package

[Buildroot] [PATCH] sdl2: new package

[Guillaume GARDET]

Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>

---
 package/Config.in      |  1 +
 package/sdl2/Config.in | 22 +++++++++++++++++
 package/sdl2/sdl2.mk   | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+)
 create mode 100644 package/sdl2/Config.in
 create mode 100644 package/sdl2/sdl2.mk

diff --git a/package/Config.in b/package/Config.in
index fe3d3d0..ad4d248 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -215,6 +215,7 @@ endif
 	source "package/psplash/Config.in"
 	source "package/sawman/Config.in"
 	source "package/sdl/Config.in"
+	source "package/sdl2/Config.in"
 	source "package/sdl_gfx/Config.in"
 	source "package/sdl_image/Config.in"
 	source "package/sdl_mixer/Config.in"
diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
new file mode 100644
index 0000000..5d0fe01
--- /dev/null
+++ b/package/sdl2/Config.in
@@ -0,0 +1,22 @@
+config BR2_PACKAGE_SDL2
+	bool "SDL2"
+	help
+	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
+	  programs portable low level access to a video framebuffer,
+	  audio output, mouse, and keyboard. It is not compatible with SDL1.
+
+	  http://www.libsdl.org/
+
+if BR2_PACKAGE_SDL2
+
+config BR2_PACKAGE_SDL2_DIRECTFB
+	bool "SDL2 DirectFB video driver"
+	depends on BR2_PACKAGE_DIRECTFB
+
+config BR2_PACKAGE_SDL2_X11
+	bool "SDL2 X11 video driver"
+	depends on BR2_PACKAGE_XORG7
+	select BR2_PACKAGE_XLIB_LIBX11
+	select BR2_PACKAGE_XLIB_LIBXEXT
+
+endif
diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
new file mode 100644
index 0000000..b9aa907
--- /dev/null
+++ b/package/sdl2/sdl2.mk
@@ -0,0 +1,67 @@
+################################################################################
+#
+# sdl2
+#
+################################################################################
+
+SDL2_VERSION = 2.0.3
+SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
+SDL2_SITE = http://www.libsdl.org/release
+SDL2_LICENSE = zlib
+SDL2_LICENSE_FILES = COPYING
+SDL2_INSTALL_STAGING = YES
+
+# We must enable static build to get compilation successful.
+SDL2_CONF_OPTS = --enable-static
+
+ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
+SDL2_DEPENDENCIES += directfb
+SDL2_CONF_OPTS += --enable-video-directfb=yes
+SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
+else
+SDL2_CONF_OPTS += --enable-video-directfb=no
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_X11),y)
+SDL2_CONF_OPTS += --enable-video-x11=yes
+SDL2_DEPENDENCIES += \
+	xlib_libX11 xlib_libXext \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER), xlib_libXrender) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR), xlib_libXrandr)
+else
+SDL2_CONF_OPTS += --enable-video-x11=no
+endif
+
+ifeq ($(BR2_PACKAGE_TSLIB),y)
+SDL2_DEPENDENCIES += tslib
+endif
+
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+SDL2_DEPENDENCIES += alsa-lib
+endif
+
+ifeq ($(BR2_PACKAGE_MESA3D),y)
+SDL_DEPENDENCIES += mesa3d
+endif
+
+SDL2_CONF_OPTS += \
+	--enable-pulseaudio=no \
+	--disable-arts \
+	--disable-esd
+
+HOST_SDL2_CONF_OPTS += \
+	--enable-pulseaudio=no \
+	--enable-video-x11=no \
+	--disable-arts \
+	--disable-esd
+
+# Remove the -Wl,-rpath option.
+define SDL2_FIXUP_SDL2_CONFIG
+	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
+		$(STAGING_DIR)/usr/bin/sdl2-config
+endef
+
+SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
+
+$(eval $(autotools-package))
+$(eval $(host-autotools-package))
-- 
1.8.4.5

[Buildroot] [PATCH] sdl2: new package

[Guillaume GARDET - Oliséo]

Ping...

Guillaume


Le 26/02/2015 10:47, Guillaume GARDET a ?crit :
> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
>
> ---
>   package/Config.in      |  1 +
>   package/sdl2/Config.in | 22 +++++++++++++++++
>   package/sdl2/sdl2.mk   | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 90 insertions(+)
>   create mode 100644 package/sdl2/Config.in
>   create mode 100644 package/sdl2/sdl2.mk
>
> diff --git a/package/Config.in b/package/Config.in
> index fe3d3d0..ad4d248 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -215,6 +215,7 @@ endif
>   	source "package/psplash/Config.in"
>   	source "package/sawman/Config.in"
>   	source "package/sdl/Config.in"
> +	source "package/sdl2/Config.in"
>   	source "package/sdl_gfx/Config.in"
>   	source "package/sdl_image/Config.in"
>   	source "package/sdl_mixer/Config.in"
> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> new file mode 100644
> index 0000000..5d0fe01
> --- /dev/null
> +++ b/package/sdl2/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_SDL2
> +	bool "SDL2"
> +	help
> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
> +	  programs portable low level access to a video framebuffer,
> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
> +
> +	  http://www.libsdl.org/
> +
> +if BR2_PACKAGE_SDL2
> +
> +config BR2_PACKAGE_SDL2_DIRECTFB
> +	bool "SDL2 DirectFB video driver"
> +	depends on BR2_PACKAGE_DIRECTFB
> +
> +config BR2_PACKAGE_SDL2_X11
> +	bool "SDL2 X11 video driver"
> +	depends on BR2_PACKAGE_XORG7
> +	select BR2_PACKAGE_XLIB_LIBX11
> +	select BR2_PACKAGE_XLIB_LIBXEXT
> +
> +endif
> diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
> new file mode 100644
> index 0000000..b9aa907
> --- /dev/null
> +++ b/package/sdl2/sdl2.mk
> @@ -0,0 +1,67 @@
> +################################################################################
> +#
> +# sdl2
> +#
> +################################################################################
> +
> +SDL2_VERSION = 2.0.3
> +SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
> +SDL2_SITE = http://www.libsdl.org/release
> +SDL2_LICENSE = zlib
> +SDL2_LICENSE_FILES = COPYING
> +SDL2_INSTALL_STAGING = YES
> +
> +# We must enable static build to get compilation successful.
> +SDL2_CONF_OPTS = --enable-static
> +
> +ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
> +SDL2_DEPENDENCIES += directfb
> +SDL2_CONF_OPTS += --enable-video-directfb=yes
> +SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
> +else
> +SDL2_CONF_OPTS += --enable-video-directfb=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SDL2_X11),y)
> +SDL2_CONF_OPTS += --enable-video-x11=yes
> +SDL2_DEPENDENCIES += \
> +	xlib_libX11 xlib_libXext \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER), xlib_libXrender) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR), xlib_libXrandr)
> +else
> +SDL2_CONF_OPTS += --enable-video-x11=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_TSLIB),y)
> +SDL2_DEPENDENCIES += tslib
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
> +SDL2_DEPENDENCIES += alsa-lib
> +endif
> +
> +ifeq ($(BR2_PACKAGE_MESA3D),y)
> +SDL_DEPENDENCIES += mesa3d
> +endif
> +
> +SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \
> +	--disable-arts \
> +	--disable-esd
> +
> +HOST_SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \
> +	--enable-video-x11=no \
> +	--disable-arts \
> +	--disable-esd
> +
> +# Remove the -Wl,-rpath option.
> +define SDL2_FIXUP_SDL2_CONFIG
> +	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
> +		$(STAGING_DIR)/usr/bin/sdl2-config
> +endef
> +
> +SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
> +
> +$(eval $(autotools-package))
> +$(eval $(host-autotools-package))

[Buildroot] [PATCH] sdl2: new package

[Thomas Petazzoni]

Dear Guillaume GARDET,

Sorry for not replying earlier. I had a look at your patch some time
ago and had some comments, but apparently never replied.

On Thu, 26 Feb 2015 10:47:51 +0100, Guillaume GARDET wrote:

> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> new file mode 100644
> index 0000000..5d0fe01
> --- /dev/null
> +++ b/package/sdl2/Config.in
> @@ -0,0 +1,22 @@
> +config BR2_PACKAGE_SDL2
> +	bool "SDL2"

I know it's not the case for the existing SDL package, but we generally
prefer to have package names in lower-case in menuconfig, for
consistency. So s/SDL2/sdl2/ here.

> +	help
> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
> +	  programs portable low level access to a video framebuffer,
> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
> +
> +	  http://www.libsdl.org/

Did you check with very basic toolchains that SDL2 builds fine? I'm
surprised you have really zero dependency on toolchain features. Can
you check for example with the following configurations:

  http://autobuild.buildroot.org/toolchains/configs/br-arm-basic.config
  http://autobuild.buildroot.org/toolchains/configs/br-arm-full-nothread.config
  http://autobuild.buildroot.org/toolchains/configs/bfin-linux-uclibc.config
  http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config

You may discover that sdl2 depends on certain toolchain options.

> +config BR2_PACKAGE_SDL2_DIRECTFB
> +	bool "SDL2 DirectFB video driver"
> +	depends on BR2_PACKAGE_DIRECTFB

No need to repeat "SDL2" here, since this option is indented below the
"sdl2" main option.

> diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
> new file mode 100644
> index 0000000..b9aa907
> --- /dev/null
> +++ b/package/sdl2/sdl2.mk
> @@ -0,0 +1,67 @@
> +################################################################################
> +#
> +# sdl2
> +#
> +################################################################################
> +
> +SDL2_VERSION = 2.0.3
> +SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
> +SDL2_SITE = http://www.libsdl.org/release
> +SDL2_LICENSE = zlib
> +SDL2_LICENSE_FILES = COPYING
> +SDL2_INSTALL_STAGING = YES
> +
> +# We must enable static build to get compilation successful.
> +SDL2_CONF_OPTS = --enable-static

Hum, this is a bit annoying, but OK.

> +
> +ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
> +SDL2_DEPENDENCIES += directfb
> +SDL2_CONF_OPTS += --enable-video-directfb=yes
> +SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
> +else
> +SDL2_CONF_OPTS += --enable-video-directfb=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SDL2_X11),y)
> +SDL2_CONF_OPTS += --enable-video-x11=yes
> +SDL2_DEPENDENCIES += \
> +	xlib_libX11 xlib_libXext \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER), xlib_libXrender) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR), xlib_libXrandr)

No space after the comma.

> +else
> +SDL2_CONF_OPTS += --enable-video-x11=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_TSLIB),y)
> +SDL2_DEPENDENCIES += tslib
> +endif

Please explicit the --enable-input-tslib / --disable-input-tslib.

> +ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
> +SDL2_DEPENDENCIES += alsa-lib
> +endif

Same: --enable-alsa / --disable-alsa.

> +
> +ifeq ($(BR2_PACKAGE_MESA3D),y)
> +SDL_DEPENDENCIES += mesa3d
> +endif

What feature is this supposed to be enabling? mesa3d is an OpenGL
implementation, but it's not the only one. So if it's OpenGL related,
most likely this is not the right thing to do.

> +SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \
> +	--disable-arts \
> +	--disable-esd

Please put this earlier, where the first non-conditional SDL2_CONF_OPTS
definition is.

> +HOST_SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \
> +	--enable-video-x11=no \
> +	--disable-arts \
> +	--disable-esd

Not needed I believe.

> +
> +# Remove the -Wl,-rpath option.
> +define SDL2_FIXUP_SDL2_CONFIG
> +	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
> +		$(STAGING_DIR)/usr/bin/sdl2-config
> +endef
> +
> +SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
> +
> +$(eval $(autotools-package))
> +$(eval $(host-autotools-package))

Why do you have a host package? There is nothing that depends on it.

Could you look at those comments, and resubmit an updated version
and/or comment on them as needed?

Thanks a lot!

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

[Buildroot] [PATCH] sdl2: new package

[Guillaume GARDET - Oliséo]

Hi,

Le 13/03/2015 17:14, Thomas Petazzoni a ?crit :
> Dear Guillaume GARDET,
>
> Sorry for not replying earlier. I had a look at your patch some time
> ago and had some comments, but apparently never replied.
>
> On Thu, 26 Feb 2015 10:47:51 +0100, Guillaume GARDET wrote:
>
>> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
>> new file mode 100644
>> index 0000000..5d0fe01
>> --- /dev/null
>> +++ b/package/sdl2/Config.in
>> @@ -0,0 +1,22 @@
>> +config BR2_PACKAGE_SDL2
>> +	bool "SDL2"
> I know it's not the case for the existing SDL package, but we generally
> prefer to have package names in lower-case in menuconfig, for
> consistency. So s/SDL2/sdl2/ here.

ok

>
>> +	help
>> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
>> +	  programs portable low level access to a video framebuffer,
>> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
>> +
>> +	  http://www.libsdl.org/
> Did you check with very basic toolchains that SDL2 builds fine? I'm
> surprised you have really zero dependency on toolchain features. Can
> you check for example with the following configurations:
>
>    http://autobuild.buildroot.org/toolchains/configs/br-arm-basic.config
>    http://autobuild.buildroot.org/toolchains/configs/br-arm-full-nothread.config
>    http://autobuild.buildroot.org/toolchains/configs/bfin-linux-uclibc.config
>    http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config
>
> You may discover that sdl2 depends on certain toolchain options.

I used sdl (1) as a template which has no wuch deps, but I will test.

>
>> +config BR2_PACKAGE_SDL2_DIRECTFB
>> +	bool "SDL2 DirectFB video driver"
>> +	depends on BR2_PACKAGE_DIRECTFB
> No need to repeat "SDL2" here, since this option is indented below the
> "sdl2" main option.

ok

>
>> diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
>> new file mode 100644
>> index 0000000..b9aa907
>> --- /dev/null
>> +++ b/package/sdl2/sdl2.mk
>> @@ -0,0 +1,67 @@
>> +################################################################################
>> +#
>> +# sdl2
>> +#
>> +################################################################################
>> +
>> +SDL2_VERSION = 2.0.3
>> +SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
>> +SDL2_SITE = http://www.libsdl.org/release
>> +SDL2_LICENSE = zlib
>> +SDL2_LICENSE_FILES = COPYING
>> +SDL2_INSTALL_STAGING = YES
>> +
>> +# We must enable static build to get compilation successful.
>> +SDL2_CONF_OPTS = --enable-static
> Hum, this is a bit annoying, but OK.
>
>> +
>> +ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
>> +SDL2_DEPENDENCIES += directfb
>> +SDL2_CONF_OPTS += --enable-video-directfb=yes
>> +SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
>> +else
>> +SDL2_CONF_OPTS += --enable-video-directfb=no
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_SDL2_X11),y)
>> +SDL2_CONF_OPTS += --enable-video-x11=yes
>> +SDL2_DEPENDENCIES += \
>> +	xlib_libX11 xlib_libXext \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER), xlib_libXrender) \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR), xlib_libXrandr)
> No space after the comma.

ok

>
>> +else
>> +SDL2_CONF_OPTS += --enable-video-x11=no
>> +endif
>> +
>> +ifeq ($(BR2_PACKAGE_TSLIB),y)
>> +SDL2_DEPENDENCIES += tslib
>> +endif
> Please explicit the --enable-input-tslib / --disable-input-tslib.

ok

>
>> +ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
>> +SDL2_DEPENDENCIES += alsa-lib
>> +endif
> Same: --enable-alsa / --disable-alsa.

ok

>
>> +
>> +ifeq ($(BR2_PACKAGE_MESA3D),y)
>> +SDL_DEPENDENCIES += mesa3d
>> +endif
> What feature is this supposed to be enabling? mesa3d is an OpenGL
> implementation, but it's not the only one. So if it's OpenGL related,
> most likely this is not the right thing to do.

Here, I also used sdl (1) package as a template. So, sdl (1) should be fixed too.

>
>> +SDL2_CONF_OPTS += \
>> +	--enable-pulseaudio=no \
>> +	--disable-arts \
>> +	--disable-esd
> Please put this earlier, where the first non-conditional SDL2_CONF_OPTS
> definition is.

ok

>
>> +HOST_SDL2_CONF_OPTS += \
>> +	--enable-pulseaudio=no \
>> +	--enable-video-x11=no \
>> +	--disable-arts \
>> +	--disable-esd
> Not needed I believe.

ok. sdl1 has it too.

>
>> +
>> +# Remove the -Wl,-rpath option.
>> +define SDL2_FIXUP_SDL2_CONFIG
>> +	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
>> +		$(STAGING_DIR)/usr/bin/sdl2-config
>> +endef
>> +
>> +SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
>> +
>> +$(eval $(autotools-package))
>> +$(eval $(host-autotools-package))
> Why do you have a host package? There is nothing that depends on it.

Indeed. I used sdl1 has a template as said earlier. :(

>
> Could you look at those comments, and resubmit an updated version
> and/or comment on them as needed?

Will send a V2.


Guillaume

[Buildroot] [PATCH] sdl2: new package

[Guillaume GARDET - Oliséo]

Hi,

Le 13/03/2015 17:14, Thomas Petazzoni a ?crit :
>> +	help
>> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
>> +	  programs portable low level access to a video framebuffer,
>> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
>> +
>> +	  http://www.libsdl.org/
> Did you check with very basic toolchains that SDL2 builds fine? I'm
> surprised you have really zero dependency on toolchain features. Can
> you check for example with the following configurations:
>
>    http://autobuild.buildroot.org/toolchains/configs/br-arm-basic.config
>    http://autobuild.buildroot.org/toolchains/configs/br-arm-full-nothread.config
>    http://autobuild.buildroot.org/toolchains/configs/bfin-linux-uclibc.config

The 3 are ok.

>    http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config

This one fails with:
"dlfcn.h: No such file or directory"

WHat would be the right "dpends on" option?


Guillaume

[Buildroot] [PATCH] sdl2: new package

[Thomas Petazzoni]

Dear Guillaume GARDET - Olis?o,

On Mon, 30 Mar 2015 14:50:55 +0200, Guillaume GARDET - Olis?o wrote:

> >    http://autobuild.buildroot.org/toolchains/configs/bfin-uclinux.config
> 
> This one fails with:
> "dlfcn.h: No such file or directory"
> 
> WHat would be the right "dpends on" option?

depends on !BR2_STATIC_LIBS

and the appropriate comment. See section "17.2.2. Dependencies on
target and toolchain options" in the Buildroot manual.

Thanks,

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

[Buildroot] [PATCH V2] sdl2: new package

[Guillaume GARDET]

Changes in V2:
* Fix SDL2 case: s/SDL2/sdl2/
* Add dependency on toolchain with dynamic library
* Remove unneeded 'SDL2' in sub-options names
* Remove unneeded space after comma in if statement
* Add explicit enable/disable options for tslib and alsa
* Remove Mesa3D optional part
* Move unconditionnal SDL2_CONF_OPTS upper
* Remove host build

Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

---
 package/Config.in      |  1 +
 package/sdl2/Config.in | 26 +++++++++++++++++++++
 package/sdl2/sdl2.mk   | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 package/sdl2/Config.in
 create mode 100644 package/sdl2/sdl2.mk

diff --git a/package/Config.in b/package/Config.in
index 566a78a..cf4949a 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -219,6 +219,7 @@ endif
 	source "package/psplash/Config.in"
 	source "package/sawman/Config.in"
 	source "package/sdl/Config.in"
+	source "package/sdl2/Config.in"
 	source "package/sdl_gfx/Config.in"
 	source "package/sdl_image/Config.in"
 	source "package/sdl_mixer/Config.in"
diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
new file mode 100644
index 0000000..18e4f75
--- /dev/null
+++ b/package/sdl2/Config.in
@@ -0,0 +1,26 @@
+config BR2_PACKAGE_SDL2
+	depends on !BR2_STATIC_LIBS
+	bool "sdl2"
+	help
+	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
+	  programs portable low level access to a video framebuffer,
+	  audio output, mouse, and keyboard. It is not compatible with SDL1.
+
+	  http://www.libsdl.org/
+
+comment "sdl2 needs a toolchain w/ dynamic library"
+        depends on BR2_STATIC_LIBS
+
+if BR2_PACKAGE_SDL2
+
+config BR2_PACKAGE_SDL2_DIRECTFB
+	bool "DirectFB video driver"
+	depends on BR2_PACKAGE_DIRECTFB
+
+config BR2_PACKAGE_SDL2_X11
+	bool "X11 video driver"
+	depends on BR2_PACKAGE_XORG7
+	select BR2_PACKAGE_XLIB_LIBX11
+	select BR2_PACKAGE_XLIB_LIBXEXT
+
+endif
diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
new file mode 100644
index 0000000..6a3caaf
--- /dev/null
+++ b/package/sdl2/sdl2.mk
@@ -0,0 +1,62 @@
+################################################################################
+#
+# sdl2
+#
+################################################################################
+
+SDL2_VERSION = 2.0.3
+SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
+SDL2_SITE = http://www.libsdl.org/release
+SDL2_LICENSE = zlib
+SDL2_LICENSE_FILES = COPYING
+SDL2_INSTALL_STAGING = YES
+
+SDL2_CONF_OPTS += \
+	--enable-pulseaudio=no \
+	--disable-arts \
+	--disable-esd
+
+# We must enable static build to get compilation succesful.
+SDL2_CONF_OPTS += --enable-static
+
+ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
+SDL2_DEPENDENCIES += directfb
+SDL2_CONF_OPTS += --enable-video-directfb=yes
+SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
+else
+SDL2_CONF_OPTS += --enable-video-directfb=no
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_X11),y)
+SDL2_CONF_OPTS += --enable-video-x11=yes
+SDL2_DEPENDENCIES += \
+	xlib_libX11 xlib_libXext \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr)
+else
+SDL2_CONF_OPTS += --enable-video-x11=no
+endif
+
+ifeq ($(BR2_PACKAGE_TSLIB),y)
+SDL2_DEPENDENCIES += tslib
+SDL2_CONF_OPTS += --enable-input-tslib
+else
+SDL2_CONF_OPTS += --disable-input-tslib
+endif
+
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+SDL2_DEPENDENCIES += alsa-lib
+SDL2_CONF_OPTS += --enable-alsa
+else
+SDL2_CONF_OPTS += --disable-alsa
+endif
+
+# Remove the -Wl,-rpath option.
+define SDL2_FIXUP_SDL2_CONFIG
+	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
+		$(STAGING_DIR)/usr/bin/sdl2-config
+endef
+
+SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
+
+$(eval $(autotools-package))
-- 
1.8.4.5

[Buildroot] [PATCH V2] sdl2: new package

[Guillaume GARDET - Oliséo]

Ping.

Guillaume


Le 30/03/2015 15:14, Guillaume GARDET a ?crit :
> Changes in V2:
> * Fix SDL2 case: s/SDL2/sdl2/
> * Add dependency on toolchain with dynamic library
> * Remove unneeded 'SDL2' in sub-options names
> * Remove unneeded space after comma in if statement
> * Add explicit enable/disable options for tslib and alsa
> * Remove Mesa3D optional part
> * Move unconditionnal SDL2_CONF_OPTS upper
> * Remove host build
>
> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>
> ---
>   package/Config.in      |  1 +
>   package/sdl2/Config.in | 26 +++++++++++++++++++++
>   package/sdl2/sdl2.mk   | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 89 insertions(+)
>   create mode 100644 package/sdl2/Config.in
>   create mode 100644 package/sdl2/sdl2.mk
>
> diff --git a/package/Config.in b/package/Config.in
> index 566a78a..cf4949a 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -219,6 +219,7 @@ endif
>   	source "package/psplash/Config.in"
>   	source "package/sawman/Config.in"
>   	source "package/sdl/Config.in"
> +	source "package/sdl2/Config.in"
>   	source "package/sdl_gfx/Config.in"
>   	source "package/sdl_image/Config.in"
>   	source "package/sdl_mixer/Config.in"
> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> new file mode 100644
> index 0000000..18e4f75
> --- /dev/null
> +++ b/package/sdl2/Config.in
> @@ -0,0 +1,26 @@
> +config BR2_PACKAGE_SDL2
> +	depends on !BR2_STATIC_LIBS
> +	bool "sdl2"
> +	help
> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
> +	  programs portable low level access to a video framebuffer,
> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
> +
> +	  http://www.libsdl.org/
> +
> +comment "sdl2 needs a toolchain w/ dynamic library"
> +        depends on BR2_STATIC_LIBS
> +
> +if BR2_PACKAGE_SDL2
> +
> +config BR2_PACKAGE_SDL2_DIRECTFB
> +	bool "DirectFB video driver"
> +	depends on BR2_PACKAGE_DIRECTFB
> +
> +config BR2_PACKAGE_SDL2_X11
> +	bool "X11 video driver"
> +	depends on BR2_PACKAGE_XORG7
> +	select BR2_PACKAGE_XLIB_LIBX11
> +	select BR2_PACKAGE_XLIB_LIBXEXT
> +
> +endif
> diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
> new file mode 100644
> index 0000000..6a3caaf
> --- /dev/null
> +++ b/package/sdl2/sdl2.mk
> @@ -0,0 +1,62 @@
> +################################################################################
> +#
> +# sdl2
> +#
> +################################################################################
> +
> +SDL2_VERSION = 2.0.3
> +SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
> +SDL2_SITE = http://www.libsdl.org/release
> +SDL2_LICENSE = zlib
> +SDL2_LICENSE_FILES = COPYING
> +SDL2_INSTALL_STAGING = YES
> +
> +SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \
> +	--disable-arts \
> +	--disable-esd
> +
> +# We must enable static build to get compilation succesful.
> +SDL2_CONF_OPTS += --enable-static
> +
> +ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
> +SDL2_DEPENDENCIES += directfb
> +SDL2_CONF_OPTS += --enable-video-directfb=yes
> +SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
> +else
> +SDL2_CONF_OPTS += --enable-video-directfb=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SDL2_X11),y)
> +SDL2_CONF_OPTS += --enable-video-x11=yes
> +SDL2_DEPENDENCIES += \
> +	xlib_libX11 xlib_libXext \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr)
> +else
> +SDL2_CONF_OPTS += --enable-video-x11=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_TSLIB),y)
> +SDL2_DEPENDENCIES += tslib
> +SDL2_CONF_OPTS += --enable-input-tslib
> +else
> +SDL2_CONF_OPTS += --disable-input-tslib
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
> +SDL2_DEPENDENCIES += alsa-lib
> +SDL2_CONF_OPTS += --enable-alsa
> +else
> +SDL2_CONF_OPTS += --disable-alsa
> +endif
> +
> +# Remove the -Wl,-rpath option.
> +define SDL2_FIXUP_SDL2_CONFIG
> +	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
> +		$(STAGING_DIR)/usr/bin/sdl2-config
> +endef
> +
> +SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
> +
> +$(eval $(autotools-package))

[Buildroot] [PATCH V2] sdl2: new package

[Romain Naour]

Hi Guillaume,

Thanks for your patch, I have some comments

Le 30/03/2015 15:14, Guillaume GARDET a ?crit :
> Changes in V2:
> * Fix SDL2 case: s/SDL2/sdl2/
> * Add dependency on toolchain with dynamic library
> * Remove unneeded 'SDL2' in sub-options names
> * Remove unneeded space after comma in if statement
> * Add explicit enable/disable options for tslib and alsa
> * Remove Mesa3D optional part
> * Move unconditionnal SDL2_CONF_OPTS upper
> * Remove host build
> 
The history of the patch changes must be bellow "---" line ...

> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> 
> ---
here:
Changes in V2:

>  package/Config.in      |  1 +
>  package/sdl2/Config.in | 26 +++++++++++++++++++++
>  package/sdl2/sdl2.mk   | 62 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 89 insertions(+)
>  create mode 100644 package/sdl2/Config.in
>  create mode 100644 package/sdl2/sdl2.mk
> 
> diff --git a/package/Config.in b/package/Config.in
> index 566a78a..cf4949a 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -219,6 +219,7 @@ endif
>  	source "package/psplash/Config.in"
>  	source "package/sawman/Config.in"
>  	source "package/sdl/Config.in"
> +	source "package/sdl2/Config.in"
>  	source "package/sdl_gfx/Config.in"
>  	source "package/sdl_image/Config.in"
>  	source "package/sdl_mixer/Config.in"
> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> new file mode 100644
> index 0000000..18e4f75
> --- /dev/null
> +++ b/package/sdl2/Config.in
> @@ -0,0 +1,26 @@
> +config BR2_PACKAGE_SDL2
> +	depends on !BR2_STATIC_LIBS
> +	bool "sdl2"
> +	help
> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
> +	  programs portable low level access to a video framebuffer,
> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
> +
> +	  http://www.libsdl.org/
> +
> +comment "sdl2 needs a toolchain w/ dynamic library"
> +        depends on BR2_STATIC_LIBS
> +
> +if BR2_PACKAGE_SDL2
> +
> +config BR2_PACKAGE_SDL2_DIRECTFB
> +	bool "DirectFB video driver"
> +	depends on BR2_PACKAGE_DIRECTFB

You can add a comment here to say that directfb support needs directfb package
to be enabled.

comment "DirectFB video driver needs directfb"
        depends on !BR2_PACKAGE_DIRECTFB

> +
> +config BR2_PACKAGE_SDL2_X11
> +	bool "X11 video driver"
> +	depends on BR2_PACKAGE_XORG7
> +	select BR2_PACKAGE_XLIB_LIBX11
> +	select BR2_PACKAGE_XLIB_LIBXEXT

Same for X11

> +
> +endif
> diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
> new file mode 100644
> index 0000000..6a3caaf
> --- /dev/null
> +++ b/package/sdl2/sdl2.mk
> @@ -0,0 +1,62 @@
> +################################################################################
> +#
> +# sdl2
> +#
> +################################################################################
> +
> +SDL2_VERSION = 2.0.3
> +SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
> +SDL2_SITE = http://www.libsdl.org/release
> +SDL2_LICENSE = zlib
> +SDL2_LICENSE_FILES = COPYING

It's COPYING.txt

Can you check the libudev dependency since libudev support is set by default ?

> +SDL2_INSTALL_STAGING = YES
> +
> +SDL2_CONF_OPTS += \
> +	--enable-pulseaudio=no \

Why do you disable pulseaudio support ? we have a pulsaudio package for the
target. Otherwise, ok the pulseaudio support can be added in a follow up patch.

> +	--disable-arts \
> +	--disable-esd
> +
> +# We must enable static build to get compilation succesful.

s/succesful/successful/

I used this patch to enable sdl support in the efl package bumped to 1.14-beta1
(patch series to be sent). At least it build fine with your sdl2 package, but I
need to do some runtime testing.

> +SDL2_CONF_OPTS += --enable-static
> +
> +ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
> +SDL2_DEPENDENCIES += directfb
> +SDL2_CONF_OPTS += --enable-video-directfb=yes
> +SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
> +else
> +SDL2_CONF_OPTS += --enable-video-directfb=no
> +endif
> +
> +ifeq ($(BR2_PACKAGE_SDL2_X11),y)
> +SDL2_CONF_OPTS += --enable-video-x11=yes
> +SDL2_DEPENDENCIES += \
> +	xlib_libX11 xlib_libXext \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr)

Some X11 dependencies are missing:
x11-xcursor
x11-xinerama
x11-xinput
x11-scrnsaver
x11-xshape (I don't think we have a package for it in Buildroot)
> +else
> +SDL2_CONF_OPTS += --enable-video-x11=no
> +endif

Also, sdl2 may depends on virtual package libgl or libgles in order to enable
video-opengl and video-opengles features.

If you don't want to enable these options right now, you need to disable them
explicitly since they are enabled by default. Doing this avoid issues with build
dependency order.

> +
> +ifeq ($(BR2_PACKAGE_TSLIB),y)
> +SDL2_DEPENDENCIES += tslib
> +SDL2_CONF_OPTS += --enable-input-tslib
> +else
> +SDL2_CONF_OPTS += --disable-input-tslib
> +endif
> +
> +ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
> +SDL2_DEPENDENCIES += alsa-lib
> +SDL2_CONF_OPTS += --enable-alsa
> +else
> +SDL2_CONF_OPTS += --disable-alsa
> +endif
> +
> +# Remove the -Wl,-rpath option.
> +define SDL2_FIXUP_SDL2_CONFIG
> +	$(SED) 's%-Wl,-rpath,\$${libdir}%%' \
> +		$(STAGING_DIR)/usr/bin/sdl2-config
> +endef

Did you try to use --disable-rpath instead ?

> +
> +SDL2_POST_INSTALL_STAGING_HOOKS += SDL2_FIXUP_SDL2_CONFIG
> +
> +$(eval $(autotools-package))
> 

Thanks,
Romain Naour

[Buildroot] [PATCH V2] sdl2: new package

[Bernd Kuhls]

Guillaume GARDET <guillaume.gardet@oliseo.fr> 
wrote in news:1427721269-8267-1-git-send-email-guillaume.gardet at oliseo.fr:

> Changes in V2:

Hi,

I used your patch to enable sdl2 support in the fs-uae (http://fs-uae.net) 
package I am currently coding (patch series to be sent). At least it build 
fine with your sdl2 package, but I need to do some runtime testing.

Regards, Bernd

[Buildroot] [PATCH V3] sdl2: new package

[Guillaume GARDET]

Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Romain Naour <romain.naour@openwide.fr>

---

Changes in V3:
* Add dependenies comments for DirectFB and X11 options
* Fix license filename
* Add explicit enable/disable options for libudev
* Fix typo: s/succesful/successful/
* Add more X11 dependencies: x11-xcursor, x11-xinerama, x11-xinput, x11-scrnsaver
* Disbale opengl/opengles video diver explicitly
* Use --disable-rpath option option instead of manual fix

Changes in V2:
* Fix SDL2 case: s/SDL2/sdl2/
* Add dependency on toolchain with dynamic library
* Remove unneeded 'SDL2' in sub-options names
* Remove unneeded space after comma in if statement
* Add explicit enable/disable options for tslib and alsa
* Remove Mesa3D optional part
* Move unconditionnal SDL2_CONF_OPTS upper
* Remove host build


 package/Config.in      |  1 +
 package/sdl2/Config.in | 32 ++++++++++++++++++++++++
 package/sdl2/sdl2.mk   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 package/sdl2/Config.in
 create mode 100644 package/sdl2/sdl2.mk

diff --git a/package/Config.in b/package/Config.in
index c2f6524..f16a114 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -245,6 +245,7 @@ endif
 	source "package/psplash/Config.in"
 	source "package/sawman/Config.in"
 	source "package/sdl/Config.in"
+	source "package/sdl2/Config.in"
 	source "package/sdl_gfx/Config.in"
 	source "package/sdl_image/Config.in"
 	source "package/sdl_mixer/Config.in"
diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
new file mode 100644
index 0000000..c3f61f5
--- /dev/null
+++ b/package/sdl2/Config.in
@@ -0,0 +1,32 @@
+config BR2_PACKAGE_SDL2
+	depends on !BR2_STATIC_LIBS
+	bool "sdl2"
+	help
+	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
+	  programs portable low level access to a video framebuffer,
+	  audio output, mouse, and keyboard. It is not compatible with SDL1.
+
+	  http://www.libsdl.org/
+
+comment "sdl2 needs a toolchain w/ dynamic library"
+        depends on BR2_STATIC_LIBS
+
+if BR2_PACKAGE_SDL2
+
+config BR2_PACKAGE_SDL2_DIRECTFB
+	bool "DirectFB video driver"
+	depends on BR2_PACKAGE_DIRECTFB
+
+comment "DirectFB video driver needs directfb"
+        depends on !BR2_PACKAGE_DIRECTFB
+
+config BR2_PACKAGE_SDL2_X11
+	bool "X11 video driver"
+	depends on BR2_PACKAGE_XORG7
+	select BR2_PACKAGE_XLIB_LIBX11
+	select BR2_PACKAGE_XLIB_LIBXEXT
+
+comment "X11 video driver needs X.org"
+        depends on !BR2_PACKAGE_XORG7
+
+endif
diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
new file mode 100644
index 0000000..424cfcb
--- /dev/null
+++ b/package/sdl2/sdl2.mk
@@ -0,0 +1,68 @@
+################################################################################
+#
+# sdl2
+#
+################################################################################
+
+SDL2_VERSION = 2.0.3
+SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
+SDL2_SITE = http://www.libsdl.org/release
+SDL2_LICENSE = zlib
+SDL2_LICENSE_FILES = COPYING.txt
+SDL2_INSTALL_STAGING = YES
+
+SDL2_CONF_OPTS += \
+	--disable-rpath \
+	--disable-arts \
+	--disable-esd \
+	--disable-pulseaudio \
+	--disable-video-opengl \
+	--disable-video-opengles
+
+# We must enable static build to get compilation successful.
+SDL2_CONF_OPTS += --enable-static
+
+ifeq ($(BR2_PACKAGE_HAS_UDEV),y)
+SDL2__DEPENDENCIES += udev
+SDL2__CONF_OPTS += --enable-libudev
+else
+SDL2_CONF_OPTS += --disable-libudev
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
+SDL2_DEPENDENCIES += directfb
+SDL2_CONF_OPTS += --enable-video-directfb=yes
+SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
+else
+SDL2_CONF_OPTS += --enable-video-directfb=no
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_X11),y)
+SDL2_CONF_OPTS += --enable-video-x11=yes --with-x
+SDL2_DEPENDENCIES += \
+	xlib_libX11 xlib_libXext \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR,xlib_libXcursor) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA,xlib_libXinerama) \
+	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO,xproto_inputproto) \
+	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO,xproto_scrnsaverproto)
+else
+SDL2_CONF_OPTS += --enable-video-x11=no --without-x
+endif
+
+ifeq ($(BR2_PACKAGE_TSLIB),y)
+SDL2_DEPENDENCIES += tslib
+SDL2_CONF_OPTS += --enable-input-tslib
+else
+SDL2_CONF_OPTS += --disable-input-tslib
+endif
+
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+SDL2_DEPENDENCIES += alsa-lib
+SDL2_CONF_OPTS += --enable-alsa
+else
+SDL2_CONF_OPTS += --disable-alsa
+endif
+
+$(eval $(autotools-package))
-- 
1.8.4.5

[Buildroot] [PATCH V3] sdl2: new package

[Yann E. MORIN]

Guillaume, All,

On 2015-06-29 22:30 +0200, Guillaume GARDET spake thusly:
> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Romain Naour <romain.naour@openwide.fr>
[--SNIP--]
> diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
> new file mode 100644
> index 0000000..c3f61f5
> --- /dev/null
> +++ b/package/sdl2/Config.in
> @@ -0,0 +1,32 @@
> +config BR2_PACKAGE_SDL2
> +	depends on !BR2_STATIC_LIBS
> +	bool "sdl2"
> +	help
> +	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
> +	  programs portable low level access to a video framebuffer,
> +	  audio output, mouse, and keyboard. It is not compatible with SDL1.
> +
> +	  http://www.libsdl.org/
> +
> +comment "sdl2 needs a toolchain w/ dynamic library"
> +        depends on BR2_STATIC_LIBS

Either you put that comment at the beginning of the file, or at the end,
not in the middle.

The reason is that Kconfig, when it detects a dependnecy chain between
two adjacent symbols, will render the second symbol indented below the
first, like so:

    config FOO
        bool "foo"

    config BAR
        boll "bar'
        depends on FOO

will be render in the menuconfig as:

    [*] foo
    [ ]   bar

To show that 'bar' is a sub-option of 'foo'.

If you put the comment in between FOO nad BAR, then you break the
"dependency chain" and the symbols will be rendred as:

    [*] foo
    [ ] bar

which is less than ideal.

[I haven't looked at the rest of the file, as I know about nothing about
sdl2]

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 V4] sdl2: new package

[Guillaume GARDET]

Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: Romain Naour <romain.naour@openwide.fr>
Cc: Yann E. Morin <yann.morin.1998@free.fr>

---

Changes in V4:
* Fix comments places in Config.in

Changes in V3:
* Add dependenies comments for DirectFB and X11 options
* Fix license filename
* Add explicit enable/disable options for libudev
* Fix typo: s/succesful/successful/
* Add more X11 dependencies: x11-xcursor, x11-xinerama, x11-xinput, x11-scrnsaver
* Disbale opengl/opengles video diver explicitly
* Use --disable-rpath option option instead of manual fix

Changes in V2:
* Fix SDL2 case: s/SDL2/sdl2/
* Add dependency on toolchain with dynamic library
* Remove unneeded 'SDL2' in sub-options names
* Remove unneeded space after comma in if statement
* Add explicit enable/disable options for tslib and alsa
* Remove Mesa3D optional part
* Move unconditionnal SDL2_CONF_OPTS upper
* Remove host build


 package/Config.in      |  1 +
 package/sdl2/Config.in | 32 ++++++++++++++++++++++++
 package/sdl2/sdl2.mk   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+)
 create mode 100644 package/sdl2/Config.in
 create mode 100644 package/sdl2/sdl2.mk

diff --git a/package/Config.in b/package/Config.in
index 13a7e74..3a1b5c6 100644
--- a/package/Config.in
+++ b/package/Config.in
@@ -246,6 +246,7 @@ endif
 	source "package/psplash/Config.in"
 	source "package/sawman/Config.in"
 	source "package/sdl/Config.in"
+	source "package/sdl2/Config.in"
 	source "package/sdl_gfx/Config.in"
 	source "package/sdl_image/Config.in"
 	source "package/sdl_mixer/Config.in"
diff --git a/package/sdl2/Config.in b/package/sdl2/Config.in
new file mode 100644
index 0000000..fe30650
--- /dev/null
+++ b/package/sdl2/Config.in
@@ -0,0 +1,32 @@
+config BR2_PACKAGE_SDL2
+	depends on !BR2_STATIC_LIBS
+	bool "sdl2"
+	help
+	  Simple DirectMedia Layer 2 - SDL2 is a library that allows
+	  programs portable low level access to a video framebuffer,
+	  audio output, mouse, and keyboard. It is not compatible with SDL1.
+
+	  http://www.libsdl.org/
+
+if BR2_PACKAGE_SDL2
+
+config BR2_PACKAGE_SDL2_DIRECTFB
+	bool "DirectFB video driver"
+	depends on BR2_PACKAGE_DIRECTFB
+
+config BR2_PACKAGE_SDL2_X11
+	bool "X11 video driver"
+	depends on BR2_PACKAGE_XORG7
+	select BR2_PACKAGE_XLIB_LIBX11
+	select BR2_PACKAGE_XLIB_LIBXEXT
+
+comment "DirectFB video driver needs directfb"
+	depends on !BR2_PACKAGE_DIRECTFB
+
+comment "X11 video driver needs X.org"
+	depends on !BR2_PACKAGE_XORG7
+
+endif
+
+comment "sdl2 needs a toolchain w/ dynamic library"
+	depends on BR2_STATIC_LIBS
diff --git a/package/sdl2/sdl2.mk b/package/sdl2/sdl2.mk
new file mode 100644
index 0000000..424cfcb
--- /dev/null
+++ b/package/sdl2/sdl2.mk
@@ -0,0 +1,68 @@
+################################################################################
+#
+# sdl2
+#
+################################################################################
+
+SDL2_VERSION = 2.0.3
+SDL2_SOURCE = SDL2-$(SDL2_VERSION).tar.gz
+SDL2_SITE = http://www.libsdl.org/release
+SDL2_LICENSE = zlib
+SDL2_LICENSE_FILES = COPYING.txt
+SDL2_INSTALL_STAGING = YES
+
+SDL2_CONF_OPTS += \
+	--disable-rpath \
+	--disable-arts \
+	--disable-esd \
+	--disable-pulseaudio \
+	--disable-video-opengl \
+	--disable-video-opengles
+
+# We must enable static build to get compilation successful.
+SDL2_CONF_OPTS += --enable-static
+
+ifeq ($(BR2_PACKAGE_HAS_UDEV),y)
+SDL2__DEPENDENCIES += udev
+SDL2__CONF_OPTS += --enable-libudev
+else
+SDL2_CONF_OPTS += --disable-libudev
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_DIRECTFB),y)
+SDL2_DEPENDENCIES += directfb
+SDL2_CONF_OPTS += --enable-video-directfb=yes
+SDL2_CONF_ENV = ac_cv_path_DIRECTFBCONFIG=$(STAGING_DIR)/usr/bin/directfb-config
+else
+SDL2_CONF_OPTS += --enable-video-directfb=no
+endif
+
+ifeq ($(BR2_PACKAGE_SDL2_X11),y)
+SDL2_CONF_OPTS += --enable-video-x11=yes --with-x
+SDL2_DEPENDENCIES += \
+	xlib_libX11 xlib_libXext \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR,xlib_libXcursor) \
+	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA,xlib_libXinerama) \
+	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO,xproto_inputproto) \
+	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO,xproto_scrnsaverproto)
+else
+SDL2_CONF_OPTS += --enable-video-x11=no --without-x
+endif
+
+ifeq ($(BR2_PACKAGE_TSLIB),y)
+SDL2_DEPENDENCIES += tslib
+SDL2_CONF_OPTS += --enable-input-tslib
+else
+SDL2_CONF_OPTS += --disable-input-tslib
+endif
+
+ifeq ($(BR2_PACKAGE_ALSA_LIB),y)
+SDL2_DEPENDENCIES += alsa-lib
+SDL2_CONF_OPTS += --enable-alsa
+else
+SDL2_CONF_OPTS += --disable-alsa
+endif
+
+$(eval $(autotools-package))
-- 
1.8.4.5

[Buildroot] [PATCH V4] sdl2: new package

[Romain Naour]

Hi Guillaume,

Le 02/07/2015 10:16, Guillaume GARDET a ?crit :
> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Romain Naour <romain.naour@openwide.fr>
> Cc: Yann E. Morin <yann.morin.1998@free.fr>
> 
> ---
> 
> Changes in V4:
> * Fix comments places in Config.in
> 
> Changes in V3:
> * Add dependenies comments for DirectFB and X11 options
> * Fix license filename
> * Add explicit enable/disable options for libudev
> * Fix typo: s/succesful/successful/
> * Add more X11 dependencies: x11-xcursor, x11-xinerama, x11-xinput, x11-scrnsaver
> * Disbale opengl/opengles video diver explicitly
> * Use --disable-rpath option option instead of manual fix
> 
> Changes in V2:
> * Fix SDL2 case: s/SDL2/sdl2/
> * Add dependency on toolchain with dynamic library
> * Remove unneeded 'SDL2' in sub-options names
> * Remove unneeded space after comma in if statement
> * Add explicit enable/disable options for tslib and alsa
> * Remove Mesa3D optional part
> * Move unconditionnal SDL2_CONF_OPTS upper
> * Remove host build
> 
> 
>  package/Config.in      |  1 +
>  package/sdl2/Config.in | 32 ++++++++++++++++++++++++
>  package/sdl2/sdl2.mk   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 101 insertions(+)
>  create mode 100644 package/sdl2/Config.in
>  create mode 100644 package/sdl2/sdl2.mk
> 

[snip]

> +SDL2_DEPENDENCIES += \
> +	xlib_libX11 xlib_libXext \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR,xlib_libXcursor) \
> +	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA,xlib_libXinerama) \
> +	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO,xproto_inputproto) \
> +	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO,xproto_scrnsaverproto)

There are missing ',' and ')' here.

This part should look like:

SDL2_DEPENDENCIES += \
	xlib_libX11 xlib_libXext \
	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender,) \
	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr,) \
	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR),xlib_libXcursor,) \
	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA),xlib_libXinerama,) \
	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO),xproto_inputproto,) \
	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO),xproto_scrnsaverproto,)

sdl2 is seems complicated package, I think it would be easier to review if you
add it progressively by using a patch series:

PATCH1: sdl2 new package (all mandatory dependencies enabled)
PATCH2: sdl2 add X11 support
PATCH3: sdl2 add directfb support
...

I'll review your patch latter new week end.

Best regards,
Romain

[Buildroot] [PATCH V4] sdl2: new package

[Guillaume GARDET - Oliséo]

Hi,

Le 05/07/2015 20:21, Romain Naour a ?crit :
> Hi Guillaume,
>
> Le 02/07/2015 10:16, Guillaume GARDET a ?crit :
>> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>> Cc: Romain Naour <romain.naour@openwide.fr>
>> Cc: Yann E. Morin <yann.morin.1998@free.fr>
>>
>> ---
>>
>> Changes in V4:
>> * Fix comments places in Config.in
>>
>> Changes in V3:
>> * Add dependenies comments for DirectFB and X11 options
>> * Fix license filename
>> * Add explicit enable/disable options for libudev
>> * Fix typo: s/succesful/successful/
>> * Add more X11 dependencies: x11-xcursor, x11-xinerama, x11-xinput, x11-scrnsaver
>> * Disbale opengl/opengles video diver explicitly
>> * Use --disable-rpath option option instead of manual fix
>>
>> Changes in V2:
>> * Fix SDL2 case: s/SDL2/sdl2/
>> * Add dependency on toolchain with dynamic library
>> * Remove unneeded 'SDL2' in sub-options names
>> * Remove unneeded space after comma in if statement
>> * Add explicit enable/disable options for tslib and alsa
>> * Remove Mesa3D optional part
>> * Move unconditionnal SDL2_CONF_OPTS upper
>> * Remove host build
>>
>>
>>   package/Config.in      |  1 +
>>   package/sdl2/Config.in | 32 ++++++++++++++++++++++++
>>   package/sdl2/sdl2.mk   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 101 insertions(+)
>>   create mode 100644 package/sdl2/Config.in
>>   create mode 100644 package/sdl2/sdl2.mk
>>
> [snip]
>
>> +SDL2_DEPENDENCIES += \
>> +	xlib_libX11 xlib_libXext \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr) \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR,xlib_libXcursor) \
>> +	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA,xlib_libXinerama) \
>> +	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO,xproto_inputproto) \
>> +	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO,xproto_scrnsaverproto)
> There are missing ',' and ')' here.

Ok for ')', it is clearly a typo, but why do you want to add an extra comma? It is not the case for other packages.

>
> This part should look like:
>
> SDL2_DEPENDENCIES += \
> 	xlib_libX11 xlib_libXext \
> 	$(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender,) \
> 	$(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr,) \
> 	$(if $(BR2_PACKAGE_XLIB_LIBXCURSOR),xlib_libXcursor,) \
> 	$(if $(BR2_PACKAGE_XLIB_LIBXINERAMA),xlib_libXinerama,) \
> 	$(if $(BR2_PACKAGE_XPROTO_INPUTPROTO),xproto_inputproto,) \
> 	$(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO),xproto_scrnsaverproto,)
>
> sdl2 is seems complicated package, I think it would be easier to review if you
> add it progressively by using a patch series:
>
> PATCH1: sdl2 new package (all mandatory dependencies enabled)
> PATCH2: sdl2 add X11 support
> PATCH3: sdl2 add directfb support

No, I won't. It is extra work for nothing. I think it is not so complicated to review only 2 files to add a new package. Splitting packages for u-boot or Linux kernel make sense most of the time, but, here, clearly no.
Buildroot maintainers/reviewers should send their comments from the 1st version, not 1 guy comments on V1, someone else on V2 etc., again the first guy for V3 on another part of the patch which was not commented the 1st time.

I contribute to lots of open source / free software and contributing to Buildroot is really painful compared to other projects. So please, all send your comments at the same time.
For SDL2, please send your comments this week, and I will send a V5 next week fixing the remaining bits and if it does not fit to another guy again, I will stop sending patches to Buildroot.
Now, I understand better why some people are maintaining a buildroot fork instead of upstreaming their patches. So, please improve your patch submission/review process.


Guillaume


> ...
>
> I'll review your patch latter new week end.
>
> Best regards,
> Romain
>
>

[Buildroot] [PATCH V4] sdl2: new package

[Thomas Petazzoni]

Hello,

On Mon, 06 Jul 2015 14:17:11 +0200, Guillaume GARDET - Olis?o wrote:

> No, I won't. It is extra work for nothing. I think it is not so complicated to review only 2 files to add a new package. Splitting packages for u-boot or Linux kernel make sense most of the time, but, here, clearly no.
> Buildroot maintainers/reviewers should send their comments from the 1st version, not 1 guy comments on V1, someone else on V2 etc., again the first guy for V3 on another part of the patch which was not commented the 1st time.
> 
> I contribute to lots of open source / free software and contributing to Buildroot is really painful compared to other projects. So please, all send your comments at the same time.
> For SDL2, please send your comments this week, and I will send a V5 next week fixing the remaining bits and if it does not fit to another guy again, I will stop sending patches to Buildroot.
> Now, I understand better why some people are maintaining a buildroot fork instead of upstreaming their patches. So, please improve your patch submission/review process.

Could you use an e-mail client that wraps lines at a reasonable length?

Regarding the patch submission/review process, we merge tons and tons
of patches all the time. However, it is true that we tend to merge
patches from people who also help in reviewing patches from others. I
don't think I've seen patch reviews/tests from you. It would definitely
help in increasing the trust that the maintainers have in your patches,
and therefore make them look at your patches faster.

Did you look at http://patchwork.ozlabs.org/project/buildroot/list/ ?
This is the complete list of pending patches we have, which as you can
see is quite long. Your help is definitely welcome to reduce this list,
by reviewing, testing and commenting patches from others.

And if you think that the entire community can synchronize to send all
the comments on the v1, then it clearly means that you have never
contributed to the Linux kernel, or at least not significant patches.
It is very common to get some comments on v1, then some other comments
on v2 (from different persons, or even from the same person who
realized some more issues). That's definitely not something you can
expect from an open-source community.

However, I do agree that we could do better to reply to patches in a
timely fashion. And to make this happen, we need *your* help to look at
patches from others.

Best regards,

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

[Buildroot] [PATCH V4] sdl2: new package

[Guillaume GARDET - Oliséo]

Le 06/07/2015 14:26, Thomas Petazzoni a ?crit :
> Hello,
>
> On Mon, 06 Jul 2015 14:17:11 +0200, Guillaume GARDET - Olis?o wrote:
>
>> No, I won't. It is extra work for nothing. I think it is not so complicated to review only 2 files to add a new package. Splitting packages for u-boot or Linux kernel make sense most of the time, but, here, clearly no.
>> Buildroot maintainers/reviewers should send their comments from the 1st version, not 1 guy comments on V1, someone else on V2 etc., again the first guy for V3 on another part of the patch which was not commented the 1st time.
>>
>> I contribute to lots of open source / free software and contributing to Buildroot is really painful compared to other projects. So please, all send your comments at the same time.
>> For SDL2, please send your comments this week, and I will send a V5 next week fixing the remaining bits and if it does not fit to another guy again, I will stop sending patches to Buildroot.
>> Now, I understand better why some people are maintaining a buildroot fork instead of upstreaming their patches. So, please improve your patch submission/review process.
> Could you use an e-mail client that wraps lines at a reasonable length?
>
> Regarding the patch submission/review process, we merge tons and tons
> of patches all the time. However, it is true that we tend to merge
> patches from people who also help in reviewing patches from others. I
> don't think I've seen patch reviews/tests from you. It would definitely
> help in increasing the trust that the maintainers have in your patches,
> and therefore make them look at your patches faster.

I do not claim to be a Buildroot maintainer/reviewer, even if I use it quite often (as a user) and train people to use it.
I guess you understand we cannot be deeply involved in all open source / free software we contribute and submit patches is already a good contribution.
Lots of people keep their patches and never send them upstream (not only for Buildroot).

>
> Did you look at http://patchwork.ozlabs.org/project/buildroot/list/ ?

Yes. This is quite small compared to u-boot one:
https://patchwork.ozlabs.org/project/uboot/list/

> This is the complete list of pending patches we have, which as you can
> see is quite long. Your help is definitely welcome to reduce this list,
> by reviewing, testing and commenting patches from others.

I guess it is.
But as I wrote previously, it is not possible to be deeply involved in all open source projects we contribute.

>
> And if you think that the entire community can synchronize to send all
> the comments on the v1, then it clearly means that you have never
> contributed to the Linux kernel, or at least not significant patches.
> It is very common to get some comments on v1, then some other comments
> on v2 (from different persons, or even from the same person who
> realized some more issues). That's definitely not something you can
> expect from an open-source community.

It happens, yes, but not on every new patch version and the patches are generally much more complicated.

>
> However, I do agree that we could do better to reply to patches in a
> timely fashion. And to make this happen, we need *your* help to look at
> patches from others.

 From my point of view, other projects, like u-boot, are more patch-friendly than Buildroot. Especially for new contributors.
You cannot ask all patch senders to become reviewers or testers.

It could make sense to split Buildroot tree with maintainers for each part as it is done for the kernel or u-boot. It will avoid that people comments here and there.
Another solution would be that 1 person/team flags a given patch and this person/team follows the patch submission flow. Maybe using the "Deleguate" flag in patchwork tool.
It may be fine to merge a patch even if it is not really perfect (e.g. does not offer all options) and then ask for updates/improvements. That way, it allows someone else to do the following patch.

I write these to help you to improve your work flow, not just to say it is better elsewhere.


That said, do you really think that 5 versions (if I include the next one) is really something normal for a simple single package which does not affect others?
I think 3 or 4 should be enough.


Guillaume

[Buildroot] [PATCH V4] sdl2: new package

[Thomas Petazzoni]

Hello,

Again, please configure Thunderbird to make it wrap the very long lines
you write.

On Mon, 06 Jul 2015 15:23:04 +0200, Guillaume GARDET - Olis?o wrote:

> I do not claim to be a Buildroot maintainer/reviewer, even if I use it quite often (as a user) and train people to use it.
> I guess you understand we cannot be deeply involved in all open source / free software we contribute and submit patches is already a good contribution.
> Lots of people keep their patches and never send them upstream (not only for Buildroot).

That is indeed true, but there's not much more we can do I believe:
people interested in doing review/testing are doing as much of it as
they can. And trust me, a number of us are already spending a crazy
amount of time doing reviews.

> > Did you look at http://patchwork.ozlabs.org/project/buildroot/list/ ?
> 
> Yes. This is quite small compared to u-boot one:
> https://patchwork.ozlabs.org/project/uboot/list/

Is the U-Boot patchwork actually kept up-to-date with the patches that
have been merged? Because there are lots of patchwork instances that
exist, but that are not actively used by the project.

> > However, I do agree that we could do better to reply to patches in a
> > timely fashion. And to make this happen, we need *your* help to look at
> > patches from others.
> 
>  From my point of view, other projects, like u-boot, are more patch-friendly than Buildroot. Especially for new contributors.

I believe it really depends on the specific patch you send. Some new
contributors in Buildroot see their patch applied very quickly, some
other new contributors have a less satisfying experience.

> It could make sense to split Buildroot tree with maintainers for each part as it is done for the kernel or u-boot. It will avoid that people comments here and there.
> Another solution would be that 1 person/team flags a given patch and this person/team follows the patch submission flow. Maybe using the "Deleguate" flag in patchwork tool.

Yeah, we have been talking since a while to add the notion of
maintainers. But that would only work for existing packages. For new
packages it's quite difficult to see to which maintainer they would
belong to. And actually, the original author of the package should
become its maintainer, so there must anyway be someone else in the
Buildroot community reviewing the initial package.

See http://lists.busybox.net/pipermail/buildroot/2015-March/123032.html
for a patch series from Yann Morin proposing to add the notion of
maintainers.

> It may be fine to merge a patch even if it is not really perfect (e.g. does not offer all options) and then ask for updates/improvements. That way, it allows someone else to do the following patch.

I am all for that, and I tend to suggest this a lot. I have no problem
merging a package that does not offer all the possibilities, as long as
it at least explicitly disables the stuff it doesn't use (to avoid
misdetection).

> That said, do you really think that 5 versions (if I include the next one) is really something normal for a simple single package which does not affect others?
> I think 3 or 4 should be enough.

Impossible to make a rule on this. It depends on good/bad the initial
submission is, it depends how deep the review is done, it depends how
complex the package is.

But I agree that something relatively simple such as sdl2 shouldn't
take 5 months to get merged, that's for sure. Especially when the
submitter (you!) has been very active and responsive to take into
account the comments.

Best regards,

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

[Buildroot] [PATCH V4] sdl2: new package

[Arnout Vandecappelle]

 Hi Guillaume,

 Let me start with thanking you for this feedback, since we are indeed
struggling with improving our patch contribution flow.

On 07/06/15 15:23, Guillaume GARDET - Olis?o wrote:
> Le 06/07/2015 14:26, Thomas Petazzoni a ?crit :

>> However, I do agree that we could do better to reply to patches in a
>> timely fashion. And to make this happen, we need *your* help to look at
>> patches from others.
> 
> From my point of view, other projects, like u-boot, are more patch-friendly than
> Buildroot. Especially for new contributors.

 Could you explain what aspect(s) of the U-Boot contribution process make it
more patch-friendly? It indeed looks like their patch queue is shorter even
though they seem to get the same order of magnitude of contributions.

 Are they less critical about patches?

 Are patches taken over by maintainters so the original contributor doesn't have
to respin it too often?

 Do they make sure review happens quickly?

 Are they able to avoid that patches that had no review for a week are
completely ignored for a long time? How do they do that?


> You cannot ask all patch senders to become reviewers or testers.

 We certainly can ask. We can't *require*.

> 
> It could make sense to split Buildroot tree with maintainers for each part as it
> is done for the kernel or u-boot. It will avoid that people comments here and
> there.
> Another solution would be that 1 person/team flags a given patch and this
> person/team follows the patch submission flow. Maybe using the "Deleguate" flag
> in patchwork tool.

 We do try to make sure of that (i.e. the first reviewer continues to look after
the patch), but indeed we have no formal process for it and it's all based on
goodwill.


> It may be fine to merge a patch even if it is not really perfect (e.g. does not
> offer all options) and then ask for updates/improvements. That way, it allows
> someone else to do the following patch.

 I think the fear is that the improvements will never materialize, and we get
stuck with imperfect packages. A typical package will not be looked at again for
a very long time, but on the other hand every package may be one that a new
contributor is looking at as an example of how things should be done...



> 
> I write these to help you to improve your work flow, not just to say it is
> better elsewhere.

 Again, we really appreciate this.

> 
> That said, do you really think that 5 versions (if I include the next one) is
> really something normal for a simple single package which does not affect others?
> I think 3 or 4 should be enough.

 Ideally, 1 iteration should be enough :-)


 Regards,
 Arnout


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH V4] sdl2: new package

[Arnout Vandecappelle]

On 07/02/15 10:16, Guillaume GARDET wrote:
> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Cc: Romain Naour <romain.naour@openwide.fr>
> Cc: Yann E. Morin <yann.morin.1998@free.fr>

  Since you're anyway spinning a v5, one more comment:

> 
> ---

[snip]
> diff --git a/package/Config.in b/package/Config.in
> index 13a7e74..3a1b5c6 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -246,6 +246,7 @@ endif
>  	source "package/psplash/Config.in"
>  	source "package/sawman/Config.in"
>  	source "package/sdl/Config.in"
> +	source "package/sdl2/Config.in"

 Even though this is correct alphabetical ordering, it's not so good. Currently,
when you select sdl, the SDL modules, below, will be indented in menuconfig so
it is clear they belong to the SDL package. By putting sdl2 in-between, the
indentation will be gone.

 Actually, the sdl_* packages should not have this 'depends on ' line but
instead there should be a condition here, then it would be much clearer. But
it's not a perfect world...

 Regards,
 Arnout

>  	source "package/sdl_gfx/Config.in"
>  	source "package/sdl_image/Config.in"
>  	source "package/sdl_mixer/Config.in"

[snip]


-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF

[Buildroot] [PATCH V4] sdl2: new package

[Romain Naour]

Hi Guillaume,

Le 06/07/2015 14:17, Guillaume GARDET - Olis?o a ?crit :
> Hi,
> 
> Le 05/07/2015 20:21, Romain Naour a ?crit :
>> Hi Guillaume,
>>
>> Le 02/07/2015 10:16, Guillaume GARDET a ?crit :
>>> Signed-off-by: Guillaume GARDET <guillaume.gardet@oliseo.fr>
>>> Cc: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
>>> Cc: Romain Naour <romain.naour@openwide.fr>
>>> Cc: Yann E. Morin <yann.morin.1998@free.fr>
>>>
>>> ---
>>>
>>> Changes in V4:
>>> * Fix comments places in Config.in
>>>
>>> Changes in V3:
>>> * Add dependenies comments for DirectFB and X11 options
>>> * Fix license filename
>>> * Add explicit enable/disable options for libudev
>>> * Fix typo: s/succesful/successful/
>>> * Add more X11 dependencies: x11-xcursor, x11-xinerama, x11-xinput,
>>> x11-scrnsaver
>>> * Disbale opengl/opengles video diver explicitly
>>> * Use --disable-rpath option option instead of manual fix
>>>
>>> Changes in V2:
>>> * Fix SDL2 case: s/SDL2/sdl2/
>>> * Add dependency on toolchain with dynamic library
>>> * Remove unneeded 'SDL2' in sub-options names
>>> * Remove unneeded space after comma in if statement
>>> * Add explicit enable/disable options for tslib and alsa
>>> * Remove Mesa3D optional part
>>> * Move unconditionnal SDL2_CONF_OPTS upper
>>> * Remove host build
>>>
>>>
>>>   package/Config.in      |  1 +
>>>   package/sdl2/Config.in | 32 ++++++++++++++++++++++++
>>>   package/sdl2/sdl2.mk   | 68 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 101 insertions(+)
>>>   create mode 100644 package/sdl2/Config.in
>>>   create mode 100644 package/sdl2/sdl2.mk
>>>
>> [snip]
>>
>>> +SDL2_DEPENDENCIES += \
>>> +    xlib_libX11 xlib_libXext \
>>> +    $(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender) \
>>> +    $(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr) \
>>> +    $(if $(BR2_PACKAGE_XLIB_LIBXCURSOR,xlib_libXcursor) \
>>> +    $(if $(BR2_PACKAGE_XLIB_LIBXINERAMA,xlib_libXinerama) \
>>> +    $(if $(BR2_PACKAGE_XPROTO_INPUTPROTO,xproto_inputproto) \
>>> +    $(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO,xproto_scrnsaverproto)
>> There are missing ',' and ')' here.
> 
> Ok for ')', it is clearly a typo, but why do you want to add an extra comma? It
> is not the case for other packages.

Ok, my mistake.

> 
>>
>> This part should look like:
>>
>> SDL2_DEPENDENCIES += \
>>     xlib_libX11 xlib_libXext \
>>     $(if $(BR2_PACKAGE_XLIB_LIBXRENDER),xlib_libXrender,) \
>>     $(if $(BR2_PACKAGE_XLIB_LIBXRANDR),xlib_libXrandr,) \
>>     $(if $(BR2_PACKAGE_XLIB_LIBXCURSOR),xlib_libXcursor,) \
>>     $(if $(BR2_PACKAGE_XLIB_LIBXINERAMA),xlib_libXinerama,) \
>>     $(if $(BR2_PACKAGE_XPROTO_INPUTPROTO),xproto_inputproto,) \
>>     $(if $(BR2_PACKAGE_XPROTO_SCRNSAVERPROTO),xproto_scrnsaverproto,)

Actually it's not correct ether...

We prefer also add autotools --enable-*/--disable-* options to explicitly
enable/disable the optional package.

Something like:
ifeq ($(BR2_PACKAGE_XLIB_LIBXCURSOR),y)
SDL2_DEPENDENCIES += xlib_libXcursor
SDL2_CONF_OPTS += --enable-video-x11-xcursor
else
SDL2_CONF_OPTS += --disable-video-x11-xcursor
endif

>>
>> sdl2 is seems complicated package, I think it would be easier to review if you
>> add it progressively by using a patch series:
>>
>> PATCH1: sdl2 new package (all mandatory dependencies enabled)
>> PATCH2: sdl2 add X11 support
>> PATCH3: sdl2 add directfb support
> 
> No, I won't. It is extra work for nothing. I think it is not so complicated to
> review only 2 files to add a new package.

This is not only review two files, the review process is also to check the
package build system to avoid build failures. Especially, when a package depends
on X11 or OpenGL (from my point of view).

Until now, I had no time to review deeply the sdl2 package. Even during the
hackaton this week.

So, I reviewed your package today and checked for missing package/toolchains
dependencies, and made several changes:

    [Romain:
      - Wrap Config.in help text at 72 columns.
      - Move sdl2 package after sdl modules in Config.in. (Arnout)
      - Explicitly disable dbus and wayland.
      - Remove double underscore (SDL2__*).
      - Unify autotools options to use --enable/--disable.
      - Use x-includes and x-libraries to avoid path poisoning.
      - Remove xlib_libXrender, xproto_inputproto and xproto_scrnsaverproto
        dependencies since the build system doesn't depend on them.
      - Add Xlib_libXi, xlib_libScrnSaver and xlib_libXxf86vm dependencies.
      - Handle autotools options (--enable/--disable) for each X11
        dependencies.]

I'll send an updated version, care to take a look and review please ?

 Splitting packages for u-boot or Linux
> kernel make sense most of the time, but, here, clearly no.
> Buildroot maintainers/reviewers should send their comments from the 1st version,
> not 1 guy comments on V1, someone else on V2 etc., again the first guy for V3 on
> another part of the patch which was not commented the 1st time.
> 
> I contribute to lots of open source / free software and contributing to
> Buildroot is really painful compared to other projects. So please, all send your
> comments at the same time.

I have nothing to say to complete Thomas's and Arnout's answers.
Feel free to come to the Buildroot Developer Days and see how patches are
discussed and reviewed:

http://elinux.org/Buildroot:DeveloperDaysELCE2015

> For SDL2, please send your comments this week, and I will send a V5 next week
> fixing the remaining bits and if it does not fit to another guy again, I will
> stop sending patches to Buildroot.
> Now, I understand better why some people are maintaining a buildroot fork
> instead of upstreaming their patches. So, please improve your patch
> submission/review process.

Yes, upstreaming is difficult but the final result is really better.
See changes that have been made to your c-icap and c-icap-modules packages.

Best regards,
Romain

> 
> 
> Guillaume
> 
> 
>> ...
>>
>> I'll review your patch latter new week end.
>>
>> Best regards,
>> Romain
>>
>>
> 

[Buildroot] [PATCH V4] sdl2: new package

[Vincent Stehlé]

Hi Guillaume and Romain,

Thank you for your effort on this nice sdl2 buildroot patch. I hope you
are still working on it.

While trying v5 of the path recently, as found in buildroot patchwork, I
needed to add the following line to sdl2.mk:

	SDL2_CONFIG_SCRIPTS = sdl2-config

Without this line, the include path returned by `sdl2-config --cflags`
will be something like /usr/include/SDL2, instead of pointing to the
buildroot staging dir.

You might want to add it to a v6, maybe?

Apart from that, it works fine for me, thanks!

Best regards,

V.

[Buildroot] [PATCH V4] sdl2: new package

[Romain Naour]

Hi Vincent, All,

Le 19/10/2015 15:45, Vincent Stehl? a ?crit :
> Hi Guillaume and Romain,
> 
> Thank you for your effort on this nice sdl2 buildroot patch. I hope you
> are still working on it.

Actually no, I only reviewed it after the Buildroot summer camp and sent an
updated version. Since then it's waiting for a reviewer or tester ;-)

> 
> While trying v5 of the path recently, as found in buildroot patchwork, I
> needed to add the following line to sdl2.mk:
> 
> 	SDL2_CONFIG_SCRIPTS = sdl2-config
> 
> Without this line, the include path returned by `sdl2-config --cflags`
> will be something like /usr/include/SDL2, instead of pointing to the
> buildroot staging dir.
> 
> You might want to add it to a v6, maybe?

Thanks for the feedback ! You're probably right, I need to rebuild it locally to
check.

I used it to enable the sdl2 support during the development of my efl patches
series, I didn't see any build issue even with the paranoid toolchain wrapper
enabled... (but the sdl2-config is probably not used)

> 
> Apart from that, it works fine for me, thanks!

Great!
I'll send an updated version with your fix, can you send your tested and/or
reviewed tag ;-)

Best regards,
Romain

> 
> Best regards,
> 
> V.
> 

[Buildroot] [PATCH V4] sdl2: new package

[Romain Naour]

Hi Vincent, All,

Le 21/10/2015 22:56, Romain Naour a ?crit :
> Hi Vincent, All,
>>
>> While trying v5 of the path recently, as found in buildroot patchwork, I
>> needed to add the following line to sdl2.mk:
>>
>> 	SDL2_CONFIG_SCRIPTS = sdl2-config
>>
>> Without this line, the include path returned by `sdl2-config --cflags`
>> will be something like /usr/include/SDL2, instead of pointing to the
>> buildroot staging dir.
>>

I confirm there is a poisoned path returned by this script:

sdl2-config --cflags
-I/usr/include/SDL2
-I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
-D_REENTRANT

sdl2-config --libs
-L/usr/lib -lSDL2 -lpthread

And thanks to your fix:

sdl2-config --cflags
-I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include/SDL2
-I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
-D_REENTRANT

sdl2-config --libs
-L/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/lib
-lSDL2 -lpthread

Good catch !

Best regards,
Romain

[Buildroot] [PATCH V4] sdl2: new package

[Thomas Petazzoni]

Romain,

On Wed, 21 Oct 2015 23:09:58 +0200, Romain Naour wrote:

> I confirm there is a poisoned path returned by this script:
> 
> sdl2-config --cflags
> -I/usr/include/SDL2
> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
> -D_REENTRANT
> 
> sdl2-config --libs
> -L/usr/lib -lSDL2 -lpthread
> 
> And thanks to your fix:
> 
> sdl2-config --cflags
> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include/SDL2
> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
> -D_REENTRANT
> 
> sdl2-config --libs
> -L/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/lib
> -lSDL2 -lpthread

I'm finally looking at sdl2, and added the SDL2_CONFIG_SCRIPTS line
suggested by Vincent. If that's the only issue I find, I'll apply the
patch with this change.

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

[Buildroot] [PATCH V4] sdl2: new package

[Romain Naour]

Hi Thomas,

Le 21/10/2015 23:22, Thomas Petazzoni a ?crit :
> Romain,
> 
> On Wed, 21 Oct 2015 23:09:58 +0200, Romain Naour wrote:
> 
>> I confirm there is a poisoned path returned by this script:
>>
>> sdl2-config --cflags
>> -I/usr/include/SDL2
>> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
>> -D_REENTRANT
>>
>> sdl2-config --libs
>> -L/usr/lib -lSDL2 -lpthread
>>
>> And thanks to your fix:
>>
>> sdl2-config --cflags
>> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include/SDL2
>> -I/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/include
>> -D_REENTRANT
>>
>> sdl2-config --libs
>> -L/home/naourr/buildroot/test/efl-1.15.2-v2/host/usr/i686-buildroot-linux-gnu/sysroot/usr/lib
>> -lSDL2 -lpthread
> 
> I'm finally looking at sdl2, and added the SDL2_CONFIG_SCRIPTS line
> suggested by Vincent. If that's the only issue I find, I'll apply the
> patch with this change.

Ok, thanks !

Best regards,
Romain
> 
> Thomas
>