Re: [Buildroot] [PATCH v3] package/qt6/qt6scxml: new package

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Roy,

Thanks for the review! I've taken it into account when merging. See below.

On Fri, 13 Sep 2024 09:17:35 +0200
Roy Kollen Svendsen <roykollensvendsen@gmail.com> wrote:

> > diff --git a/package/qt6/qt6scxml/Config.in
> > b/package/qt6/qt6scxml/Config.in
> > new file mode 100644
> > index 0000000000..ff738bdc17
> > --- /dev/null
> > +++ b/package/qt6/qt6scxml/Config.in
> > @@ -0,0 +1,14 @@
> > +config BR2_PACKAGE_QT6SCXML
> > +       bool "qt6scxml"
> > +       help
> > +         This repository contains two relates state machine modules:
> >  
> 
> I think this is more correct:
> 
>          This package contains the following Qt modules:
> 
> 
> > +
> > +         * Qt6::StateMachine: The State Machine framework provides
> >  
> 
> Maybe indent with two spaces the bullet points and associated text. I've
> seen this in other packages.
> 
> Example from bind:
> 
> help
>  BIND (Berkeley Internet Name Domain) is an
>  implementation of the Domain Name System (DNS) protocols
>  and provides an openly redistributable reference
>  implementation of the major components of the Domain
>  Name System, including:
> 
>    * a Domain Name System server (named)
>    * a Domain Name System resolver library
>    * tools for verifying the proper operation of the DNS
>      server
> 
> Maybe drop the '::' so the name correspond to what is found in the
> documentation:
> 
>  * Qt StateMachine: The State Machine framework provides

I indeed rephrased a bit the help text. Not exactly as you suggested,
because I also made it consistent with other Qt6 packages, but
definitely took into account your suggestions.


> +           create state machines from SCXML files.
> > +
> > +         https://doc.qt.io/qt-6/qtscxml-index.html  
> 
> 
> Maybe we should also say something about the statemachine compiler tool?

I wasn't sure what to add, and it can be added as a follow-up patch if
needed.

> Some test qml files are under GPL-3.0-only. So I think it would be best to
> summarize as (buildsystem, examples, snippets)

ACK, taken into account.

> > +ifeq ($(BR2_PACKAGE_QT6DECLARATIVE),y)
> > +QT6SCXML_DEPENDENCIES += qt6declarative
> > +HOST_QT6SCXML_DEPENDENCIES += host-qt6declarative
> 
> What if we only build host-qt6scxml?

It is not possible to build just host-qt6scxml. For now, it's a
"hidden" host package, in the sense that it can only be built as a
dependency of target qt6scxml. So I think that solution was good
enough: when target qt6scxml is built with declarative support, it
needs host-qt6scxml to have been built with declarative support.

Thanks!

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