Re: [Buildroot] [PATCH 1/1] package/oatpp:: new package

["Yann E. MORIN" <yann.morin.1998@free.fr>]

Alessandro, All,

Thanks for this patc; please find my review and comments below.

On 2023-01-28 10:41 +0100, Alessandro Partesotti spake thusly:
> This package introduce oatpp in BR buildsystem. oatpp must be used as static library in $(STAGING_DIR)/usr/include/oatpp-$(OATPP_VERSION)/oatpp for user that want to build therir own application by linking oatpp in a buildroot build system.

Please, wrap your commit log to about 72 chars.

Why does it need to be a static library? Why does a shared library not
work? I see tht upstream suggests that, so maybe just state so.

> Signed-off-by: Alessandro Partesotti <a.partesotti@gmail.com>
> ---
>  package/Config.in       |  3 +++
>  package/oatpp/Config.in | 14 ++++++++++++++
>  package/oatpp/oatpp.mk  | 16 ++++++++++++++++

You are missing a hash file, which contains the hashes for the source
archive and the license file(s). See below.

> diff --git a/package/Config.in b/package/Config.in
> index 995dae2c57..481876a278 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1941,6 +1941,7 @@ menu "Networking"
>  	source "package/nss-mdns/Config.in"
>  	source "package/nss-myhostname/Config.in"
>  	source "package/nss-pam-ldapd/Config.in"
> +	source "package/oatpp/Config.in"
>  	source "package/omniorb/Config.in"
>  	source "package/open-isns/Config.in"
>  	source "package/open62541/Config.in"
> @@ -2702,4 +2703,6 @@ menu "Text editors and viewers"
>  	source "package/vim/Config.in"
>  endmenu
>  
> +
>  endmenu
> +

Two spurious empty lines added.

> diff --git a/package/oatpp/Config.in b/package/oatpp/Config.in
> new file mode 100644
> index 0000000000..86eb4573a6
> --- /dev/null
> +++ b/package/oatpp/Config.in
> @@ -0,0 +1,14 @@
> +comment "Oat++ needs a toolchain w/ C++, threads and Paranoid Unsafe Path compiler flag disabled"
> +        depends on !BR2_INSTALL_LIBSTDCPP || !BR2_TOOLCHAIN_HAS_THREADS || BR2_COMPILER_PARANOID_UNSAFE_PATH

If the paranoid unsafe paths check triggers, it means that the target
build is using headers (or libraries) from the host, and that is
definitely not correct, as those are not suitable to generate target
code.

Instead, the package must be fixed.

Long-term, we may even make that check mandatory and not configurable,
see:
    http://patchwork.ozlabs.org/project/buildroot/patch/20221107214903.1565321-1-yann.morin.1998@free.fr/

> +config BR2_PACKAGE_OATPP
> +	bool "Oat++"
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	depends on !BR2_COMPILER_PARANOID_UNSAFE_PATH
> +	help
> +		Oat++ is an open-source C++ web framework for highly scalable and resource-efficient web applications.
> +		It provides all the necessary components for production-grade development.
> +		This package allows you to use oatpp to statically build this a project inside the staging directory.
> +		See https://oatpp.io/docs/start

Help text should be indented with a TAB and two spaces, and wrapped at
72 chars; running "make check-package" will point to coding style
issues.

> diff --git a/package/oatpp/oatpp.mk b/package/oatpp/oatpp.mk
> new file mode 100644
> index 0000000000..7abdeb6de5
> --- /dev/null
> +++ b/package/oatpp/oatpp.mk
> @@ -0,0 +1,16 @@
> +################################################################################
> +#
> +# oatpp
> +#
> +################################################################################
> +
> +OATPP_VERSION= 1.3.0

Spaces around the equal sign; "make check-package" points to it too.

> +OATPP_SOURCE= $(OATPP_VERSION).tar.gz
> +OATPP_SITE= https://github.com/oatpp/oatpp/archive/refs/tags
> +#OATPP_SITE= git://github.com/oatpp/oatpp.git

Don't keep commented-out code; just remove it.

For github, we have two options:
  - if the package has been "released" by upstream, then they pushed a
    tarball and we must use it;
  - otherwise, we have a macro that assembles a proper URL.

In this case, there is no archive published by upstream, so we must use
the macro:

    OATPP_VERSION = 1.3.0
    OATPP_SITE = $(call github,oatpp,oatpp,$(OATPP_VERSION))

And that's all (no _SOURCE, no _SITE_METHOD).

https://buildroot.org/downloads/manual/manual.html#github-download-url

You are also missing the license declaration:
    OATPP_LICENSE = Apache-2.0
    OATPP_LICENSE_FILES = LICENSE

> +OATPP_INSTALL_STAGING= YES
> +OATPP_INSTALL_TARGET= NO

Shy no installation in target? Ah, yes, it's a static lib. Usually, we
like a small reminder about that:

    # Only builds a static lib:
    OATPP_INSTALL_TARGET= NO

> +OATPP_MAKE=make

Why do you need to force make? If that's because of a parallel build
issue, then use $(MAKE1) instead, and explain it in the commit message.

As it is supposed to be a static-only lib (as per upstream suggestion),
you probably also need to tell cmake to not build shared libs, which is
otherwise what Buildroot enforces:

    # As per upstream, oat++ is meant to be a static-only lib
    OATPP_CMAKE_OPTS = -DBUILD_SHARED_LIBS=OFF

Can you address those and respin an updated patch, please?

Regards,
Yann E. MORIN.

> +$(eval $(cmake-package))
> +
> -- 
> 2.25.1
> 
> _______________________________________________
> buildroot mailing list
> buildroot@buildroot.org
> https://lists.buildroot.org/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot