add frr package

This option is required by frr package, so enable it by default.

Signed-off-by Vadim Kochan <[email protected]>

Quoted 3 lines · On Mon, 24 Feb 2020 19:26:50 +0200 Vadim Kochan <[email protected]> wrote:

This option is required by frr package, so enable it by default.

Signed-off-by Vadim Kochan <[email protected]>

What is this option doing? How much more space does it require? In other words, do we want to enable it unconditionally, or do we want to make it optional ?

Hello Vadim,

On Mon, 24 Feb 2020 19:26:51 +0200
Vadim Kochan <[email protected]> wrote:

package/rtrlib/Config.in line 4

Quoted 10 lines

package/rtrlib/Config.in new file +4 -0

new file mode 100644
index 0000000000..440eeb5d63
@@ -0,0 +1,13 @@
+config BR2_PACKAGE_RTRLIB
+	bool "rtrlib"
+	depends on BR2_TOOLCHAIN_HAS_THREADS
+	depends on BR2_TOOLCHAIN_HAS_SSP

It's pretty uncommon for a package to require SSP support. Can you tell it instead to not pass -fstack-protector when building, and let Buildroot decide when to pass it ?

package/frr/Config.in new file +3 -0

new file mode 100644
index 0000000000..01673eb837
@@ -0,0 +1,23 @@
+config BR2_PACKAGE_FRR
+	bool "frr"
+	depends on BR2_USE_MMU # fork()

package/frr/S50frr new file +5 -0

new file mode 100644
index 0000000000..dec7d82a88
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# Starts frr.
+#
+

package/frr/frr.mk new file +4 -0

new file mode 100644
index 0000000000..c53e9bb4b0
@@ -0,0 +1,105 @@
+################################################################################
+#
+# frr
+#

package/frr/frr.mk new file +8 -0

+
+HOST_FRR_DEPENDENCIES = host-flex host-bison host-python
+
+FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
+		--sysconfdir=/etc/frr \
+		--localstatedir=/var/run/frr \
+		--with-moduledir=/usr/lib/frr/modules \
+		--enable-configfile-mask=0640 \

package/frr/frr.mk new file +4 -0

+	/etc/frr/vtysh.conf f 644 frr frrvty - - - - -
+endef
+
+define FRR_USERS

Hello Vadim,

On Mon, 24 Feb 2020 19:26:52 +0200
Vadim Kochan <[email protected]> wrote:

DEVELOPERS F: package/brcm-patchram-plus/ · line 2483

Quoted 9 lines

DEVELOPERS +1 -0

index e07236937b..ef6592afa0 100644
@@ -2480,6 +2480,7 @@ F:	package/brcm-patchram-plus/
 F:	package/gettext-tiny/
 F:	package/tinyssh/
 F:	package/rtrlib/
+F:	package/frr/

Alphabetic ordering is not good here.

Quoted 16 lines

package/frr/Config.in new file +9 -0

new file mode 100644
index 0000000000..01673eb837
@@ -0,0 +1,23 @@
+config BR2_PACKAGE_FRR
+	bool "frr"
+	depends on BR2_USE_MMU # fork()
+	depends on BR2_PACKAGE_BASH # init
+	select BR2_PACKAGE_RTRLIB
+	select BR2_PACKAGE_READLINE
+	select BR2_PACKAGE_JSON_C
+	select BR2_PACKAGE_LIBYANG
+	select BR2_PACKAGE_LIBCAP

package/frr/Config.in line 13

Quoted 4 lines

package/frr/Config.in new file +4 -0

+	select BR2_PACKAGE_LIBNL
+	select BR2_PACKAGE_NCURSES
+	select BR2_PACKAGE_NETSNMP
+	select BR2_PACKAGE_C_ARES

That's a lot of packages that you select here, are you sure you properly propagated the "depends on" of all those packages in this Config.in ?

For example, rtrlib depends on threads and SSP, so these need to be propagated here (of course, except if you get rid of the SSP dependency in rtrlib, as suggested in my review of PATCH 2/3).

package/frr/Config.in line 23

Quoted 10 lines

package/frr/Config.in new file +10 -0

+	help
+	  The FRRouting Protocol Suite.
+
+	  FRR is free software that implements and manages various IPv4 and
+	  IPv6 routing protocols.
+
+	  https://frrouting.org
+
+comment "frr requires BASH for init service"
+	depends on !BR2_PACKAGE_BASH

So it's the frrinit.sh script that requires bash ?

This comment also need a BR2_USE_MMU dependency, so that it doesn't show up on noMMU systems.

Quoted 16 lines

package/frr/S50frr new file +10 -0

new file mode 100644
index 0000000000..dec7d82a88
@@ -0,0 +1,38 @@
+#!/bin/sh
+#
+# Starts frr.
+#
+
+start() {
+	printf "Starting frr: "
+	start-stop-daemon -S -q -b \
+		--exec /usr/sbin/frrinit.sh -- start
+	[ $? = 0 ] && echo "OK" || echo "FAIL"

package/frr/S50frr line 15

Quoted 5 lines

package/frr/S50frr new file +5 -0

+}
+stop() {
+	printf "Stopping frr: "
+	/usr/sbin/frrinit.sh stop
+	start-stop-daemon -K -q -p /run/frr.pid

This looks odd. Why do you need to call frrinit.sh stop and then use start-stop-daemon ?

Also, make sure to use package/busybox/S01sysklogd as a template for your init scripts.

Quoted 12 lines

package/frr/frr.mk new file +4 -0

new file mode 100644
index 0000000000..c53e9bb4b0
@@ -0,0 +1,105 @@
+################################################################################
+#
+# frr
+#

package/frr/frr.mk line 14

Quoted 10 lines

package/frr/frr.mk new file +10 -0

+################################################################################
+
+FRR_VERSION = 7.3
+FRR_SOURCE = frr-$(FRR_VERSION).tar.gz
+FRR_SITE = https://github.com/FRRouting/frr/archive
+FRR_LICENSE = GPL-2.0
+FRR_LICENSE_FILES = COPYING
+
+FRR_DEPENDENCIES = rtrlib readline json-c libyang libcap libnl \
+		   ncurses host-frr c-ares

Only one tab to indent the second line.

Quoted 12 lines

package/frr/frr.mk new file +12 -0

+
+HOST_FRR_DEPENDENCIES = host-flex host-bison host-python
+
+FRR_CONF_OPTS = --with-clippy=$(HOST_DIR)/bin/clippy \
+		--sysconfdir=/etc/frr \
+		--localstatedir=/var/run/frr \
+		--with-moduledir=/usr/lib/frr/modules \
+		--enable-configfile-mask=0640 \
+		--enable-logfile-mask=0640 \
+		--enable-multipath=256 \
+		--disable-ospfclient \
+		--enable-shell-access \

package/frr/frr.mk line 32

Quoted 6 lines

package/frr/frr.mk new file +6 -0

+		--enable-user=frr \
+		--enable-group=frr \
+		--enable-vty-group=frrvty \
+		--disable-exampledir \
+		--enable-rpki \
+		--enable-fpm

Ditto, only one tab to indent the continuation lines.

package/frr/frr.mk line 40

Quoted 9 lines

package/frr/frr.mk new file +8 -0

+
+HOST_FRR_CONF_OPTS = --enable-clippy-only
+
+define FRR_RUN_BOOTSTRAP
+	(cd $(@D) && PATH=$(BR_PATH) ./bootstrap.sh)
+endef
+FRR_PRE_CONFIGURE_HOOKS += FRR_RUN_BOOTSTRAP
+HOST_FRR_PRE_CONFIGURE_HOOKS += FRR_RUN_BOOTSTRAP

This won't work as you don't have any guarantee that host-autoconf and host-automake will be built before. Can you use FRR_AUTORECONF = YES, as this is really the best solution ? If not, you need to keep your hooks, but explicitly add host-autoconf and host-automake to the package dependencies.

package/frr/frr.mk line 47

Quoted 7 lines

package/frr/frr.mk new file +6 -0

+define HOST_FRR_INSTALL_CMDS
+	$(INSTALL) -D -m 0755 $(@D)/lib/clippy $(HOST_DIR)/bin/clippy
+endef
+
+
+# for some reason the normal 'install' target fails

Why? Can it be fixed? At least reported to the upstream developers?

package/frr/frr.mk line 52

Quoted 6 lines

package/frr/frr.mk new file +5 -0

+FRR_INSTALL_TARGET_OPTS = DESTDIR="$(TARGET_DIR)" libdir="/usr/lib" \
+			  install-binPROGRAMS \
+			  install-sbinPROGRAMS \
+			  install-sbinSCRIPTS \
+			  installdirs

Only one tab to indent the continuation lines.

package/frr/frr.mk line 55

Quoted 4 lines

package/frr/frr.mk new file +3 -0

+
+define FRR_INSTALL_CONFIG_FILES
+	(cd $(@D) && PATH=$(BR_PATH) ./bootstrap.sh)

Calling bootstrap.sh during the installation? Why?

package/frr/frr.mk line 57

Quoted 3 lines

package/frr/frr.mk new file +2 -0

+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/log/frr
+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/run/frr

I'm not sure this is going to work well, as /var/log and /var/run are symlinks to /tmp by default, and a tmpfs is mounted to /tmp.

These directories need to be created at runtime.

package/frr/frr.mk line 59

Quoted 2 lines

package/frr/frr.mk new file +1 -0

+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/etc/frr

Not needed, as the -D option to the $(INSTALL) commands above will create this folder.

package/frr/frr.mk line 64

Quoted 5 lines

package/frr/frr.mk new file +5 -0

+	$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/daemons $(TARGET_DIR)/etc/frr/daemons
+	$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/daemons.conf $(TARGET_DIR)/etc/frr/daemons.conf
+	$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/frr.conf $(TARGET_DIR)/etc/frr/frr.conf
+	$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/vtysh.conf $(TARGET_DIR)/etc/frr/vtysh.conf
+	$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/support_bundle_commands.conf $(TARGET_DIR)/etc/frr/support_bundle_commands.conf

This probably calls for a for loop:

	$(foreach f,daemons daemons.conf frr.conf vtysh.conf support_bundle_commands.conf,\
		$(INSTALL) -D -m 0644 $(@D)/tools/etc/frr/$(f) \
			$(TARGET_DIR)/etc/frr/$(f)
	)

package/frr/frr.mk line 75

Quoted 11 lines

package/frr/frr.mk new file +10 -0

+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/zebra.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/bgpd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/ospfd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/ospf6d.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/isisd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/ripd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/ripngd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/pimd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/ldpd.conf
+	$(INSTALL) -m 0644 /dev/null $(TARGET_DIR)/etc/frr/nhrpd.conf

Installing /dev/null ? Here is just creates an empty regular file. Does it really makes sense? What are you trying to do here ?

package/frr/frr.mk line 81

Quoted 6 lines

package/frr/frr.mk new file +6 -0

+endef
+FRR_POST_INSTALL_TARGET_HOOKS += FRR_INSTALL_CONFIG_FILES
+
+define FRR_PERMISSIONS
+	/var/log/frr d 755 frr frr - - - - -
+	/var/run/frr d 755 frr frr - - - - -

This is not going to work, as explained above.

package/frr/frr.mk line 91

Quoted 10 lines

package/frr/frr.mk new file +10 -0

+	/etc/frr/zebra.conf f 644 frr frr - - - - -
+	/etc/frr/bgpd.conf f 644 frr frr - - - - -
+	/etc/frr/ospfd.conf f 644 frr frr - - - - -
+	/etc/frr/ospf6d.conf f 644 frr frr - - - - -
+	/etc/frr/isisd.conf f 644 frr frr - - - - -
+	/etc/frr/ripd.conf f 644 frr frr - - - - -
+	/etc/frr/ripngd.conf f 644 frr frr - - - - -
+	/etc/frr/pimd.conf f 644 frr frr - - - - -
+	/etc/frr/ldpd.conf f 644 frr frr - - - - -
+	/etc/frr/nhrpd.conf f 644 frr frr - - - - -

So all these empty files need to be writable ?

Quoted 4 lines

package/frr/frr.mk new file +4 -0

+	/etc/frr/vtysh.conf f 644 frr frrvty - - - - -
+endef
+
+define FRR_USERS

package/frr/frr.mk line 105

Quoted 10 lines

package/frr/frr.mk new file +10 -0

+	frr -1 frr -1 * /var/run/frr - frrvty FRR user priv
+endef
+
+define FRR_INSTALL_INIT_SYSV
+	$(INSTALL) -D -m 755 $(FRR_PKGDIR)/S50frr \
+		$(TARGET_DIR)/etc/init.d/S50frr
+endef
+
+$(eval $(autotools-package))
+$(eval $(host-autotools-package))

Thanks!

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Quoted 15 lines

Quoted 15 lines

package/frr/Config.in new file +9 -0

new file mode 100644
index 0000000000..01673eb837
@@ -0,0 +1,23 @@
+config BR2_PACKAGE_FRR
+	bool "frr"
+	depends on BR2_USE_MMU # fork()
+	depends on BR2_PACKAGE_BASH # init
+	select BR2_PACKAGE_RTRLIB
+	select BR2_PACKAGE_READLINE
+	select BR2_PACKAGE_JSON_C
+	select BR2_PACKAGE_LIBYANG
+	select BR2_PACKAGE_LIBCAP

Quoted 8 lines

package/frr/Config.in line 13

Quoted 4 lines

package/frr/Config.in new file +4 -0

+	select BR2_PACKAGE_LIBNL
+	select BR2_PACKAGE_NCURSES
+	select BR2_PACKAGE_NETSNMP
+	select BR2_PACKAGE_C_ARES  

That's a lot of packages that you select here, are you sure you properly propagated the "depends on" of all those packages in this Config.in ?

will try to check by disabling one by one.

Quoted 4 lines

Quoted 4 lines

package/frr/Config.in new file +4 -0

+	help
+	  The FRRouting Protocol Suite.
+
+	  FRR is free software that implements and manages various IPv4 and

Quoted 8 lines

package/frr/Config.in line 23

Quoted 6 lines

package/frr/Config.in new file +6 -0

+	  IPv6 routing protocols.
+
+	  https://frrouting.org
+
+comment "frr requires BASH for init service"
+	depends on !BR2_PACKAGE_BASH  

So it's the frrinit.sh script that requires bash ?

Yes, this is runtime dependency.

Quoted 3 lines

Quoted 1 line

+# for some reason the normal 'install' target fails

Why? Can it be fixed? At least reported to the upstream developers?

Ohhh, so, there is some miss-behave in case of cross-compilation that:
1) for some frr daemon installation uses -L/usr/lib

