[v2,1/1] flatcc: new package

This adds flatcc as a new package, pulling v0.3.3 from github. flatcc has
both a host tool - the compiler, and libraries for the target.

Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>

---
Changes v1 -> v2
  - Updated to v0.3.3 of flatcc
  - Added hash file
  - Added some build patches to flatcc in order to use v0.3.3
  - Added ability to handle static and shared libs at same time
  - Addressed other reviewer comments

Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>
---
 package/Config.in                                  |   1 +
 ...d-Specify-C-language-explicitly-for-CMake.patch |  30 ++++
 ...-cmake-to-build-both-static-and-shared-li.patch | 158 +++++++++++++++++++++
 package/flatcc/Config.in                           |   9 ++
 package/flatcc/flatcc.hash                         |   2 +
 package/flatcc/flatcc.mk                           |  63 ++++++++
 6 files changed, 263 insertions(+)
 create mode 100644 package/flatcc/0001-build-Specify-C-language-explicitly-for-CMake.patch
 create mode 100644 package/flatcc/0002-build-allow-cmake-to-build-both-static-and-shared-li.patch
 create mode 100644 package/flatcc/Config.in
 create mode 100644 package/flatcc/flatcc.hash
 create mode 100644 package/flatcc/flatcc.mk

Hi Steve,

On Mon, May 2, 2016 at 1:29 AM, Steve deRosier <derosier@gmail.com> wrote:
> This adds flatcc as a new package, pulling v0.3.3 from github. flatcc has
> both a host tool - the compiler, and libraries for the target.
>
> Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>
>
> ---
> Changes v1 -> v2
>   - Updated to v0.3.3 of flatcc
>   - Added hash file
>   - Added some build patches to flatcc in order to use v0.3.3
>   - Added ability to handle static and shared libs at same time
>   - Addressed other reviewer comments
>
> Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>
> ---
>  package/Config.in                                  |   1 +
>  ...d-Specify-C-language-explicitly-for-CMake.patch |  30 ++++
>  ...-cmake-to-build-both-static-and-shared-li.patch | 158 +++++++++++++++++++++
>  package/flatcc/Config.in                           |   9 ++
>  package/flatcc/flatcc.hash                         |   2 +
>  package/flatcc/flatcc.mk                           |  63 ++++++++
>  6 files changed, 263 insertions(+)
>  create mode 100644 package/flatcc/0001-build-Specify-C-language-explicitly-for-CMake.patch
>  create mode 100644 package/flatcc/0002-build-allow-cmake-to-build-both-static-and-shared-li.patch
>  create mode 100644 package/flatcc/Config.in
>  create mode 100644 package/flatcc/flatcc.hash
>  create mode 100644 package/flatcc/flatcc.mk
>
> diff --git a/package/Config.in b/package/Config.in
> index 9d668bf..4f00446 100644
> --- a/package/Config.in
> +++ b/package/Config.in
> @@ -1208,6 +1208,7 @@ menu "Other"
>         source "package/ding-libs/Config.in"
>         source "package/eigen/Config.in"
>         source "package/elfutils/Config.in"
> +       source "package/flatcc/Config.in"
>         source "package/fftw/Config.in"
>         source "package/flann/Config.in"
>         source "package/gflags/Config.in"
> diff --git a/package/flatcc/0001-build-Specify-C-language-explicitly-for-CMake.patch b/package/flatcc/0001-build-Specify-C-language-explicitly-for-CMake.patch
> new file mode 100644
> index 0000000..bccc0f7
> --- /dev/null
> +++ b/package/flatcc/0001-build-Specify-C-language-explicitly-for-CMake.patch
> @@ -0,0 +1,30 @@
> +From 82b75921413502374ad23d7a9d93b9e9021c0742 Mon Sep 17 00:00:00 2001
> +From: Steve deRosier <steve.derosier@lairdtech.com>
> +Date: Thu, 28 Apr 2016 15:58:52 -0700
> +Subject: [PATCH 1/2] build: Specify C language explicitly for CMake
> +
> +If the build system doesn't have a C++ compiler installed, CMake will fail
> +with a line like:
> +Check for working CXX compiler: CMAKE_CXX_COMPILER_FULLPATH-NOTFOUND
> +
> +As flatcc is explicitly C-only, let's be sure we can use it without a C++
> +compiler installed.
> +
> +Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>
> +
> +diff --git a/CMakeLists.txt b/CMakeLists.txt
> +index 6157f96..b9f5bc3 100644
> +--- a/CMakeLists.txt
> ++++ b/CMakeLists.txt
> +@@ -4,7 +4,7 @@
> + #cmake_minimum_required (VERSION 2.8.11)
> + cmake_minimum_required (VERSION 2.8)
> +
> +-project (FlatCC)
> ++project (FlatCC C)
> +
> + #
> + # NOTE: when changing build options, clean the build using:
> +--
> +1.9.1
> +
> diff --git a/package/flatcc/0002-build-allow-cmake-to-build-both-static-and-shared-li.patch b/package/flatcc/0002-build-allow-cmake-to-build-both-static-and-shared-li.patch
> new file mode 100644
> index 0000000..091631e
> --- /dev/null
> +++ b/package/flatcc/0002-build-allow-cmake-to-build-both-static-and-shared-li.patch
> @@ -0,0 +1,158 @@
> +From bad274137c7cd8bd356b65ae4e3fcbd60b072b5d Mon Sep 17 00:00:00 2001
> +From: Steve deRosier <steve.derosier@lairdtech.com>
> +Date: Fri, 29 Apr 2016 16:25:29 -0700
> +Subject: [PATCH 2/2] build: allow cmake to build both static and shared
> + libraries
> +
> +By default, cmake can only build one of static or shared libraries, not
> +both. Systems like Buildroot have the ability to configure the build to
> +build both static and shared libraries, and the current behavior gets in
> +the way of that. This patch makes it possible to do both on the same build.
> +
> +Signed-off-by: Steve deRosier <steve.derosier@lairdtech.com>
> +
> +diff --git a/CMakeLists.txt b/CMakeLists.txt
> +index b9f5bc3..da30854 100644
> +--- a/CMakeLists.txt
> ++++ b/CMakeLists.txt
> +@@ -12,6 +12,11 @@ project (FlatCC C)
> + #   scripts/cleanall.sh
> + #
> +
> ++# Options to control if we build static or shared libraries. Needed because
> ++# cmake has us explicitly do both versions if we want both versions.
> ++option(FLATCC_WITH_STATIC "Build the static version of the library" ON)
> ++option(FLATCC_WITH_SHARED "Build the shared version of the library" ON)
I'm not a big fan of this because:
- it kinda adds some feature to flatcc;
- it completely by-passes the standard CMake way of driving
shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
infrastructure automaticllay set [1].

On this latter point, this is a true limitation of CMake compared to autotools:
CMake only provide one boolean flag to control the shared libs. build,
instead of a tristate option allowing building: shared libs only, or
static libs only, or shared and static libs.

I tend to give priority to the shared libs build (and this is what the
infra does [1]).

IMO (that does not include other developers), I tend to understand the
"shared and static lib" option like this:
"do your best, build some libs, shared or static or both, I don't
really, but give me something I can use."

> ++
> + # Disable if cross compiling or if experiencing build issues with certain
> + # custom CMake configurations - suspect dependency issue on flatcc tool
> + # in custom build steps used by tests and samples.
> +@@ -66,6 +71,10 @@ option (FLATCC_ALLOW_WERROR "allow -Werror to be configured" ON)
> + # try using this option.
> + option (FLATCC_IGNORE_CONST_COND "silence const condition warnings" OFF)
> +
> ++if (NOT (FLATCC_WITH_STATIC OR FLATCC_WITH_SHARED))
> ++      message(FATAL_ERROR "Makes no sense to compile with neither static nor shared libraries.")
> ++endif()
In such a case, you could check for BUILD_SHARED_LIBS flags...

Or the other way around, check first for BUILD_SHARED_LIB, then set
FLATCC_WITH_STATIC and/or FLATCC_WITH_SHARED from it.


