Re: [Buildroot] [PATCH 1/1] package/qt6: add qt6 multimedia

From: Thomas Petazzoni via buildroot <buildroot@buildroot.org>
To: Roy Kollen Svendsen <roykollensvendsen@gmail.com>
Cc: baxiche@gmail.com, Jesse Van Gavere <jesseevg@gmail.com>,
	Samuel Martin <s.martin49@gmail.com>,
	buildroot@buildroot.org
Subject: Re: [Buildroot] [PATCH 1/1] package/qt6: add qt6 multimedia
Date: Sat, 14 Sep 2024 12:13:20 +0200	[thread overview]
Message-ID: <20240914121320.4672b1ca@windsurf> (raw)
In-Reply-To: <CA+R_dep6gZd6Kv6SuDPE-+RCfjbNArRd9U+yvnYiZ=YX5R9H7Q@mail.gmail.com>

Hello Baxiche, Hello Roy,

First of all, thanks Roy for the review, much appreciated! See my
comments below.

On Wed, 14 Aug 2024 13:59:04 +0200
Roy Kollen Svendsen <roykollensvendsen@gmail.com> wrote:

> > +QT6MULTIMEDIA_LICENSE = \
> > +       LGPL-3.0 or GPL-2.0 or GPL-3.0, \
> > +       LGPL-2.1+, \
> > +       BSD-3-Clause (docs), \
> > +       GFDL-1.3 no invariants (docs), \
> > +       BSD-3-Clause (examples), \
> > +       BSL-1.0, \
> > +       MPL-2.0 \
> > +       MIT  
> 
> Thomas, should we order these licenses alpabetically, and avoid
> duplicating the lines?:
> 
> To avoid duplicating lines I would do:
> 
> QT6MULTIMEDIA_LICENSE = \
>       LGPL-3.0 or GPL-2.0 or GPL-3.0, \
>       LGPL-2.1+, \
>       BSD-3-Clause (docs, examples), \
>       GFDL-1.3 no invariants (docs), \
>       BSL-1.0, \
>       MPL-2.0 \
>       MIT
> 
> And order alphabetically:
>       BSD-3-Clause (docs, examples), \
>       BSL-1.0, \
>       GFDL-1.3 no invariants (docs), \
>       LGPL-2.1+, \
>       LGPL-3.0 or GPL-2.0 or GPL-3.0, \
>       MIT \
>       MPL-2.0

Yes, I agree this looks nicer. We don't enforce alphabetic ordering for
the licenses currently, but here it definitely makes sense to do so.

> > +ifeq ($(BR2_PACKAGE_QT6DECLARATIVE),y)
> > +QT6MULTIMEDIA_DEPENDENCIES += qt6declarative
> > +endif  
> 
> I think it is custom to order these ifeq-statements alphabetically?

Yes, it's good to do so.

> > +ifeq ($(BR2_PACKAGE_PULSEAUDIO),y)
> > +QT6MULTIMEDIA_DEPENDENCIES += pulseaudio
> > +endif
> > +
> > +ifeq ($(BR2_PACKAGE_FFMPEG)$(BR2_PACKAGE_PULSEAUDIO),yy)  
> 
> It may be better to avoid doing the BR2_PACKAGE_PULSEAUDIO==y twice,
> by moving the BR2_PACKAGE_FFMPEG==y inside ifeq
> ($(BR2_PACKAGE_PULSEAUDIO),y)-block above.
> 
> > +QT6MULTIMEDIA_CONF_OPTS += -DFFMPEG_DIR=$(BUILD_DIR)/ffmpeg-$(FFMPEG_VERSION) -DQT_DEPLOY_FFMPEG=ON  
> 
> What about splitting the long line to increase readability?:
> 
> QT6MULTIMEDIA_CONF_OPTS += \
>       -DFFMPEG_DIR=$(BUILD_DIR)/ffmpeg-$(FFMPEG_VERSION) \

I don't really like this. Why is this needed? Why does qt6multimedia
need to poke directly into the ffmpeg source code?

If this hadn't been present, I would have applied the patch with the
fixes proposed by Roy, but I'm not a big fan of this ffmpeg thing.

Also, are there some options to explicitly disable things?

I.e, instead of:

+ifeq ($(BR2_PACKAGE_PULSEAUDIO),y)
+QT6MULTIMEDIA_DEPENDENCIES += pulseaudio
+endif

have something to explicitly enable pulseaudio support in
qt6multimedia, and in an "else" clause, something to explicitly disable
pulseaudio support?

Roy: as Baxiche didn't come back with a new iteration, I'm not sure
they will ever come back with a new version. I'm going to mark this
patch as Changes Requested, but if Baxiche doesn't come back with a new
version and no-one else picks it up, it means it will be forgotten.

Best regards,

Thomas
-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot