| Message ID | 20211016093234.2477835-2-fontaine.fabrice@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] package/knock: bump to version 0.8 | expand |
On Sat, 16 Oct 2021 11:32:34 +0200 Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) > +KNOCK_DEPENDENCIES = libpcap From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1: Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a conditional is the root of all evil." Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a conditional is the root of all evil." Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a conditional is the root of all evil." Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a conditional is the root of all evil." Care to repeat after me ? :-) Anyway, with this typo fixed, applied! Thanks, Thomas
Thomas, All, On 2021-10-18 22:04 +0200, Thomas Petazzoni spake thusly: > On Sat, 16 Oct 2021 11:32:34 +0200 > Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > > +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) > > +KNOCK_DEPENDENCIES = libpcap > From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1: > Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a > conditional is the root of all evil." > Care to repeat after me ? :-) OK, I missed that in my review of v1... /me hides in shame... But seriously though: why is that not caught by check-package? Because it is not actually an override: the variable is not yet set there, so it is valid to unconditionally set it. check-package should probably recognise that the variable is <FOO>_DEPENDENCIES and consider that conditional assignment should still be forbidden in that case. Regards, Yann E. MORIN.
On 18/10/2021 22:18, Yann E. MORIN wrote: > Thomas, All, > > On 2021-10-18 22:04 +0200, Thomas Petazzoni spake thusly: >> On Sat, 16 Oct 2021 11:32:34 +0200 >> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: >>> +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) >>> +KNOCK_DEPENDENCIES = libpcap >> From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1: >> Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a >> conditional is the root of all evil." >> Care to repeat after me ? :-) > > OK, I missed that in my review of v1... > /me hides in shame... > > But seriously though: why is that not caught by check-package? > > Because it is not actually an override: the variable is not yet set > there, so it is valid to unconditionally set it. > > check-package should probably recognise that the variable is > <FOO>_DEPENDENCIES and consider that conditional assignment should > still be forbidden in that case. I had a look at this, and it is not entirely trivial to test for in check-package. Basically, this is allowed: ifeq(...) FOO_VAR = ... endif ... $(FOO_VAR) ... # Empty if condition not true And this as well: ifeq(...) FOO_VAR = ... else FOO_VAR = ... endif But this is not: ifeq(...) FOO_VAR = ... endif ifeq(...) FOO_VAR = ... endif So basically: - after a variable is assigned, it can't be assigned to any more in the same conditional branch; - it can be assigned to again in the else branch (including else ifeq constructs); - it can not be assigned to any more after the endif. In addition, conditions can be nested... The only way I can think of implementing this is by keeping a stack of conditionally assigned variables. In the else branch, the top of the stack is saved en empties. At the endif, the top of the stack is popped, merged with the saved one, and merged into the new top of the stack. Wow, by writing it down, I almost implemented it :-) Let's see if I can still find the time to actually do it tonight... Regards, Arnout
Arnout, All, On 2021-10-21 20:43 +0200, Arnout Vandecappelle spake thusly: > On 18/10/2021 22:18, Yann E. MORIN wrote: > >Thomas, All, > >On 2021-10-18 22:04 +0200, Thomas Petazzoni spake thusly: > >>On Sat, 16 Oct 2021 11:32:34 +0200 > >>Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: > >>>+ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) > >>>+KNOCK_DEPENDENCIES = libpcap > >> From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1: > >> Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a > >> conditional is the root of all evil." > >>Care to repeat after me ? :-) > > > >OK, I missed that in my review of v1... > >/me hides in shame... > > > >But seriously though: why is that not caught by check-package? > > > >Because it is not actually an override: the variable is not yet set > >there, so it is valid to unconditionally set it. > > > >check-package should probably recognise that the variable is > ><FOO>_DEPENDENCIES and consider that conditional assignment should > >still be forbidden in that case. > > I had a look at this, and it is not entirely trivial to test for in check-package. > > Basically, this is allowed: > > ifeq(...) > FOO_VAR = ... > endif > ... $(FOO_VAR) ... # Empty if condition not true > > And this as well: > > ifeq(...) > FOO_VAR = ... > else > FOO_VAR = ... > endif > > But this is not: > > ifeq(...) > FOO_VAR = ... > endif > > ifeq(...) > FOO_VAR = ... > endif > > > So basically: > - after a variable is assigned, it can't be assigned to any more in the same > conditional branch; > - it can be assigned to again in the else branch (including else ifeq constructs); > - it can not be assigned to any more after the endif. > > In addition, conditions can be nested... > > The only way I can think of implementing this is by keeping a stack of > conditionally assigned variables. In the else branch, the top of the stack > is saved en empties. At the endif, the top of the stack is popped, merged > with the saved one, and merged into the new top of the stack. > > Wow, by writing it down, I almost implemented it :-) Let's see if I can > still find the time to actually do it tonight... Sorry, but I think I got lost somewhere... Currently, check-package does not whine for: ifeq(...) FOO_VAR = ... endif but it does whine for: FOO_VAR = ... ifeq(...) FOO_VAR = ... endif And these two behaviours are, I think, correct. What I am arguing, is that FOO_DEPENDENCIES should be treated specially, where check-package should whine for: ifeq(...) FOO_DEPENDENCIES = ... endif as well as for: FOO_DEPENDENCIES = ... ifeq(...) FOO_DEPENDENCIES = ... endif Regards, Yann E. MORIN.
On 21/10/2021 22:12, Yann E. MORIN wrote: > Arnout, All, > > On 2021-10-21 20:43 +0200, Arnout Vandecappelle spake thusly: >> On 18/10/2021 22:18, Yann E. MORIN wrote: >>> Thomas, All, >>> On 2021-10-18 22:04 +0200, Thomas Petazzoni spake thusly: >>>> On Sat, 16 Oct 2021 11:32:34 +0200 >>>> Fabrice Fontaine <fontaine.fabrice@gmail.com> wrote: >>>>> +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) >>>>> +KNOCK_DEPENDENCIES = libpcap >>>> From commit 97f3ad7af3dd54a15a10aa35786e7fa08cf5e7b1: >>>> Repeat after me: "Forcing the value of <pkg>_DEPENDENCIES inside a >>>> conditional is the root of all evil." >>>> Care to repeat after me ? :-) >>> >>> OK, I missed that in my review of v1... >>> /me hides in shame... >>> >>> But seriously though: why is that not caught by check-package? >>> >>> Because it is not actually an override: the variable is not yet set >>> there, so it is valid to unconditionally set it. >>> >>> check-package should probably recognise that the variable is >>> <FOO>_DEPENDENCIES and consider that conditional assignment should >>> still be forbidden in that case. >> >> I had a look at this, and it is not entirely trivial to test for in check-package. >> >> Basically, this is allowed: >> >> ifeq(...) >> FOO_VAR = ... >> endif >> ... $(FOO_VAR) ... # Empty if condition not true >> >> And this as well: >> >> ifeq(...) >> FOO_VAR = ... >> else >> FOO_VAR = ... >> endif >> >> But this is not: >> >> ifeq(...) >> FOO_VAR = ... >> endif >> >> ifeq(...) >> FOO_VAR = ... >> endif >> >> >> So basically: >> - after a variable is assigned, it can't be assigned to any more in the same >> conditional branch; >> - it can be assigned to again in the else branch (including else ifeq constructs); >> - it can not be assigned to any more after the endif. >> >> In addition, conditions can be nested... >> >> The only way I can think of implementing this is by keeping a stack of >> conditionally assigned variables. In the else branch, the top of the stack >> is saved en empties. At the endif, the top of the stack is popped, merged >> with the saved one, and merged into the new top of the stack. >> >> Wow, by writing it down, I almost implemented it :-) Let's see if I can >> still find the time to actually do it tonight... > > Sorry, but I think I got lost somewhere... > > Currently, check-package does not whine for: > > ifeq(...) > FOO_VAR = ... > endif > > but it does whine for: > > FOO_VAR = ... > ifeq(...) > FOO_VAR = ... > endif > > And these two behaviours are, I think, correct. > > What I am arguing, is that FOO_DEPENDENCIES should be treated specially, > where check-package should whine for: > > ifeq(...) > FOO_DEPENDENCIES = ... > endif > > as well as for: > > FOO_DEPENDENCIES = ... > ifeq(...) > FOO_DEPENDENCIES = ... > endif Yeah, special-casing DEPENDENCIES is definitely an option. My "wanted behaviour" was for other variables which can have the same problem. Probably doesn't happen very often though. Regards, Arnout
diff --git a/package/knock/Config.in b/package/knock/Config.in index 0060ad07ce..5a4c00d9e8 100644 --- a/package/knock/Config.in +++ b/package/knock/Config.in @@ -1,7 +1,5 @@ config BR2_PACKAGE_KNOCK bool "knock" - depends on BR2_USE_MMU # fork() - select BR2_PACKAGE_LIBPCAP help A port knocking implementation. Provides a daemon and a user application. Port knocking can be @@ -11,3 +9,15 @@ config BR2_PACKAGE_KNOCK firewall. http://www.zeroflux.org/projects/knock + +if BR2_PACKAGE_KNOCK + +config BR2_PACKAGE_KNOCK_KNOCKD + bool "knockd" + default y + depends on BR2_USE_MMU # fork() + select BR2_PACKAGE_LIBPCAP + help + Enable knockd + +endif diff --git a/package/knock/knock.mk b/package/knock/knock.mk index 2cfa738074..82a9ba9b53 100644 --- a/package/knock/knock.mk +++ b/package/knock/knock.mk @@ -9,10 +9,15 @@ KNOCK_SITE = $(call github,jvinet,knock,v$(KNOCK_VERSION)) KNOCK_AUTORECONF = YES KNOCK_LICENSE = GPL-2.0+ KNOCK_LICENSE_FILES = COPYING -KNOCK_DEPENDENCIES = libpcap +ifeq ($(BR2_PACKAGE_KNOCK_KNOCKD),y) +KNOCK_DEPENDENCIES = libpcap +KNOCK_CONF_OPTS += --enable-knockd ifeq ($(BR2_STATIC_LIBS),y) -KNOCK_CONF_OPTS = LIBS="`$(STAGING_DIR)/usr/bin/pcap-config --static --additional-libs`" +KNOCK_CONF_OPTS += LIBS="`$(STAGING_DIR)/usr/bin/pcap-config --static --additional-libs`" +endif +else +KNOCK_CONF_OPTS += --disable-knockd endif $(eval $(autotools-package))
Add knockd option to make libpcap and MMU optional and enable it by default for backward compatibility Signed-off-by: Fabrice Fontaine <fontaine.fabrice@gmail.com> --- package/knock/Config.in | 14 ++++++++++++-- package/knock/knock.mk | 9 +++++++-- 2 files changed, 19 insertions(+), 4 deletions(-)