> ++
> + if (FLATCC_TEST)
> +     enable_testing()
> + endif()
> +@@ -99,9 +108,17 @@ set (dist_dir "${PROJECT_SOURCE_DIR}")
> + # set (dist_dir "${CMAKE_BINARY_DIR}")
> +
> + # The targets we copy to bin and lib directories, i.e. not tests.
> ++if (FLATCC_WITH_STATIC)
> ++    set(static_libs flatcc flatccrt)
> ++endif()
> ++
> ++if (FLATCC_WITH_SHARED)
> ++    set(static_libs flatcc_shared flatccrt_shared)
> ++endif()
> ++
> + set(dist_targets
> +-    flatcc
> +-    flatccrt
> ++    ${static_libs}
> ++    ${shared_libs}
> +     flatcc_cli
> + )
> +
> +diff --git a/src/cli/CMakeLists.txt b/src/cli/CMakeLists.txt
> +index e4baf28..58df256 100644
> +--- a/src/cli/CMakeLists.txt
> ++++ b/src/cli/CMakeLists.txt
> +@@ -7,9 +7,15 @@ add_executable(flatcc_cli
> +     flatcc_cli.c
> + )
> +
> +-target_link_libraries(flatcc_cli
> +-    flatcc
> +-)
> ++if (FLATCC_WITH_SHARED)
> ++    target_link_libraries(flatcc_cli
> ++        flatcc_shared
> ++    )
> ++elseif (FLATCC_WITH_STATIC)
> ++    target_link_libraries(flatcc_cli
> ++        flatcc
> ++    )
> ++endif()
> +
> + # Rename because the libflatcc library and the flatcc executable would
> + # conflict if they had the same target name `flatcc`.
> +diff --git a/src/compiler/CMakeLists.txt b/src/compiler/CMakeLists.txt
> +index 9bf6b13..6b41289 100644
> +--- a/src/compiler/CMakeLists.txt
> ++++ b/src/compiler/CMakeLists.txt
> +@@ -34,5 +34,26 @@ if (FLATCC_REFLECTION)
> +     set (SOURCES ${SOURCES} codegen_schema.c)
> + endif(FLATCC_REFLECTION)
> +
> +-add_library(flatcc ${SOURCES})
> ++if (FLATCC_WITH_STATIC)
> ++    add_library(flatcc STATIC ${SOURCES})
> ++    list(APPEND FLATCC_LIBRARIES flatcc)
> +
> ++    if (WIN32)
> ++        # Windows uses the same .lib ending for static libraries and shared
> ++        # library linker files, so rename the static library.
> ++        set_target_properties(flatcc
> ++            PROPERTIES
> ++            OUTPUT_NAME flatcc_static)
> ++    endif()
> ++endif()
> ++
> ++if (FLATCC_WITH_SHARED)
> ++     add_library(flatcc_shared SHARED ${SOURCES})
> ++     list(APPEND FLATCC_LIBRARIES flatcc_shared)
> ++
> ++     # We want the shared lib to be named "libflatcc"
> ++     # not "libflatcc_shared".
> ++     set_target_properties(flatcc_shared
> ++         PROPERTIES
> ++         OUTPUT_NAME flatcc)
> ++endif()
> +diff --git a/src/runtime/CMakeLists.txt b/src/runtime/CMakeLists.txt
> +index 8fe6fef..652dd14 100644
> +--- a/src/runtime/CMakeLists.txt
> ++++ b/src/runtime/CMakeLists.txt
> +@@ -2,10 +2,34 @@ include_directories (
> +     "${PROJECT_SOURCE_DIR}/include"
> + )
> +
> +-add_library(flatccrt
> ++set (FLATCCRT_SOURCES
> +     builder.c
> +     emitter.c
> +     verifier.c
> +     json_parser.c
> +     json_printer.c
> + )
> ++
> ++if (FLATCC_WITH_STATIC)
> ++    add_library(flatccrt STATIC ${FLATCCRT_SOURCES})
> ++    list(APPEND FLATCC_LIBRARIES flatccrt)
> ++
> ++    if (WIN32)
> ++        # Windows uses the same .lib ending for static libraries and shared
> ++        # library linker files, so rename the static library.
> ++        set_target_properties(flatccrt
> ++            PROPERTIES
> ++            OUTPUT_NAME flatccrt_static)
> ++    endif()
> ++endif()
> ++
> ++if (FLATCC_WITH_SHARED)
> ++    add_library(flatccrt_shared SHARED ${FLATCCRT_SOURCES})
> ++    list(APPEND FLATCC_LIBRARIES flatccrt_shared)
> ++
> ++    # We want the shared lib to be named "libflatccrt"
> ++    # not "libflatccrt_shared".
> ++    set_target_properties(flatccrt_shared
> ++        PROPERTIES
> ++        OUTPUT_NAME flatccrt)
> ++endif()
> +--
> +1.9.1
> +
> diff --git a/package/flatcc/Config.in b/package/flatcc/Config.in
> new file mode 100644
> index 0000000..a3b9b01
> --- /dev/null
> +++ b/package/flatcc/Config.in
> @@ -0,0 +1,9 @@
> +config BR2_PACKAGE_FLATCC
> +       bool "flatcc"
> +       depends on BR2_ENDIAN = "LITTLE"
> +       help
> +         flatcc is C language implementation of Google Flatbuffers. It
> +         consists of both a library for the target as well as a
> +         flatbuffer compiler tool for the host.
> +
> +         https://github.com/dvidelabs/flatcc
> diff --git a/package/flatcc/flatcc.hash b/package/flatcc/flatcc.hash
> new file mode 100644
> index 0000000..881e0ca
> --- /dev/null
> +++ b/package/flatcc/flatcc.hash
> @@ -0,0 +1,2 @@
> +# Locally calculated
> +sha256 14903f53536947295214f7c1537b6ff933565453a440e610f0b85c3fb3fe6642  flatcc-v0.3.3.tar.gz
> diff --git a/package/flatcc/flatcc.mk b/package/flatcc/flatcc.mk
> new file mode 100644
> index 0000000..a7e9b7a
> --- /dev/null
> +++ b/package/flatcc/flatcc.mk
> @@ -0,0 +1,63 @@
> +################################################################################
> +#
> +# FLATCC
> +#
> +################################################################################
> +FLATCC_VERSION = v0.3.3
> +FLATCC_SITE =$(call github,dvidelabs,flatcc,$(FLATCC_VERSION))
s/=$/= $/

> +FLATCC_LICENSE = Apache-2.0
> +FLATCC_LICENSE_FILES = LICENSE
> +FLATCC_INSTALL_STAGING = YES
> +FLATCC_DEPENDENCIES = host-flatcc
> +
> +FLATCC_CONF_OPTS += -DFLATCC_TEST=OFF
> +HOST_FLATCC_CONF_OPTS += -DFLATCC_TEST=OFF
> +
> +define HOST_FLATCC_INSTALL_CMDS
> +       $(INSTALL) -D -m 0755 $(@D)/bin/flatcc $(HOST_DIR)/usr/bin/flatcc
> +endef
> +
> +# Need to force flatcc to do static or shared or both libraries
> +ifeq ($(BR2_STATIC_LIBS),y)
> +FLATCC_CONF_OPTS += -DFLATCC_WITH_SHARED=OFF -DFLATCC_WITH_STATIC=ON
> +else ifeq ($(BR2_SHARED_STATIC_LIBS),y)
> +FLATCC_CONF_OPTS += -DFLATCC_WITH_SHARED=ON -DFLATCC_WITH_STATIC=ON
> +else ifeq ($(BR2_SHARED_LIBS),y)
> +FLATCC_CONF_OPTS += -DFLATCC_WITH_SHARED=ON -DFLATCC_WITH_STATIC=OFF
> +endif
> +
> +# flatcc's cmake build doesn't include install targets, so we need to manually
> +# install the various components to their respective destinations. There's
> +# several cases that need to be taken care of, so we do a little dance to do so.
Sad, no install rules at all in this package. :-(

Below, I don't see any install rules for the executables. Are they
useless? or for test purpose?
Or maybe they are correctly handle by the build system; in such a
case, a comment would be welcome ;-)

> +ifeq ($(BR2_SHARED_LIBS),)
I think this would be easier (at least to review, then maintain):
ifeq ($(BR2_STATIC_LIBS)$(BR2_SHARED_STATIC_LIBS),y)

> +define FLATCC_INSTALL_STAGING_STATIC
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.a $(STAGING_DIR)/usr/lib/libflatcc.a
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.a $(STAGING_DIR)/usr/lib/libflatccrt.a
> +endef
> +endif
> +
> +ifeq ($(BR2_STATIC_LIBS),)
ditto

> +define FLATCC_INSTALL_STAGING_SHARED
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(STAGING_DIR)/usr/lib/libflatcc.so
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(STAGING_DIR)/usr/lib/libflatccrt.so
> +endef
> +endif
You add a patch to the cmake code to build static/shared libs, why not
doing the same for the install rules?

> +
> +define FLATCC_INSTALL_STAGING_CMDS
> +       cp -r $(@D)/include/flatcc $(STAGING_DIR)/usr/include/.
> +       $(FLATCC_INSTALL_STAGING_STATIC)
> +       $(FLATCC_INSTALL_STAGING_SHARED)
> +endef
ditto

> +
> +# If we don't have the shared libs to install, don't run target install
> +ifeq ($(FLATCC_INSTALL_STAGING_SHARED),)
How about using a more straight forward logic?
ifeq ($(BR2_STATIC_LIBS),y)

> +FLATCC_INSTALL_TARGET = NO
> +endif
> +
> +define FLATCC_INSTALL_TARGET_CMDS
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(TARGET_DIR)/usr/lib/libflatcc.so
> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(TARGET_DIR)/usr/lib/libflatccrt.so
> +endef
> +
> +$(eval $(cmake-package))
> +$(eval $(host-cmake-package))
What is the purpose of the host package?
As-is, I doubt the host package installs anything in the HOST_DIR.


[1] https://git.buildroot.org/buildroot/tree/package/pkg-cmake.mk#n100


Regards,

On 05/02/16 21:44, Samuel Martin wrote:
> IMO (that does not include other developers), I tend to understand the
> "shared and static lib" option like this:
> "do your best, build some libs, shared or static or both, I don't
> really, but give me something I can use."

  I think it's more like: "In the past, we used to build both static and shared 
libraries in many cases. So let's try to keep doing that." What is very clear, 
however: when this option is enabled, shared libraries are preferred if both 
can't be built.

  But for me, I wouldn't care much if we just removed this option.


  Regards,
  Arnout

Hi Samuel,

Thanks for taking a detailed look at this. Hopefully my answers below
will address your concerns.


On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com> wrote:
>> +
>> ++# Options to control if we build static or shared libraries. Needed because
>> ++# cmake has us explicitly do both versions if we want both versions.
>> ++option(FLATCC_WITH_STATIC "Build the static version of the library" ON)
>> ++option(FLATCC_WITH_SHARED "Build the shared version of the library" ON)
> I'm not a big fan of this because:
> - it kinda adds some feature to flatcc;
> - it completely by-passes the standard CMake way of driving
> shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
> infrastructure automaticllay set [1].
>

I wish you spoke up two weeks ago when Arnout explicitly asked me to
add the shared+static to the build. You could've saved me a good week
of work and two weeks delay in getting support for this upstreamed.

As you note, CMake can only do static OR shared libraries without
specific supporting code in the project's CMakeLists file. The change
I made is a common idiom to support both at the same time. As I was
asked to add support for buildroot's static + shared if I could, I
chose to do so.

As for adding a feature to flatcc, that's intended. This will be
upstreamed to flatcc, however I don't want to wait for the maintainer
to do a v0.3.4 or greater release with my patch in it.



> On this latter point, this is a true limitation of CMake compared to autotools:
> CMake only provide one boolean flag to control the shared libs. build,
> instead of a tristate option allowing building: shared libs only, or
> static libs only, or shared and static libs.
>

I can't change CMake, but I could easily adjust flatcc to do this.
Hence I chose to do it this way.


> I tend to give priority to the shared libs build (and this is what the
> infra does [1]).
>
> IMO (that does not include other developers), I tend to understand the
> "shared and static lib" option like this:
> "do your best, build some libs, shared or static or both, I don't
> really, but give me something I can use."

Sure. If I left it the way it was, the shared+static was totally
broken. The interpretation I have of the rules and what was suggested
to me was it isn't a "do your best", it's a "do as I tell you", which
is build _both_.


>
>> ++
>> + # Disable if cross compiling or if experiencing build issues with certain
>> + # custom CMake configurations - suspect dependency issue on flatcc tool
>> + # in custom build steps used by tests and samples.
>> +@@ -66,6 +71,10 @@ option (FLATCC_ALLOW_WERROR "allow -Werror to be configured" ON)
>> + # try using this option.
>> + option (FLATCC_IGNORE_CONST_COND "silence const condition warnings" OFF)
>> +
>> ++if (NOT (FLATCC_WITH_STATIC OR FLATCC_WITH_SHARED))
>> ++      message(FATAL_ERROR "Makes no sense to compile with neither static nor shared libraries.")
>> ++endif()
> In such a case, you could check for BUILD_SHARED_LIBS flags...
>
> Or the other way around, check first for BUILD_SHARED_LIB, then set
> FLATCC_WITH_STATIC and/or FLATCC_WITH_SHARED from it.
>

Setting BUILD_SHARED_LIBS only causes cmake to build shared libs.
BUILD_STATIC_LIBS causes cmake to build static.  Setting both only
resulted in one working (sorry, can't recall which one off the top of
my head). No matter how you cut it, I had to modify flatcc to get both
built. Trust me, I tried everything I could think of to get this to
work without modifying flatcc. I couldn't trust cmake, and I had to
make changes, and this is a pattern that other BR projects happen to
use. So I chose to use it.

As for your second item, it was harmless to flatcc to build both by
default. If someone explicitly turns both off, then we should tell
them so, not ignore what they've asked and silently choose something
other than they explicitly asked.


>> +
>> +# flatcc's cmake build doesn't include install targets, so we need to manually
>> +# install the various components to their respective destinations. There's
>> +# several cases that need to be taken care of, so we do a little dance to do so.
> Sad, no install rules at all in this package. :-(
>

Mikkel has already corrected this upstream with our input but it is
not available up through the current release (v0.3.3). For now, this
is easy enough to do manually, and I'll remove it when we upgrade to a
release that includes the feature. After I verify it does what I want
for both the host and target cases.

> Below, I don't see any install rules for the executables. Are they
> useless? or for test purpose?
> Or maybe they are correctly handle by the build system; in such a
> case, a comment would be welcome ;-)

Well, flatcc contains two things:
* A library
* A flatbuffer schema compiler

There is no executable created/needed for the target build.
The host build only needs the compiler.

I thought both the commit message and what flatcc.mk installs makes it
pretty clear. Perhaps I was wrong about that?


>> +define FLATCC_INSTALL_STAGING_SHARED
>> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(STAGING_DIR)/usr/lib/libflatcc.so
>> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(STAGING_DIR)/usr/lib/libflatccrt.so
>> +endef
>> +endif
> You add a patch to the cmake code to build static/shared libs, why not
> doing the same for the install rules?
>

Sure, I suppose I could have. However:
* There is already a patch upstream for this (though I've yet to check
it's suitability for a xcompile env where we need both host and target
builds).
* The install is easy to do from the flatcc.mk without changing flatcc
* I want to be sure to install flatcc for the host, but not for the
target (because such thing is useless)
* I want to be sure to install the libraries in the right places for the target

Also - while you're right, I could've added a patch to flatcc, I
didn't want to unnecessary add stuff to flatcc itself. The difference
between the install vs the shared/static is it's impossible to do the
shared/static with only changes to flatcc.mk.


>> +
>> +# If we don't have the shared libs to install, don't run target install
>> +ifeq ($(FLATCC_INSTALL_STAGING_SHARED),)
> How about using a more straight forward logic?
> ifeq ($(BR2_STATIC_LIBS),y)
>

Because doing otherwise was more complex for the shared+static case.
This seemed more straight-forward to me: "if we don't build the shared
libs, then don't try to install them". I suppose it's a matter of
preference.


>> +FLATCC_INSTALL_TARGET = NO
>> +endif
>> +
>> +define FLATCC_INSTALL_TARGET_CMDS
>> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(TARGET_DIR)/usr/lib/libflatcc.so
>> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(TARGET_DIR)/usr/lib/libflatccrt.so
>> +endef
>> +
>> +$(eval $(cmake-package))
>> +$(eval $(host-cmake-package))
> What is the purpose of the host package?
> As-is, I doubt the host package installs anything in the HOST_DIR.
>

The flatcc executable is a schema compiler. This is a tool that needs
to be built for the host to run on the host in order to build packages
that use this tool. It very much installs flatcc in HOST_DIR,
specifically: '$(HOST_DIR)/usr/bin/flatcc'.

In short, both target and host package are mandatory for flatcc to be
a functional BR package.

If you'd like to learn more about this package, I refer you to the
project's github readme:
https://github.com/dvidelabs/flatcc



Anyway - I was asked to address the lack of the shared + static case.
Thus I did. Different opinions exist between the various users and
maintainers of BR. That's fine and healthy. However, I don't want the
addition of this package to be caught between a damned if you
do/damned if you don't disagreement on this subject. The package now
supports static, shared and shared+static. It builds in all three. It
builds even if the configuration doesn't select a C++ compiler (which
I only caught when trying to test this). All in all, I think it's an
improvement.

- Steve

On 05/02/16 23:35, Steve deRosier wrote:
> Hi Samuel,
>
> Thanks for taking a detailed look at this. Hopefully my answers below
> will address your concerns.
>
>
> On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com> wrote:
>>> +
>>> ++# Options to control if we build static or shared libraries. Needed because
>>> ++# cmake has us explicitly do both versions if we want both versions.
>>> ++option(FLATCC_WITH_STATIC "Build the static version of the library" ON)
>>> ++option(FLATCC_WITH_SHARED "Build the shared version of the library" ON)
>> I'm not a big fan of this because:
>> - it kinda adds some feature to flatcc;
>> - it completely by-passes the standard CMake way of driving
>> shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
>> infrastructure automaticllay set [1].
>>
>
> I wish you spoke up two weeks ago when Arnout explicitly asked me to
> add the shared+static to the build. You could've saved me a good week
> of work and two weeks delay in getting support for this upstreamed.

  Well, I was just talking about your custom install commands. I didn't realize 
that in the shared+static case, the static libs weren't even built. I just 
noticed in your code that for a config variable that can have three values 
(static, shared, static+shared) you only handled two (static, others). So I made 
the remark that you forgot about the static+shared case, with a suggestion about 
how it could be handled.

[snip]

  Regards,
  Arnout

On Tue, May 3, 2016 at 10:23 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 05/02/16 23:35, Steve deRosier wrote:
>>
>> Hi Samuel,
>>
>> Thanks for taking a detailed look at this. Hopefully my answers below
>> will address your concerns.
>>
>>
>> On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com>
>> wrote:
>>>>
>>>> +
>>>> ++# Options to control if we build static or shared libraries. Needed
>>>> because
>>>> ++# cmake has us explicitly do both versions if we want both versions.
>>>> ++option(FLATCC_WITH_STATIC "Build the static version of the library"
>>>> ON)
>>>> ++option(FLATCC_WITH_SHARED "Build the shared version of the library"
>>>> ON)
>>>
>>> I'm not a big fan of this because:
>>> - it kinda adds some feature to flatcc;
>>> - it completely by-passes the standard CMake way of driving
>>> shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
>>> infrastructure automaticllay set [1].
>>>
>>
>> I wish you spoke up two weeks ago when Arnout explicitly asked me to
>> add the shared+static to the build. You could've saved me a good week
>> of work and two weeks delay in getting support for this upstreamed.
>
>
>  Well, I was just talking about your custom install commands. I didn't
> realize that in the shared+static case, the static libs weren't even built.
> I just noticed in your code that for a config variable that can have three
> values (static, shared, static+shared) you only handled two (static,
> others). So I made the remark that you forgot about the static+shared case,
> with a suggestion about how it could be handled.
>

OK, understood. I guess I could've responded then that it doesn't
build both and put some code in there to eliminate that case. Sorry, I
misunderstood the review.

But, now the work's been done to handle the extra case. I'd like see
it go in with the extra functionality as we're going to upstream the
feature anyway.

@Samuel and @Arnout, does that plan sound OK? Can we move forward with
this?  Or are there other things that I need to address?

Thanks,
- Steve

On 05/04/16 18:12, Steve deRosier wrote:
> On Tue, May 3, 2016 at 10:23 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
>> On 05/02/16 23:35, Steve deRosier wrote:
>>>
>>> Hi Samuel,
>>>
>>> Thanks for taking a detailed look at this. Hopefully my answers below
>>> will address your concerns.
>>>
>>>
>>> On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com>
>>> wrote:
>>>>>
>>>>> +
>>>>> ++# Options to control if we build static or shared libraries. Needed
>>>>> because
>>>>> ++# cmake has us explicitly do both versions if we want both versions.
>>>>> ++option(FLATCC_WITH_STATIC "Build the static version of the library"
>>>>> ON)
>>>>> ++option(FLATCC_WITH_SHARED "Build the shared version of the library"
>>>>> ON)
>>>>
>>>> I'm not a big fan of this because:
>>>> - it kinda adds some feature to flatcc;
>>>> - it completely by-passes the standard CMake way of driving
>>>> shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
>>>> infrastructure automaticllay set [1].
>>>>
>>>
>>> I wish you spoke up two weeks ago when Arnout explicitly asked me to
>>> add the shared+static to the build. You could've saved me a good week
>>> of work and two weeks delay in getting support for this upstreamed.
>>
>>
>>  Well, I was just talking about your custom install commands. I didn't
>> realize that in the shared+static case, the static libs weren't even built.
>> I just noticed in your code that for a config variable that can have three
>> values (static, shared, static+shared) you only handled two (static,
>> others). So I made the remark that you forgot about the static+shared case,
>> with a suggestion about how it could be handled.
>>
>
> OK, understood. I guess I could've responded then that it doesn't
> build both and put some code in there to eliminate that case. Sorry, I
> misunderstood the review.
>
> But, now the work's been done to handle the extra case. I'd like see
> it go in with the extra functionality as we're going to upstream the
> feature anyway.
>
> @Samuel and @Arnout, does that plan sound OK? Can we move forward with
> this?  Or are there other things that I need to address?

  Hm, tricky...

  On the one hand, I don't want to block your submission over this issue.

  On the other hand, we don't want to carry a feature patch that may never be 