2) install tries to install binaries (apps & libs) to $(TARGET)/$(O) path which is invalid.

So, I just gave up and tried to do a bit manual way. Would be great if can suggest some generic hint with $(TARGET)$(O) issue.

Quoted 7 lines

package/frr/Config.in

Quoted 2 lines

package/frr/Config.in new file +2 -0

+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/log/frr
+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/run/frr  

I'm not sure this is going to work well, as /var/log and /var/run are symlinks to /tmp by default, and a tmpfs is mounted to /tmp.

These directories need to be created at runtime.

So in case of systemd it can be handled by tmpfs file, but in case of sysv is it OK to do it in init script ?

Quoted 23 lines · On Mon, 24 Feb 2020 23:14:27 +0200 vadim4j at gmail.com wrote:

Quoted 23 lines

Quoted 9 lines

package/frr/Config.in new file +3 -0

new file mode 100644
index 0000000000..01673eb837
@@ -0,0 +1,23 @@
+config BR2_PACKAGE_FRR
+	bool "frr"
+	depends on BR2_USE_MMU # fork()

package/frr/Config.in line 13

Quoted 10 lines

package/frr/Config.in new file +10 -0

+	depends on BR2_PACKAGE_BASH # init
+	select BR2_PACKAGE_RTRLIB
+	select BR2_PACKAGE_READLINE
+	select BR2_PACKAGE_JSON_C
+	select BR2_PACKAGE_LIBYANG
+	select BR2_PACKAGE_LIBCAP
+	select BR2_PACKAGE_LIBNL
+	select BR2_PACKAGE_NCURSES
+	select BR2_PACKAGE_NETSNMP
+	select BR2_PACKAGE_C_ARES  

That's a lot of packages that you select here, are you sure you properly propagated the "depends on" of all those packages in this Config.in ?

Quoted 1 line

will try to check by disabling one by one.

Huh? That's not what I'm asking here. I'm just asking that you look at the definition of each of those options, and if any of them has some "depends on" dependency, you need to replicate them into the frr package Config.in file.

Of course, you have to ensure that all these packages are really useful for you.

BTW: the list should be sorted alphabetically.

Quoted 12 lines

Quoted 12 lines

package/frr/Config.in line 23

Quoted 10 lines

package/frr/Config.in new file +10 -0

+	help
+	  The FRRouting Protocol Suite.
+
+	  FRR is free software that implements and manages various IPv4 and
+	  IPv6 routing protocols.
+
+	  https://frrouting.org
+
+comment "frr requires BASH for init service"
+	depends on !BR2_PACKAGE_BASH  

So it's the frrinit.sh script that requires bash ?

Quoted 1 line

Yes, this is runtime dependency.

And using this script is important?

Quoted 5 lines

Quoted 5 lines

Quoted 3 lines

+# for some reason the normal 'install' target fails

Why? Can it be fixed? At least reported to the upstream developers?

Quoted 8 lines

Ohhh, so, there is some miss-behave in case of cross-compilation that:
1) for some frr daemon installation uses -L/usr/lib

