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

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

Hello Jan,

On Sun, 20 Feb 2022 15:08:11 +0100
Jan Havran <havran.jan@email.cz> wrote:

> RTKLIB is an open source program package for standard and precise
> positioning with GNSS.
> 
> Version used here is from rtkexplorer, which is better optimized
> and more frequently updated than original version by Tomoji Takasu.
> 
> Signed-off-by: Jan Havran <havran.jan@email.cz>

Thanks for your contribution, and sorry for the huge delay in getting
back to you.

I reviewed this today, but unfortunately there are too many things that
are wrong in rtklib itself (not in your patch).

First and foremost, the Github repository comes with pre-compiled code:
lib/iers/gcc/iers.a and lib/openblas/libopenblas.a.

Then, when you compile the rnx2rtkp application, it links with iers.a
(precompiled). I was surprised that this could work, because iers.a
contains x86-64 objects, while I was cross-compiling for ARM.

Turns that our that the rnx2rtkp links the executable with the iers.a
library... but doesn't use any symbols of it. It also links with
libgfortran for no reason (because it's iers.a that contains Fortran
code).

Of course, this means it "works", but it gives a very very bad feeling
about the rtklib build system overall.

Finally, the makefile machinery in rtklib doesn't allow passing custom
CFLAGS to amend/override the CFLAGS defined by the rtklib makefiles
themselves. So Buildroot optimization/debug flags are not taken into
account.

If you're still interested by this package, could you have a look, and
perhaps work out these issues upstream?


> diff --git a/package/rtklib/rtklib.hash b/package/rtklib/rtklib.hash
> new file mode 100644
> index 0000000000..faed92243b
> --- /dev/null
> +++ b/package/rtklib/rtklib.hash
> @@ -0,0 +1,3 @@
> +# Locally computed
> +sha256  735d43939ae08b0da64d75afe5750bc983032144c5622e620eee987508946fc5  rtklib-b34e.tar.gz
> +sha256  219747832d49ee958457b2934080ab8d94bd9d8e45fcb1c36f89776fd2c5ed8a  license.txt
> diff --git a/package/rtklib/rtklib.mk b/package/rtklib/rtklib.mk
> new file mode 100644
> index 0000000000..421cdddfcd
> --- /dev/null
> +++ b/package/rtklib/rtklib.mk
> @@ -0,0 +1,31 @@
> +################################################################################
> +#
> +# rtklib
> +#
> +################################################################################
> +
> +RTKLIB_VERSION = b34e
> +RTKLIB_SITE = $(call github,rtklibexplorer,RTKLIB,$(RTKLIB_VERSION))
> +RTKLIB_LICENSE = BSD-2-Clause
> +RTKLIB_LICENSE_FILES = license.txt
> +
> +RTKLIB_APPS += $(if $(BR2_PACKAGE_RTKLIB_CONVBIN),convbin,)
> +RTKLIB_APPS += $(if $(BR2_PACKAGE_RTKLIB_POS2KML),pos2kml,)
> +RTKLIB_APPS += $(if $(BR2_PACKAGE_RTKLIB_RNX2RTKP),rnx2rtkp,)
> +RTKLIB_APPS += $(if $(BR2_PACKAGE_RTKLIB_RTKRCV),rtkrcv,)
> +RTKLIB_APPS += $(if $(BR2_PACKAGE_RTKLIB_STR2STR),str2str,)

Can be simplified as:

RTKLIB_APPS = \
        $(if $(BR2_PACKAGE_RTKLIB_CONVBIN),convbin) \
        $(if $(BR2_PACKAGE_RTKLIB_POS2KML),pos2kml) \
        $(if $(BR2_PACKAGE_RTKLIB_RNX2RTKP),rnx2rtkp) \
        $(if $(BR2_PACKAGE_RTKLIB_RTKRCV),rtkrcv) \
        $(if $(BR2_PACKAGE_RTKLIB_STR2STR),str2str)

> +define RTKLIB_BUILD_CMDS
> +	for APP in $(RTKLIB_APPS); do \
> +		cd $(@D)/app/consapp/$$APP/gcc ; \
> +		$(TARGET_MAKE_ENV) $(MAKE) CC="$(TARGET_CC)" CCFLAGS="$(TARGET_CFLAGS) $(TARGET_LDFLAGS)" ; \
> +	done
> +endef

Instead of a shell "for" loop, using a make "foreach" loop, so that the
loops abort if there is a failure.

Instead of cd-ing into the directory, use make -C option.

CCFLAGS is not used anywhere in the rtklib build system, so passing
CCFLAGS has no effect.

At the end of my experiments, I had:

define RTKLIB_BUILD_CMDS
	find $(@D) -name '*.a' | xargs rm -f
	$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/lib/iers/gcc \
	 	CC="$(TARGET_CC)" \
	 	F77="$(TARGET_FC)"
	$(foreach app,$(RTKLIB_APPS),\
		$(TARGET_MAKE_ENV) $(MAKE) -C $(@D)/app/consapp/$(app)/gcc \
			CC="$(TARGET_CC)"
	)
endef

Which got rid of pre-compiled .a files, and rebuilds iers.a from
source. That's when I understood/discovered that iers.a was not even
used, and decided the rtklib build system was definitely too broken.

> +define RTKLIB_INSTALL_TARGET_CMDS
> +	for APP in $(RTKLIB_APPS); do \
> +		$(INSTALL) -m 0755 $(@D)/app/consapp/$$APP/gcc/$$APP $(TARGET_DIR)/usr/bin/$$APP ; \
> +	done
> +endef

I had replaced this with:

define RTKLIB_INSTALL_TARGET_CMDS
	$(foreach app,$(RTKLIB_APPS), \
		$(INSTALL) -m 0755 $(@D)/app/consapp/$(app)/gcc/$(app) \
			$(TARGET_DIR)/usr/bin/$(app)
	)
endef

also to use a make "foreach" loop.

Could you have a look at those issues, and if you're interested, send
an updated patch?

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