accepted upstream.

  Since the patch wasn't OK yet as it was (it should look at the global 
BUILD_SHARED_LIBS), isn't it easier to just drop that patch, and revert the 
installation commands to your original version, adding a comment that in the 
SHARED_STATIC case only shared libs are built anyway? Or is that too frustrating 
because you did redundant work?

  I can understand that it's frustrating. Actually, reading back what I wrote 
yesterday, I'm afraid that I may have made it worse by making you feel it was 
your own fault. But that's not at all the case: I haven't been sufficiently 
clear in my original review, and you can't magically know all the constraints 
that we try to satisfy.

  That said, a bit of frustration is going to be unavoidable here... I'm sorry 
we haven't been a better upstream for you. And to make it worse, it looks like 
your first submission has been even more unappreciated...

  Regards,
  Arnout

On Wed, May 4, 2016 at 10:02 AM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 05/04/16 18:12, Steve deRosier wrote:
>> @Samuel and @Arnout, does that plan sound OK? Can we move forward with
>> this?  Or are there other things that I need to address?
>
>
>  Hm, tricky...
>
>  On the one hand, I don't want to block your submission over this issue.
>
>  On the other hand, we don't want to carry a feature patch that may never be
> accepted upstream.
>
>  Since the patch wasn't OK yet as it was (it should look at the global
> BUILD_SHARED_LIBS), isn't it easier to just drop that patch, and revert the
> installation commands to your original version, adding a comment that in the
> SHARED_STATIC case only shared libs are built anyway? Or is that too
> frustrating because you did redundant work?
>
>  I can understand that it's frustrating. Actually, reading back what I wrote
> yesterday, I'm afraid that I may have made it worse by making you feel it
> was your own fault. But that's not at all the case: I haven't been
> sufficiently clear in my original review, and you can't magically know all
> the constraints that we try to satisfy.
>
>  That said, a bit of frustration is going to be unavoidable here... I'm
> sorry we haven't been a better upstream for you. And to make it worse, it
> looks like your first submission has been even more unappreciated...
>

Frustrating, sure, a bit. It's the things you think are going to be
easy and straightforward that always seem to the most frustrating. But
hey, I've upstreamed to the kernel and other things in the past, so I
know these things happen and I'll get over it. That's why we
communicate about these things. ;)

How about this: I'll work with Mikkel a bit and see what we can do
upstream in flatcc to make it a bit more buildroot compatible so I
don't have to do heroic measures to make it work. Then I'll resubmit
for v0.3.4 or .5 and get it in. For now I'll just have to live with my
version in my buildroot fork so we can get our work done on v0.3.3.

So, please don't merge v2 of this patch. I'll try again in a few weeks.

- Steve

Hi Steve,

On May 2, 2016 11:35 PM, "Steve deRosier" <derosier@gmail.com> wrote:
>
> Hi Samuel,
>
> Thanks for taking a detailed look at this. Hopefully my answers below
> will address your concerns.
>
>
> On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com> wrote:
> >> +
> >> ++# Options to control if we build static or shared libraries. Needed because
> >> ++# cmake has us explicitly do both versions if we want both versions.
> >> ++option(FLATCC_WITH_STATIC "Build the static version of the library" ON)
> >> ++option(FLATCC_WITH_SHARED "Build the shared version of the library" ON)
> > I'm not a big fan of this because:
> > - it kinda adds some feature to flatcc;
> > - it completely by-passes the standard CMake way of driving
> > shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
> > infrastructure automaticllay set [1].
> >
>
> I wish you spoke up two weeks ago when Arnout explicitly asked me to
> add the shared+static to the build. You could've saved me a good week
> of work and two weeks delay in getting support for this upstreamed.
Indeed. I missed that one... Sorry about that :-s


>
> As you note, CMake can only do static OR shared libraries without
> specific supporting code in the project's CMakeLists file. The change
> I made is a common idiom to support both at the same time. As I was
> asked to add support for buildroot's static + shared if I could, I
> chose to do so.
>
> As for adding a feature to flatcc, that's intended. This will be
> upstreamed to flatcc, however I don't want to wait for the maintainer
> to do a v0.3.4 or greater release with my patch in it.
Fair enough.

>
>
>
> > On this latter point, this is a true limitation of CMake compared to autotools:
> > CMake only provide one boolean flag to control the shared libs. build,
> > instead of a tristate option allowing building: shared libs only, or
> > static libs only, or shared and static libs.
> >
>
> I can't change CMake, but I could easily adjust flatcc to do this.
> Hence I chose to do it this way.
>
>
> > I tend to give priority to the shared libs build (and this is what the
> > infra does [1]).
> >
> > IMO (that does not include other developers), I tend to understand the
> > "shared and static lib" option like this:
> > "do your best, build some libs, shared or static or both, I don't
> > really, but give me something I can use."
>
> Sure. If I left it the way it was, the shared+static was totally
> broken. The interpretation I have of the rules and what was suggested
> to me was it isn't a "do your best", it's a "do as I tell you", which
> is build _both_.
Sorry if we were not clear enough :-s

At least, this discussion will clarify this point, and maybe align BR
developers on the same line.

>
>
> >
> >> ++
> >> + # Disable if cross compiling or if experiencing build issues with certain
> >> + # custom CMake configurations - suspect dependency issue on flatcc tool
> >> + # in custom build steps used by tests and samples.
> >> +@@ -66,6 +71,10 @@ option (FLATCC_ALLOW_WERROR "allow -Werror to be configured" ON)
> >> + # try using this option.
> >> + option (FLATCC_IGNORE_CONST_COND "silence const condition warnings" OFF)
> >> +
> >> ++if (NOT (FLATCC_WITH_STATIC OR FLATCC_WITH_SHARED))
> >> ++      message(FATAL_ERROR "Makes no sense to compile with neither static nor shared libraries.")
> >> ++endif()
> > In such a case, you could check for BUILD_SHARED_LIBS flags...
> >
> > Or the other way around, check first for BUILD_SHARED_LIB, then set
> > FLATCC_WITH_STATIC and/or FLATCC_WITH_SHARED from it.
> >
>
> Setting BUILD_SHARED_LIBS only causes cmake to build shared libs.
> BUILD_STATIC_LIBS causes cmake to build static.
Hmm, I doubt the BUILD_STATIC_LIBS has any effect;.
There is no occurrence of the symbol in the cmake source code.