2) install tries to install binaries (apps & libs) to $(TARGET)/$(O) path which is invalid.

So, I just gave up and tried to do a bit manual way. Would be great if can suggest some generic hint with $(TARGET)$(O) issue.

Would need to reproduce the issue and investigate to provide some hints, I'm not sure I'll have the time soon. Maybe someone else can help here, and provide some suggestions ?

Quoted 9 lines

Quoted 7 lines

package/frr/Config.in

Quoted 2 lines

package/frr/Config.in new file +2 -0

+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/log/frr
+	$(INSTALL) -d -m 0755 $(TARGET_DIR)/var/run/frr  

I'm not sure this is going to work well, as /var/log and /var/run are symlinks to /tmp by default, and a tmpfs is mounted to /tmp.

These directories need to be created at runtime.

So in case of systemd it can be handled by tmpfs file, but in case of sysv is it OK to do it in init script ?

Yes, it is OK.

Best regards,

Thomas

Signature · 3 lines

Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Quoted 4 lines

Huh? That's not what I'm asking here. I'm just asking that you look at the definition of each of those options, and if any of them has some "depends on" dependency, you need to replicate them into the frr package Config.in file.

Hmmm, you mean to add here "depends on" from selected packages ?

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

So it's the frrinit.sh script that requires bash ?

Yes, this is runtime dependency.

And using this script is important?

Well, it runs daemons on system init to handle routing protocols.

Quoted 5 lines · On Mon, 24 Feb 2020 23:36:54 +0200 vadim4j at gmail.com wrote:

Quoted 4 lines

Huh? That's not what I'm asking here. I'm just asking that you look at the definition of each of those options, and if any of them has some "depends on" dependency, you need to replicate them into the frr package Config.in file.

Hmmm, you mean to add here "depends on" from selected packages ?

Yes, like we do in all other Buildroot packages.

See also section "17.2.3. Choosing depends on or select" in
https://buildroot.org/downloads/manual/manual.html.

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

So it's the frrinit.sh script that requires bash ?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Yes, this is runtime dependency.

And using this script is important?

Well, it runs daemons on system init to handle routing protocols.

OK, but is it complicated? Can it be replaced by some simpler Buildroot-compliant init script?