frr: fix host build error on macOS #24053

httpstorm · 2024-04-30T15:51:13Z

Fixes:

make package/feeds/packages/frr/host/{clean,compile} -j 1 V=s
…
lib/command_graph.c:16:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma DEFINE_MTYPE_STATIC(LIB, CMD_TOKENS, "Command Tokens"); ^
./lib/memory.h:139:2: note: expanded from macro 'DEFINE_MTYPE_STATIC'
        DEFINE_MTYPE_ATTR(group, name, static, desc)                           \
        ^
./lib/memory.h:109:26: note: expanded from macro 'DEFINE_MTYPE_ATTR'
                __attribute__((section(".data.mtypes"))) = { {                 \

Maintainer: @lucize
Compile tested: macOS 14.4.1, Ubuntu 22.04.4 LTS

Changes from the original version [1]: refreshed by @robimarko and fixed formatting style (me) to meet the requirements for sending upstream [2].
[1] FRRouting/frr#6032
[2] FRRouting/frr#15890

@neheb @1715173329 @PolynomialDivision @dhewg @hnyman

robimarko · 2024-04-30T17:30:50Z

Seems upstream likes it as well, so lets give CI some time though I think it will fail at least on some archs since SDKs have not been rebuilt

Fixes: lib/command_graph.c:16:1: error: argument to 'section' attribute is not valid for this target: mach-o section specifier requires a segment and section separated by a comma DEFINE_MTYPE_STATIC(LIB, CMD_TOKENS, "Command Tokens"); ^ ./lib/memory.h:139:2: note: expanded from macro 'DEFINE_MTYPE_STATIC' DEFINE_MTYPE_ATTR(group, name, static, desc) \ ^ ./lib/memory.h:109:26: note: expanded from macro 'DEFINE_MTYPE_ATTR' __attribute__((section(".data.mtypes"))) = { { \ [1] FRRouting/frr#6032 [2] FRRouting/frr#15890 Signed-off-by: Georgi Valkov <gvalkov@gmail.com>

httpstorm · 2024-04-30T17:40:31Z

@robimarko
I made a small update to match the changes I was asked to make upstream. The tests will have to run again.

robimarko · 2024-04-30T17:41:28Z

They havent even started so no problem

lucize · 2024-04-30T17:42:32Z

sorry for no input, but have tested some weeks on frr10, guess the issues is the same there also

neheb · 2024-04-30T20:52:19Z

multiple definition of `xstrdup'; /builder/staging_dir/host/lib/libelf.a(libgnu_la-xmalloc.o):xmalloc.c:(.text+0x260): first defined here

Probably have to wait a day before the elfutils fix gets propagated.

robimarko · 2024-04-30T20:53:42Z

Some SDK-s have been rebuilt, some not so that is why some archs passed while others failed.

httpstorm · 2024-05-01T16:13:37Z

@robimarko @neheb @trippleflux
The patch is now upstream.
But there's an unrelated problem with frr. The host builds fine. However the target package fails. Same error on macOS and Ubuntu. I tried with and without this patch, makes no difference. I tried building the checkpoint openwrt/openwrt@5a028a8 right before we made changes to gnulib, elfutils and friends. Same error on macOS. Ubuntu it is still building, it might take a couple of hours. But it's likely unrelated to Tony's PR. I've never used frr so I have no idea of a checkpoint that builds correctly. Usually I do binary search between one good and one bad checkpoint, to find what caused a regression. But now I don't have a good reference. One thing that we might try is updating from release 9.0.0 to 10.0.0.

Long story: after my PRs are accepted I like cloning and building from zero. 3 architectures. And then I flash my routers to make sure all is well. My configuration builds without any issues. Then I built any affected packages and collect some logs, because Rosen mentioned this and our runners were not available at the time:

multiple definition of `xstrdup'; /builder/staging_dir/host/lib/libelf.a(libgnu_la-xmalloc.o):xmalloc.c:(.text+0x260): first defined here

No this issue is no longer present. Logs below.

./scripts/diffconfig.sh

CONFIG_TARGET_x86=y
CONFIG_TARGET_x86_64=y
CONFIG_TARGET_x86_64_DEVICE_generic=y
CONFIG_FRR_INTERNAL=y
CONFIG_FRR_NO_SNMP=y
CONFIG_MOLD=y
CONFIG_PACKAGE_frr=y
CONFIG_PACKAGE_frr-libfrr=y
CONFIG_PACKAGE_frr-watchfrr=y
CONFIG_PACKAGE_frr-zebra=y
CONFIG_PACKAGE_libatomic=y
CONFIG_PACKAGE_libcap=y
CONFIG_PACKAGE_libpcre2=y
CONFIG_PACKAGE_libprotobuf-c=y
CONFIG_PACKAGE_libyang=y
CONFIG_PCRE2_JIT_ENABLED=y
CONFIG_MOLD=y
# CONFIG_USE_MOLD is not set

make package/feeds/packages/frr/{clean,compile} -j 1 V=s
…
lib/vty.c: In function 'vty_mgmt_resume_response':
lib/vty.c:195:27: error: 'VTYSH_READ' undeclared (first use in this function); did you mean 'VTY_READ'?
  195 |                 vty_event(VTYSH_READ, vty);
      |                           ^~~~~~~~~~
      |                           VTY_READ

robimarko · 2024-05-01T16:22:43Z

@httpstorm Yes, I saw the same build error, but I dont think its related at all to host tooling.
I tried going back to before elfutils and those changes and it failed the same.

httpstorm · 2024-05-01T18:49:34Z

@robimarko
Yes, the Ubuntu build before Tony's PR failed with the same error. So we know it's unrelated.
I'm trying version 10. Host builds. Package requires update to libyang >= 2.1.128.

Edit: lib updates and builds fine. frr-10 fails with the same errors.
Edit: same errors on master. I gave we have to look into the source code.

httpstorm · 2024-05-01T20:09:31Z

@robimarko
The first error in code is that not all use cases of the VTYSH_* enums are guarded by #ifdef VTYSH. These enums are enabled by the VTYSH macro, which is defined if sub package frr-vtysh is enabled in menuconfig. This is also a configuration issue and can be resolved by making sure the package always enabled in the Makefile.
There are more errors in vey.c.

lucize · 2024-05-01T21:40:23Z

@httpstorm if you have a bit of patience I'll push the 10 version, but I've tested it with older SDK from a few weeks ago, I have to see with the new changes

httpstorm · 2024-05-02T02:35:26Z

@lucize @trippleflux
Please send diffconfig. In particularly select only the architecture and frr packages you normally build. Also the commit hash that you have compiled successfully. The first error was resolved by selecting a frr-vtysh. That indicates missing dependency in net/frr/Makefile. It's possible that it will build if some of others are also enabled.
https://openwrt.org/docs/guide-developer/toolchain/use-buildsystem#creating_diff_file

# Write the changes to diffconfig
./scripts/diffconfig.sh > diffconfig

httpstorm · 2024-05-02T03:37:30Z

@lucize

@httpstorm if you have a bit of patience I'll push the 10 version, but I've tested it with older SDK from a few weeks ago, I have to see with the new changes

Yes, it would be better if you do it. I'm new to this package. What environment is required to run and test it?

robimarko · 2024-05-02T08:20:56Z

For me as soon as vtysh is also selected it builds just fine

httpstorm · 2024-05-02T08:28:24Z

@robimarko

This solves it by selecting frr-vtysh by default. So it's all we need on version 9.0.0. I don't see a package quagga and the default selection can later be turned off. It is not a hard dependency. Do we need to use a statement like y if PACKAGE_frr and not quagga-vtysh? I'm not sure what the correct syntax is and if we need to overcomplicate things, considering we have a CONFLICTS statement?

# net/frr/Makefile
 define Package/frr-vtysh
   $(call Package/frr/Default)
   DEPENDS+=+frr-libfrr +libreadline +libncurses +more
   TITLE:=integrated shell for frr routing software
   CONFLICTS:=quagga-vtysh
+  DEFAULT:=y if PACKAGE_frr
 endef

robimarko · 2024-05-02T08:38:28Z

I am not sure that its a proper fix to force vtysh by default, because how it worked before?
There must have been an upstream change that missed the required guards.

Reference: FRRouting/frr#15752

httpstorm · 2024-05-02T08:51:36Z

@robimarko

I am not sure that its a proper fix to force vtysh by default, because how it worked before?
There must have been an upstream change that missed the required guards.
Reference: FRRouting/frr#15752

The only way it could work before is if people know they should enable it. Indeed it's a bug in frr. Maybe it worked before version 9 or whatever checkpoint we are using. The Makefile is a bit messy about the source file and version: shouldn't we have either PKG_RELEASE or PKG_SOURCE_DATE and PKG_SOURCE_VERSION? They can't just rewrite the checkpoint without breaking the hash. So likely someone updated the version and compiled with vtysh so the error didn't show.

Later if we want to move forward to version 10 or master, it gets a bit more complicated.
First we need to update to libyang-2.1.148 (the actual requirement was lower). That's the last version on 2.1.* and is likely safe, but we need to test a few packages that depend on it (list below). Then frr-10 and master both build fine.

However if we update to the latest libyang-2.2.8, there is an API change in struct ly_err_item, defined in libyang/log.h, which introduces the second build error I had:
CESNET/libyang@7a26677#diff-67a354ecb35f341abf57f84bd0d714b05280fddb1063b90a31f0d3ef67675bd7

lib/vty.c:3676:28: error: 'struct ly_err_item' has no member named 'path'
 3676 |         bool have_path = ei->path && ei->path[0] != 0;
      |                            ^~

That would make it more complicated, considering the following packages also depend on libyang:

frr-libfrr frr libnetconf2 libsysrepo netopeer2-cli netopeer2-server yanglint

httpstorm · 2024-05-02T09:02:25Z

@robimarko
It's a quote from a developer: FRRouting/frr#15752 (comment)

From my perspective vtysh is a required component. We should disable the ability to turn this off.

In that case it should be a hard dependency.

Edit: the issue author also has a point: for hacking or reducing the attack surface the less modules there are the better. The same also applies in our case since we are cross-compiling for devices with limited resources.

robimarko · 2024-05-02T09:45:42Z

@httpstorm Can you please open a new issue specific for this build error?

Also, after reading through the FRR issue, I agree that vtysh should be on by default, maybe not even a separate package but just part of the default compile options

httpstorm · 2024-05-02T11:42:03Z

@robimarko
Ready frr: fails to compile when submodule frr-libfrr is not enabled #24063

lucize · 2024-05-02T17:50:25Z

usually we always build vtysh and we didn't see when it got broken I'm sure this is a bug in newer frr version and my approach was to depend it on frr as the package is quite large I don't think people are using this without the built-in vtysh

This was referenced Apr 30, 2024

tools/elfutils: remove dirname openwrt/openwrt#15337

Merged

lib: fix build error on MacOS FRRouting/frr#15890

Merged

httpstorm force-pushed the frr-macOS-section.2024-04-30 branch from e6e1af1 to 5cda1f9 Compare April 30, 2024 17:38

robimarko merged commit 6b7d905 into openwrt:master Apr 30, 2024
6 of 12 checks passed

httpstorm mentioned this pull request May 2, 2024

frr: fails to compile when submodule frr-libfrr is not enabled #24063

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frr: fix host build error on macOS #24053

frr: fix host build error on macOS #24053

httpstorm commented Apr 30, 2024 •

edited

Loading

robimarko commented Apr 30, 2024

httpstorm commented Apr 30, 2024

robimarko commented Apr 30, 2024

lucize commented Apr 30, 2024

neheb commented Apr 30, 2024

robimarko commented Apr 30, 2024

httpstorm commented May 1, 2024

robimarko commented May 1, 2024 •

edited

Loading

httpstorm commented May 1, 2024 •

edited

Loading

httpstorm commented May 1, 2024

lucize commented May 1, 2024

httpstorm commented May 2, 2024

httpstorm commented May 2, 2024

robimarko commented May 2, 2024

httpstorm commented May 2, 2024

robimarko commented May 2, 2024 •

edited

Loading

httpstorm commented May 2, 2024 •

edited

Loading

httpstorm commented May 2, 2024 •

edited

Loading

robimarko commented May 2, 2024

httpstorm commented May 2, 2024 •

edited

Loading

lucize commented May 2, 2024

frr: fix host build error on macOS #24053

frr: fix host build error on macOS #24053

Conversation

httpstorm commented Apr 30, 2024 • edited Loading

robimarko commented Apr 30, 2024

httpstorm commented Apr 30, 2024

robimarko commented Apr 30, 2024

lucize commented Apr 30, 2024

neheb commented Apr 30, 2024

robimarko commented Apr 30, 2024

httpstorm commented May 1, 2024

robimarko commented May 1, 2024 • edited Loading

httpstorm commented May 1, 2024 • edited Loading

httpstorm commented May 1, 2024

lucize commented May 1, 2024

httpstorm commented May 2, 2024

httpstorm commented May 2, 2024

robimarko commented May 2, 2024

httpstorm commented May 2, 2024

robimarko commented May 2, 2024 • edited Loading

httpstorm commented May 2, 2024 • edited Loading

httpstorm commented May 2, 2024 • edited Loading

robimarko commented May 2, 2024

httpstorm commented May 2, 2024 • edited Loading

lucize commented May 2, 2024

httpstorm commented Apr 30, 2024 •

edited

Loading

robimarko commented May 1, 2024 •

edited

Loading

httpstorm commented May 1, 2024 •

edited

Loading

robimarko commented May 2, 2024 •

edited

Loading

httpstorm commented May 2, 2024 •

edited

Loading

httpstorm commented May 2, 2024 •

edited

Loading

httpstorm commented May 2, 2024 •

edited

Loading