> Setting both only
> resulted in one working (sorry, can't recall which one off the top of
> my head). No matter how you cut it, I had to modify flatcc to get both
> built. Trust me, I tried everything I could think of to get this to
> work without modifying flatcc. I couldn't trust cmake, and I had to
> make changes, and this is a pattern that other BR projects happen to
> use. So I chose to use it.
>
> As for your second item, it was harmless to flatcc to build both by
> default. If someone explicitly turns both off, then we should tell
> them so, not ignore what they've asked and silently choose something
> other than they explicitly asked.
>
>
> >> +
> >> +# flatcc's cmake build doesn't include install targets, so we need to manually
> >> +# install the various components to their respective destinations. There's
> >> +# several cases that need to be taken care of, so we do a little dance to do so.
> > Sad, no install rules at all in this package. :-(
> >
>
> Mikkel has already corrected this upstream with our input but it is
> not available up through the current release (v0.3.3). For now, this
> is easy enough to do manually, and I'll remove it when we upgrade to a
> release that includes the feature. After I verify it does what I want
> for both the host and target cases.
Then, just backport the patch.

This way, it is less pain when bumping the package to check or detect
that some patches are no longer needed, and does not require heavy
patches on the *.mk file.

>
> > Below, I don't see any install rules for the executables. Are they
> > useless? or for test purpose?
> > Or maybe they are correctly handle by the build system; in such a
> > case, a comment would be welcome ;-)
>
> Well, flatcc contains two things:
> * A library
> * A flatbuffer schema compiler
>
> There is no executable created/needed for the target build.
> The host build only needs the compiler.
Well, in this case, I would have installed everything in the host and
target trees using as much as possible what the package's the
build-system offers (usually 'make install ...'), and add the hooks
removing the compiler in the target tree, and the libs. in the host
tree.

So, if flatcc correctly implements the install target, you won't have
to override the install commands, just implement the hooks.

>
> I thought both the commit message and what flatcc.mk installs makes it
> pretty clear. Perhaps I was wrong about that?
Well... mea culpa. I overlooked the commit log, and I resumed the
review several times before sending it... so forgot the commit log
details.
Regarding this point, the commit log is ok.

>
>
> >> +define FLATCC_INSTALL_STAGING_SHARED
> >> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(STAGING_DIR)/usr/lib/libflatcc.so
> >> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(STAGING_DIR)/usr/lib/libflatccrt.so
> >> +endef
> >> +endif
> > You add a patch to the cmake code to build static/shared libs, why not
> > doing the same for the install rules?
> >
>
> Sure, I suppose I could have. However:
> * There is already a patch upstream for this (though I've yet to check
> it's suitability for a xcompile env where we need both host and target
> builds).
Just backport the patch.

> * The install is easy to do from the flatcc.mk without changing flatcc
But it is fragile to package update...

> * I want to be sure to install flatcc for the host, but not for the
> target (because such thing is useless)
Hooks are here for this purpose;-)

> * I want to be sure to install the libraries in the right places for the target
IMO, it is part of the job of the project itself, not really the
package integrators one.

>
> Also - while you're right, I could've added a patch to flatcc, I
> didn't want to unnecessary add stuff to flatcc itself. The difference
> between the install vs the shared/static is it's impossible to do the
> shared/static with only changes to flatcc.mk.

Well, few rules we try to follow in Buildroot:
- submit upstream patches fixing packages (missing/broken install
rules, cross-compilation failure, arch. support, etc), so this can
lower our job on the next package bump;
- limit the "hack" we do in the package's *.mk or via patches to stuff
we know upstream don't want or cannot be upstreamed.

So, if something can be done upstream and in Buildroot, it is better
to do it upstream (as patches, preferrably the backported/submitted
ones) ;-)

>
>
> >> +
> >> +# If we don't have the shared libs to install, don't run target install
> >> +ifeq ($(FLATCC_INSTALL_STAGING_SHARED),)
> > How about using a more straight forward logic?
> > ifeq ($(BR2_STATIC_LIBS),y)
> >
>
> Because doing otherwise was more complex for the shared+static case.
> This seemed more straight-forward to me: "if we don't build the shared
> libs, then don't try to install them". I suppose it's a matter of
> preference.
>
>
> >> +FLATCC_INSTALL_TARGET = NO
> >> +endif
> >> +
> >> +define FLATCC_INSTALL_TARGET_CMDS
> >> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatcc.so $(TARGET_DIR)/usr/lib/libflatcc.so
> >> +       $(INSTALL) -D -m 0755 $(@D)/lib/libflatccrt.so $(TARGET_DIR)/usr/lib/libflatccrt.so
> >> +endef
> >> +
> >> +$(eval $(cmake-package))
> >> +$(eval $(host-cmake-package))
> > What is the purpose of the host package?
> > As-is, I doubt the host package installs anything in the HOST_DIR.
> >
>
> The flatcc executable is a schema compiler. This is a tool that needs
> to be built for the host to run on the host in order to build packages
> that use this tool. It very much installs flatcc in HOST_DIR,
> specifically: '$(HOST_DIR)/usr/bin/flatcc'.
>
> In short, both target and host package are mandatory for flatcc to be
> a functional BR package.
>
> If you'd like to learn more about this package, I refer you to the
> project's github readme:
> https://github.com/dvidelabs/flatcc
>
>
>
> Anyway - I was asked to address the lack of the shared + static case.
> Thus I did. Different opinions exist between the various users and
> maintainers of BR. That's fine and healthy. However, I don't want the
> addition of this package to be caught between a damned if you
> do/damned if you don't disagreement on this subject. The package now
> supports static, shared and shared+static. It builds in all three. It
> builds even if the configuration doesn't select a C++ compiler (which
> I only caught when trying to test this). All in all, I think it's an
> improvement.

Generally speaking, we prefer dealing with patches already merged
upstream (or at least submitted upstream), than recode the same things
in the *.mk files.

