| Message ID | 1542300189-12939-1-git-send-email-sebastien.duponcheel@corp.ovh.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/1] shadowsocks-libev: add connmarktos build option | expand |
Hi Sébastien, Thanks for your contribution. A few comments below. DUPONCHEEL Sébastien writes: > Signed-off-by: DUPONCHEEL Sébastien <sebastien.duponcheel@corp.ovh.com> > --- > package/shadowsocks-libev/Config.in | 7 +++++++ > package/shadowsocks-libev/shadowsocks-libev.mk | 4 ++++ > 2 files changed, 11 insertions(+) > > diff --git a/package/shadowsocks-libev/Config.in b/package/shadowsocks-libev/Config.in > index f58abdb..acd9a67 100644 > --- a/package/shadowsocks-libev/Config.in > +++ b/package/shadowsocks-libev/Config.in > @@ -15,6 +15,13 @@ config BR2_PACKAGE_SHADOWSOCKS_LIBEV > > https://github.com/shadowsocks/shadowsocks-libev > > +config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS > + bool "enable connmarktos feature" > + depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV > + select BR2_PACKAGE_LIBNETFILTER_CONNTRACK > + help > + Build with the connmark to TOS feature If the size increase of enabling this feature is not huge we usually just enable it unconditionally when the required dependencies are enabled. This reduced the number of config options that the user has to go through. > comment "shadowsocks-libev needs a toolchain w/ threads" > depends on BR2_TOOLCHAIN_HAS_SYNC_4 > depends on BR2_TOOLCHAIN_HAS_SYNC_8 || !BR2_ARCH_IS_64 > diff --git a/package/shadowsocks-libev/shadowsocks-libev.mk b/package/shadowsocks-libev/shadowsocks-libev.mk > index 7fdcd3f..34d95ca 100644 > --- a/package/shadowsocks-libev/shadowsocks-libev.mk > +++ b/package/shadowsocks-libev/shadowsocks-libev.mk > @@ -21,4 +21,8 @@ ifeq ($(BR2_riscv),y) > SHADOWSOCKS_LIBEV_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) -D_REENTRANT" > endif > > +ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) So instead of that do ifeq ($(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),y) > +SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos If libnetfilter_conntrack is a build time dependency you also need to add it to SHADOWSOCKS_LIBEV_DEPENDENCIES here to make sure it build before shadowsocks-libev. > +endif You should also add --disable-connmarktos (or the equivalent option) in the 'else' part of this condition. > $(eval $(autotools-package)) baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Hi Sébastien, Please keep the list on Cc. DUPONCHEEL Sébastien writes: > it's nice to see you here :) > Thank you for your review and comments. > > See my proposal below : > > Le 15/11/2018 à 19:24, Baruch Siach a écrit: >> Thanks for your contribution. A few comments below. >> >> DUPONCHEEL Sébastien writes: >> >>> Signed-off-by: DUPONCHEEL Sébastien <sebastien.duponcheel@corp.ovh.com> >>> --- >>> package/shadowsocks-libev/Config.in | 7 +++++++ >>> package/shadowsocks-libev/shadowsocks-libev.mk | 4 ++++ >>> 2 files changed, 11 insertions(+) >>> >>> diff --git a/package/shadowsocks-libev/Config.in b/package/shadowsocks-libev/Config.in >>> index f58abdb..acd9a67 100644 >>> --- a/package/shadowsocks-libev/Config.in >>> +++ b/package/shadowsocks-libev/Config.in >>> @@ -15,6 +15,13 @@ config BR2_PACKAGE_SHADOWSOCKS_LIBEV >>> >>> https://github.com/shadowsocks/shadowsocks-libev >>> >>> +config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS >>> + bool "enable connmarktos feature" >>> + depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV >>> + select BR2_PACKAGE_LIBNETFILTER_CONNTRACK >>> + help >>> + Build with the connmark to TOS feature >> If the size increase of enabling this feature is not huge we usually >> just enable it unconditionally when the required dependencies are >> enabled. This reduced the number of config options that the user has to >> go through. > > For now, this feature only concerns ss-server and requires the operation of > advanced iptables and tc rules. This is a very specific feature that will not > be widely used, I think it is better to disable it by default > > So, i propose to be more verbose on what this feature implies : > > config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS > - bool "enable connmarktos feature" > + bool "ss-server: enable connmarktos feature" > depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV > select BR2_PACKAGE_LIBNETFILTER_CONNTRACK > help > - Build with the connmark to TOS feature > + Build ss-server with the connmark to TOS feature. > + This feature require advanced tc, iptables and conntrac > + rules to perform QoS on the server side. > + if unsure say N. Sounds reasonable to me. Let's see what others think. >>> comment "shadowsocks-libev needs a toolchain w/ threads" >>> depends on BR2_TOOLCHAIN_HAS_SYNC_4 >>> depends on BR2_TOOLCHAIN_HAS_SYNC_8 || !BR2_ARCH_IS_64 >>> diff --git a/package/shadowsocks-libev/shadowsocks-libev.mk b/package/shadowsocks-libev/shadowsocks-libev.mk >>> index 7fdcd3f..34d95ca 100644 >>> --- a/package/shadowsocks-libev/shadowsocks-libev.mk >>> +++ b/package/shadowsocks-libev/shadowsocks-libev.mk >>> @@ -21,4 +21,8 @@ ifeq ($(BR2_riscv),y) >>> SHADOWSOCKS_LIBEV_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) -D_REENTRANT" >>> endif >>> >>> +ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) >> So instead of that do >> >> ifeq ($(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),y) >> >>> +SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos >> If libnetfilter_conntrack is a build time dependency you also need to >> add it to SHADOWSOCKS_LIBEV_DEPENDENCIES here to make sure it build >> before shadowsocks-libev. > > As seen in "squid.mk", i propose to do something like this : > > -SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre > +SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre \ > + $(if $(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),libnetfilter_conntrack) The _OPTS and _DEPENDENCIES additions are usually grouped together. The squid case is spacial because there is no _OPTS change there. >>> +endif >> You should also add --disable-connmarktos (or the equivalent option) in >> the 'else' part of this condition. > > Unfortunately there is no such option. The AC_ARG_ENABLE macro in configure.ac automatically provides --enable-foo and --disable-foo. baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
Hello, On Sat, 17 Nov 2018 22:49:58 +0200, Baruch Siach wrote: > > config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS > > - bool "enable connmarktos feature" > > + bool "ss-server: enable connmarktos feature" > > depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV > > select BR2_PACKAGE_LIBNETFILTER_CONNTRACK > > help > > - Build with the connmark to TOS feature > > + Build ss-server with the connmark to TOS feature. > > + This feature require advanced tc, iptables and conntrac > > + rules to perform QoS on the server side. > > + if unsure say N. > > Sounds reasonable to me. Let's see what others think. Yes, soon reasonable to me as well. Perhaps the option title should be: bool "conmarktos support in ss-server" or bool "ss-server conmarktos support" Sébastien, could you take into account the various comments received from Baruch, and send an updated version ? Thanks! Thomas
Yes, i am doing this ASAP. Best regards, DUPONCHEEL Sébastien. Le 20/11/2018 à 09:00, Thomas Petazzoni a écrit : > Hello, > > On Sat, 17 Nov 2018 22:49:58 +0200, Baruch Siach wrote: > >>> config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS >>> - bool "enable connmarktos feature" >>> + bool "ss-server: enable connmarktos feature" >>> depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV >>> select BR2_PACKAGE_LIBNETFILTER_CONNTRACK >>> help >>> - Build with the connmark to TOS feature >>> + Build ss-server with the connmark to TOS feature. >>> + This feature require advanced tc, iptables and conntrac >>> + rules to perform QoS on the server side. >>> + if unsure say N. >> Sounds reasonable to me. Let's see what others think. > Yes, soon reasonable to me as well. Perhaps the option title should be: > > bool "conmarktos support in ss-server" > > or > > bool "ss-server conmarktos support" > > Sébastien, could you take into account the various comments received > from Baruch, and send an updated version ? > > Thanks! > > Thomas
Hi Baruch, Please see my feedback below : Le 17/11/2018 à 21:49, Baruch Siach a écrit : > Hi Sébastien, > > Please keep the list on Cc. > > DUPONCHEEL Sébastien writes: >> it's nice to see you here :) >> Thank you for your review and comments. >> >> See my proposal below : >> >> Le 15/11/2018 à 19:24, Baruch Siach a écrit: >>> Thanks for your contribution. A few comments below. >>> >>> DUPONCHEEL Sébastien writes: >>> >>>> Signed-off-by: DUPONCHEEL Sébastien <sebastien.duponcheel@corp.ovh.com> >>>> --- >>>> package/shadowsocks-libev/Config.in | 7 +++++++ >>>> package/shadowsocks-libev/shadowsocks-libev.mk | 4 ++++ >>>> 2 files changed, 11 insertions(+) >>>> >>>> diff --git a/package/shadowsocks-libev/Config.in b/package/shadowsocks-libev/Config.in >>>> index f58abdb..acd9a67 100644 >>>> --- a/package/shadowsocks-libev/Config.in >>>> +++ b/package/shadowsocks-libev/Config.in >>>> @@ -15,6 +15,13 @@ config BR2_PACKAGE_SHADOWSOCKS_LIBEV >>>> >>>> https://github.com/shadowsocks/shadowsocks-libev >>>> >>>> +config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS >>>> + bool "enable connmarktos feature" >>>> + depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV >>>> + select BR2_PACKAGE_LIBNETFILTER_CONNTRACK >>>> + help >>>> + Build with the connmark to TOS feature >>> If the size increase of enabling this feature is not huge we usually >>> just enable it unconditionally when the required dependencies are >>> enabled. This reduced the number of config options that the user has to >>> go through. >> For now, this feature only concerns ss-server and requires the operation of >> advanced iptables and tc rules. This is a very specific feature that will not >> be widely used, I think it is better to disable it by default >> >> So, i propose to be more verbose on what this feature implies : >> >> config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS >> - bool "enable connmarktos feature" >> + bool "ss-server: enable connmarktos feature" >> depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV >> select BR2_PACKAGE_LIBNETFILTER_CONNTRACK >> help >> - Build with the connmark to TOS feature >> + Build ss-server with the connmark to TOS feature. >> + This feature require advanced tc, iptables and conntrac >> + rules to perform QoS on the server side. >> + if unsure say N. > Sounds reasonable to me. Let's see what others think. > >>>> comment "shadowsocks-libev needs a toolchain w/ threads" >>>> depends on BR2_TOOLCHAIN_HAS_SYNC_4 >>>> depends on BR2_TOOLCHAIN_HAS_SYNC_8 || !BR2_ARCH_IS_64 >>>> diff --git a/package/shadowsocks-libev/shadowsocks-libev.mk b/package/shadowsocks-libev/shadowsocks-libev.mk >>>> index 7fdcd3f..34d95ca 100644 >>>> --- a/package/shadowsocks-libev/shadowsocks-libev.mk >>>> +++ b/package/shadowsocks-libev/shadowsocks-libev.mk >>>> @@ -21,4 +21,8 @@ ifeq ($(BR2_riscv),y) >>>> SHADOWSOCKS_LIBEV_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) -D_REENTRANT" >>>> endif >>>> >>>> +ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) >>> So instead of that do >>> >>> ifeq ($(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),y) >>> >>>> +SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos >>> If libnetfilter_conntrack is a build time dependency you also need to >>> add it to SHADOWSOCKS_LIBEV_DEPENDENCIES here to make sure it build >>> before shadowsocks-libev. >> As seen in "squid.mk", i propose to do something like this : >> >> -SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre >> +SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre \ >> + $(if $(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),libnetfilter_conntrack) > The _OPTS and _DEPENDENCIES additions are usually grouped together. The > squid case is spacial because there is no _OPTS change there. > >>>> +endif >>> You should also add --disable-connmarktos (or the equivalent option) in >>> the 'else' part of this condition. >> Unfortunately there is no such option. > The AC_ARG_ENABLE macro in configure.ac automatically provides > --enable-foo and --disable-foo. The AC_ARG_ENABLE statement in shadowsocks looks broken, neither --disable-connmarktos nor --enable-connmarktos=no are working. In fact, it activates the feature. ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos else SHADOWSOCKS_LIBEV_CONF_OPTS += --disable-connmarktos endif seb@compile:src/buildroot ‹shadowsocks›$ cat .config | grep CONNMARK # BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS is not set seb@compile:src/buildroot ‹shadowsocks*›$ ldd ./output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/bin/ss-server | grep libnetfilter_conntrack.so.3 libnetfilter_conntrack.so.3 => /home/seb/src/buildroot/output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/lib/libnetfilter_conntrack.so.3 (0x00006f816dde4000) Best regards, DUPONCHEEL Sébastien. > baruch > > -- > http://baruch.siach.name/blog/ ~. .~ Tk Open Systems > =}------------------------------------------------ooO--U--Ooo------------{= > - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - <html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body text="#000000" bgcolor="#FFFFFF"> <p>Hi Baruch,</p> <p>Please see my feedback below :<br> </p> <div class="moz-cite-prefix">Le 17/11/2018 à 21:49, Baruch Siach a écrit :<br> </div> <blockquote type="cite" cite="mid:871s7jh1jd.fsf@tkos.co.il"> <pre class="moz-quote-pre" wrap="">Hi Sébastien, Please keep the list on Cc. DUPONCHEEL Sébastien writes: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">it's nice to see you here :) Thank you for your review and comments. See my proposal below : Le 15/11/2018 à 19:24, Baruch Siach a écrit: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Thanks for your contribution. A few comments below. DUPONCHEEL Sébastien writes: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Signed-off-by: DUPONCHEEL Sébastien <a class="moz-txt-link-rfc2396E" href="mailto:sebastien.duponcheel@corp.ovh.com"><sebastien.duponcheel@corp.ovh.com></a> --- package/shadowsocks-libev/Config.in | 7 +++++++ package/shadowsocks-libev/shadowsocks-libev.mk | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/package/shadowsocks-libev/Config.in b/package/shadowsocks-libev/Config.in index f58abdb..acd9a67 100644 --- a/package/shadowsocks-libev/Config.in +++ b/package/shadowsocks-libev/Config.in @@ -15,6 +15,13 @@ config BR2_PACKAGE_SHADOWSOCKS_LIBEV <a class="moz-txt-link-freetext" href="https://github.com/shadowsocks/shadowsocks-libev">https://github.com/shadowsocks/shadowsocks-libev</a> +config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS + bool "enable connmarktos feature" + depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV + select BR2_PACKAGE_LIBNETFILTER_CONNTRACK + help + Build with the connmark to TOS feature </pre> </blockquote> <pre class="moz-quote-pre" wrap="">If the size increase of enabling this feature is not huge we usually just enable it unconditionally when the required dependencies are enabled. This reduced the number of config options that the user has to go through. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> For now, this feature only concerns ss-server and requires the operation of advanced iptables and tc rules. This is a very specific feature that will not be widely used, I think it is better to disable it by default So, i propose to be more verbose on what this feature implies : config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS - bool "enable connmarktos feature" + bool "ss-server: enable connmarktos feature" depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV select BR2_PACKAGE_LIBNETFILTER_CONNTRACK help - Build with the connmark to TOS feature + Build ss-server with the connmark to TOS feature. + This feature require advanced tc, iptables and conntrac + rules to perform QoS on the server side. + if unsure say N. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Sounds reasonable to me. Let's see what others think. </pre> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> comment "shadowsocks-libev needs a toolchain w/ threads" depends on BR2_TOOLCHAIN_HAS_SYNC_4 depends on BR2_TOOLCHAIN_HAS_SYNC_8 || !BR2_ARCH_IS_64 diff --git a/package/shadowsocks-libev/shadowsocks-libev.mk b/package/shadowsocks-libev/shadowsocks-libev.mk index 7fdcd3f..34d95ca 100644 --- a/package/shadowsocks-libev/shadowsocks-libev.mk +++ b/package/shadowsocks-libev/shadowsocks-libev.mk @@ -21,4 +21,8 @@ ifeq ($(BR2_riscv),y) SHADOWSOCKS_LIBEV_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) -D_REENTRANT" endif +ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) </pre> </blockquote> <pre class="moz-quote-pre" wrap="">So instead of that do ifeq ($(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),y) </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos </pre> </blockquote> <pre class="moz-quote-pre" wrap="">If libnetfilter_conntrack is a build time dependency you also need to add it to SHADOWSOCKS_LIBEV_DEPENDENCIES here to make sure it build before shadowsocks-libev. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> As seen in "squid.mk", i propose to do something like this : -SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre +SHADOWSOCKS_LIBEV_DEPENDENCIES = host-pkgconf c-ares libev libsodium mbedtls pcre \ + $(if $(BR2_PACKAGE_LIBNETFILTER_CONNTRACK),libnetfilter_conntrack) </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> The _OPTS and _DEPENDENCIES additions are usually grouped together. The squid case is spacial because there is no _OPTS change there. </pre> <blockquote type="cite"> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">+endif </pre> </blockquote> <pre class="moz-quote-pre" wrap="">You should also add --disable-connmarktos (or the equivalent option) in the 'else' part of this condition. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Unfortunately there is no such option. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> The AC_ARG_ENABLE macro in configure.ac automatically provides --enable-foo and --disable-foo.</pre> </blockquote> <pre>The AC_ARG_ENABLE statement in shadowsocks looks broken, neither --disable-connmarktos nor --enable-connmarktos=no are working. In fact, it activates the feature. ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos else SHADOWSOCKS_LIBEV_CONF_OPTS += --disable-connmarktos endif seb@compile:src/buildroot ‹shadowsocks›$ cat .config | grep CONNMARK # BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS is not set seb@compile:src/buildroot ‹shadowsocks*›$ ldd ./output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/bin/ss-server | grep libnetfilter_conntrack.so.3 libnetfilter_conntrack.so.3 => /home/seb/src/buildroot/output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/lib/libnetfilter_conntrack.so.3 (0x00006f816dde4000) Best regards, DUPONCHEEL Sébastien. </pre> <blockquote type="cite" cite="mid:871s7jh1jd.fsf@tkos.co.il"> <pre class="moz-quote-pre" wrap="">baruch -- <a class="moz-txt-link-freetext" href="http://baruch.siach.name/blog/">http://baruch.siach.name/blog/</a> ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - <a class="moz-txt-link-abbreviated" href="mailto:baruch@tkos.co.il">baruch@tkos.co.il</a> - tel: +972.52.368.4656, <a class="moz-txt-link-freetext" href="http://www.tkos.co.il">http://www.tkos.co.il</a> - </pre> </blockquote> </body> </html>
Hi Sébastien, DUPONCHEEL Sébastien writes: >>>> You should also add --disable-connmarktos (or the equivalent option) in >>>> the 'else' part of this condition. >>> Unfortunately there is no such option. >> The AC_ARG_ENABLE macro in configure.ac automatically provides >> --enable-foo and --disable-foo. > > The AC_ARG_ENABLE statement in shadowsocks looks broken, neither --disable-connmarktos > nor --enable-connmarktos=no are working. In fact, it activates the feature. > > ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) > SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack > SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos > else > SHADOWSOCKS_LIBEV_CONF_OPTS += --disable-connmarktos > endif > > seb@compile:src/buildroot ‹shadowsocks›$ cat .config | grep CONNMARK > # BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS is not set > seb@compile:src/buildroot ‹shadowsocks*›$ ldd ./output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/bin/ss-server | grep libnetfilter_conntrack.so.3 > libnetfilter_conntrack.so.3 => /home/seb/src/buildroot/output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/lib/libnetfilter_conntrack.so.3 (0x00006f816dde4000) Did you try full rebuild? Changing the Buildroot config does not trigger rebuild of already built packages. https://buildroot.org/downloads/manual/manual.html#full-rebuild For a single package test you can use this shortcut instead of a full rebuild: rm -r output/build/shadowsocks-libev-* make shadowsocks-libev baruch -- http://baruch.siach.name/blog/ ~. .~ Tk Open Systems =}------------------------------------------------ooO--U--Ooo------------{= - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -
On 20/11/2018 14:56, DUPONCHEEL Sébastien wrote: > Hi Baruch, > > Please see my feedback below : > > Le 17/11/2018 à 21:49, Baruch Siach a écrit : [snip] >> The AC_ARG_ENABLE macro in configure.ac automatically provides >> --enable-foo and --disable-foo. > > The AC_ARG_ENABLE statement in shadowsocks looks broken, neither --disable-connmarktos > nor --enable-connmarktos=no are working. In fact, it activates the feature. Yes, that's a classic problem - people refuse to read documentation so they don't notice that the third argument is not "code to run when the option is yes" but actually it is "code to run when the option is given". So, either you fix this by patching shadowsocks-libev's configure.ac and replacing - enable_connmarktos="yes" + enable_connmarktos="$enableval" and sending the patch upstream, or you do ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos # --disable-connmarktos *enables* it. endif Obviously, the first option is preferred. Regards, Arnout > > ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) > SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack > SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos > else > SHADOWSOCKS_LIBEV_CONF_OPTS += --disable-connmarktos > endif > > seb@compile:src/buildroot ‹shadowsocks›$ cat .config | grep CONNMARK > # BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS is not set > seb@compile:src/buildroot ‹shadowsocks*›$ ldd ./output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/bin/ss-server | grep libnetfilter_conntrack.so.3 > libnetfilter_conntrack.so.3 => /home/seb/src/buildroot/output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/lib/libnetfilter_conntrack.so.3 (0x00006f816dde4000) > > Best regards, > DUPONCHEEL Sébastien. > >> baruch >> >> -- >> http://baruch.siach.name/blog/ ~. .~ Tk Open Systems >> =}------------------------------------------------ooO--U--Ooo------------{= >> - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - > > _______________________________________________ > buildroot mailing list > buildroot@busybox.net > http://lists.busybox.net/mailman/listinfo/buildroot >
Pull request opened upstream, thank you. Best regards, DUPONCHEEL Sébastien Le 20/11/2018 à 23:25, Arnout Vandecappelle a écrit : > > On 20/11/2018 14:56, DUPONCHEEL Sébastien wrote: >> Hi Baruch, >> >> Please see my feedback below : >> >> Le 17/11/2018 à 21:49, Baruch Siach a écrit : > [snip] >>> The AC_ARG_ENABLE macro in configure.ac automatically provides >>> --enable-foo and --disable-foo. >> The AC_ARG_ENABLE statement in shadowsocks looks broken, neither --disable-connmarktos >> nor --enable-connmarktos=no are working. In fact, it activates the feature. > Yes, that's a classic problem - people refuse to read documentation so they > don't notice that the third argument is not "code to run when the option is yes" > but actually it is "code to run when the option is given". > > So, either you fix this by patching shadowsocks-libev's configure.ac and replacing > > - enable_connmarktos="yes" > + enable_connmarktos="$enableval" > > and sending the patch upstream, or you do > > ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) > SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack > SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos > # --disable-connmarktos *enables* it. > endif > > Obviously, the first option is preferred. > > Regards, > Arnout > >> ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) >> SHADOWSOCKS_LIBEV_DEPENDENCIES += libnetfilter_conntrack >> SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos >> else >> SHADOWSOCKS_LIBEV_CONF_OPTS += --disable-connmarktos >> endif >> >> seb@compile:src/buildroot ‹shadowsocks›$ cat .config | grep CONNMARK >> # BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS is not set >> seb@compile:src/buildroot ‹shadowsocks*›$ ldd ./output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/bin/ss-server | grep libnetfilter_conntrack.so.3 >> libnetfilter_conntrack.so.3 => /home/seb/src/buildroot/output/host/x86_64-buildroot-linux-uclibc/sysroot/usr/lib/libnetfilter_conntrack.so.3 (0x00006f816dde4000) >> >> Best regards, >> DUPONCHEEL Sébastien. >> >>> baruch >>> >>> -- >>> http://baruch.siach.name/blog/ ~. .~ Tk Open Systems >>> =}------------------------------------------------ooO--U--Ooo------------{= >>> - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il - >> _______________________________________________ >> buildroot mailing list >> buildroot@busybox.net >> http://lists.busybox.net/mailman/listinfo/buildroot >>
diff --git a/package/shadowsocks-libev/Config.in b/package/shadowsocks-libev/Config.in index f58abdb..acd9a67 100644 --- a/package/shadowsocks-libev/Config.in +++ b/package/shadowsocks-libev/Config.in @@ -15,6 +15,13 @@ config BR2_PACKAGE_SHADOWSOCKS_LIBEV https://github.com/shadowsocks/shadowsocks-libev +config BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS + bool "enable connmarktos feature" + depends on BR2_PACKAGE_SHADOWSOCKS_LIBEV + select BR2_PACKAGE_LIBNETFILTER_CONNTRACK + help + Build with the connmark to TOS feature + comment "shadowsocks-libev needs a toolchain w/ threads" depends on BR2_TOOLCHAIN_HAS_SYNC_4 depends on BR2_TOOLCHAIN_HAS_SYNC_8 || !BR2_ARCH_IS_64 diff --git a/package/shadowsocks-libev/shadowsocks-libev.mk b/package/shadowsocks-libev/shadowsocks-libev.mk index 7fdcd3f..34d95ca 100644 --- a/package/shadowsocks-libev/shadowsocks-libev.mk +++ b/package/shadowsocks-libev/shadowsocks-libev.mk @@ -21,4 +21,8 @@ ifeq ($(BR2_riscv),y) SHADOWSOCKS_LIBEV_CONF_ENV += CFLAGS="$(TARGET_CFLAGS) -D_REENTRANT" endif +ifeq ($(BR2_PACKAGE_SHADOWSOCKS_LIBEV_CONNMARKTOS),y) +SHADOWSOCKS_LIBEV_CONF_OPTS += --enable-connmarktos +endif + $(eval $(autotools-package))
Signed-off-by: DUPONCHEEL Sébastien <sebastien.duponcheel@corp.ovh.com> --- package/shadowsocks-libev/Config.in | 7 +++++++ package/shadowsocks-libev/shadowsocks-libev.mk | 4 ++++ 2 files changed, 11 insertions(+)