About the shared+static librairy build support, IMHO, it's better to
stick to what the upstream does, i.e.:
- if upstream already offers a support for building static and shared
libraries at the same time, then use what the upstream offers; fix it
if needed;
- otherwise, do not try to do add complex stuff in buildroot (either
patching the sources, or implementing some package's build-system
lacks in the Buildroot's *.mk files). We don't usually like carrying
feature patches (unless they are already merged upstream or about to
be), and we try hard to keep things as simple as possible in Buildroot
;-). So, just build shared libraries in that cases. It won't be the
first package doing that, nor the last one ;-)

This could certainly be mention somewhere in the doc as "What to do
with package not supporting building shared and static libraries in
the same build".

Regards,

Samuel

Hi Samuel,

On Wed, May 4, 2016 at 1:53 PM, Samuel Martin <s.martin49@gmail.com> wrote:
> Hi Steve,
[snip]
>
> About the shared+static librairy build support, IMHO, it's better to
> stick to what the upstream does, i.e.:
> - if upstream already offers a support for building static and shared
> libraries at the same time, then use what the upstream offers; fix it
> if needed;
> - otherwise, do not try to do add complex stuff in buildroot (either
> patching the sources, or implementing some package's build-system
> lacks in the Buildroot's *.mk files). We don't usually like carrying
> feature patches (unless they are already merged upstream or about to
> be), and we try hard to keep things as simple as possible in Buildroot
> ;-). So, just build shared libraries in that cases. It won't be the
> first package doing that, nor the last one ;-)

Our emails probably crossed.  Suffice it to say, I agree with you -
simple is better that's my preference anyway. I'll try again after I
iron out some issues upstream on flatcc.

- Steve

Steve, Arnout,

On Wed, May 4, 2016 at 7:02 PM, Arnout Vandecappelle <arnout@mind.be> wrote:
> On 05/04/16 18:12, Steve deRosier wrote:
>>
>> On Tue, May 3, 2016 at 10:23 AM, Arnout Vandecappelle <arnout@mind.be>
>> wrote:
>>>
>>> On 05/02/16 23:35, Steve deRosier wrote:
>>>>
>>>>
>>>> Hi Samuel,
>>>>
>>>> Thanks for taking a detailed look at this. Hopefully my answers below
>>>> will address your concerns.
>>>>
>>>>
>>>> On Mon, May 2, 2016 at 12:44 PM, Samuel Martin <s.martin49@gmail.com>
>>>> wrote:
>>>>>>
>>>>>>
>>>>>> +
>>>>>> ++# Options to control if we build static or shared libraries. Needed
>>>>>> because
>>>>>> ++# cmake has us explicitly do both versions if we want both versions.
>>>>>> ++option(FLATCC_WITH_STATIC "Build the static version of the library"
>>>>>> ON)
>>>>>> ++option(FLATCC_WITH_SHARED "Build the shared version of the library"
>>>>>> ON)
>>>>>
>>>>>
>>>>> I'm not a big fan of this because:
>>>>> - it kinda adds some feature to flatcc;
>>>>> - it completely by-passes the standard CMake way of driving
>>>>> shared/static libs build (using BUILD_SHARED_LIBS) that the Buildroot
>>>>> infrastructure automaticllay set [1].
>>>>>
>>>>
>>>> I wish you spoke up two weeks ago when Arnout explicitly asked me to
>>>> add the shared+static to the build. You could've saved me a good week
>>>> of work and two weeks delay in getting support for this upstreamed.
>>>
>>>
>>>
>>>  Well, I was just talking about your custom install commands. I didn't
>>> realize that in the shared+static case, the static libs weren't even
>>> built.
>>> I just noticed in your code that for a config variable that can have
>>> three
>>> values (static, shared, static+shared) you only handled two (static,
>>> others). So I made the remark that you forgot about the static+shared
>>> case,
>>> with a suggestion about how it could be handled.
>>>
>>
>> OK, understood. I guess I could've responded then that it doesn't
>> build both and put some code in there to eliminate that case. Sorry, I
>> misunderstood the review.
>>
>> But, now the work's been done to handle the extra case. I'd like see
>> it go in with the extra functionality as we're going to upstream the
>> feature anyway.
>>
>> @Samuel and @Arnout, does that plan sound OK? Can we move forward with
>> this?  Or are there other things that I need to address?

IMHO, the install rule fix is more critical than the shared+static libs support.
For backported patches, it is ok.
For feature patches: it depends how big are the patches, if they have
been submitted, how confident we/you feel it is gonna make it
upstream... But when the submitter is actively working with upstream
to merge the patches, I don't see any reasons to obstruct it
integration in BR..

>
>ed
>  Hm, tricky...
>
>  On the one hand, I don't want to block your submission over this issue.
>
>  On the other hand, we don't want to carry a feature patch that may never be
> accepted upstream.

Well, your first submission is by far simpler than the second one!

>
>  Since the patch wasn't OK yet as it was (it should look at the global
> BUILD_SHARED_LIBS), isn't it easier to just drop that patch, and revert the
> installation commands to your original version, adding a comment that in the
> SHARED_STATIC case only shared libs are built anyway? Or is that too
> frustrating because you did redundant work?

I would suggest to make flatcc get in BR with a simple patch (just
backport the install rule fix and a trivial shared+static libs support
behaving as the shared only libs one, with some comments).
In parallel, keep working with upstream. Once this is merged upstream,
it will be easier for you to update the package in BR.

>
>  I can understand that it's frustrating. Actually, reading back what I wrote
> yesterday, I'm afraid that I may have made it worse by making you feel it
> was your own fault. But that's not at all the case: I haven't been
> sufficiently clear in my original review, and you can't magically know all
> the constraints that we try to satisfy.

Steve, I'm truly sorry if you feel frustrated about this, don't be!
Your work is valuable, and definitely not a waste of time. Just BR was
not the right place were to submit this ;-)
In the end, what is important is that things get in the place where
they belong, and that everyone can benefit from them.

Regards,

Hi all,

[snip]

What the status of this patch ?

Steve you said that you'll send an updated version:
"So, please don't merge v2 of this patch. I'll try again in a few weeks."

I'll mark it changes requested in the patchwork, please don't forget to send an
updated version.

Best regards,
Romain