| Message ID | 20160912192324.23682-3-s.martin49@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hello, On Mon, 12 Sep 2016 21:23:24 +0200, Samuel Martin wrote: > Fixes: > http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ > > Signed-off-by: Samuel Martin <s.martin49@gmail.com> > --- > package/nginx-naxsi/Config.in | 1 + > 1 file changed, 1 insertion(+) Applied to master, thanks. Thomas
On 12-09-16 21:23, Samuel Martin wrote: > Fixes: > http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ > > Signed-off-by: Samuel Martin <s.martin49@gmail.com> > --- > package/nginx-naxsi/Config.in | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/package/nginx-naxsi/Config.in b/package/nginx-naxsi/Config.in > index 2f7c2da..6c175eb 100644 > --- a/package/nginx-naxsi/Config.in > +++ b/package/nginx-naxsi/Config.in > @@ -1,5 +1,6 @@ > config BR2_PACKAGE_NGINX_NAXSI > bool "nginx-naxsi" > + select BR2_PACKAGE_PCRE > help > NAXSI means Nginx Anti XSS & SQL Injection. I may be missing something, but shouldn't there be a _DEPENDENCIES += in .mk? Regards, Arnout
On Tue, Sep 13, 2016 at 8:40 AM, Arnout Vandecappelle <arnout@mind.be> wrote: > > > On 12-09-16 21:23, Samuel Martin wrote: >> Fixes: >> http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ >> >> Signed-off-by: Samuel Martin <s.martin49@gmail.com> >> --- >> package/nginx-naxsi/Config.in | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/package/nginx-naxsi/Config.in b/package/nginx-naxsi/Config.in >> index 2f7c2da..6c175eb 100644 >> --- a/package/nginx-naxsi/Config.in >> +++ b/package/nginx-naxsi/Config.in >> @@ -1,5 +1,6 @@ >> config BR2_PACKAGE_NGINX_NAXSI >> bool "nginx-naxsi" >> + select BR2_PACKAGE_PCRE >> help >> NAXSI means Nginx Anti XSS & SQL Injection. > > I may be missing something, but shouldn't there be a _DEPENDENCIES += in .mk? Nope, this is not needed. naxsi is a nginx' module, built by nginx.mk, which already has an automatic dependency on pcre when selected. Regards,
On 13-09-16 09:57, Samuel Martin wrote: > On Tue, Sep 13, 2016 at 8:40 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >> >> >> On 12-09-16 21:23, Samuel Martin wrote: >>> Fixes: >>> http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ >>> >>> Signed-off-by: Samuel Martin <s.martin49@gmail.com> >>> --- >>> package/nginx-naxsi/Config.in | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/package/nginx-naxsi/Config.in b/package/nginx-naxsi/Config.in >>> index 2f7c2da..6c175eb 100644 >>> --- a/package/nginx-naxsi/Config.in >>> +++ b/package/nginx-naxsi/Config.in >>> @@ -1,5 +1,6 @@ >>> config BR2_PACKAGE_NGINX_NAXSI >>> bool "nginx-naxsi" >>> + select BR2_PACKAGE_PCRE >>> help >>> NAXSI means Nginx Anti XSS & SQL Injection. >> >> I may be missing something, but shouldn't there be a _DEPENDENCIES += in .mk? > > Nope, this is not needed. > naxsi is a nginx' module, built by nginx.mk, which already has an > automatic dependency on pcre when selected. I don't think we should rely on transitive dependencies. naxsi.h #include's pcre.h, so there should be an explicit dependency in the .mk file. Whenever you don't have that, the 'select' in Config.in should carry a comment explaining why not (usually 'runtime dependency'). Regards, Arnout
On Tue, Sep 13, 2016 at 7:30 PM, Arnout Vandecappelle <arnout@mind.be> wrote: > > > On 13-09-16 09:57, Samuel Martin wrote: >> On Tue, Sep 13, 2016 at 8:40 AM, Arnout Vandecappelle <arnout@mind.be> wrote: >>> >>> >>> On 12-09-16 21:23, Samuel Martin wrote: >>>> Fixes: >>>> http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ >>>> >>>> Signed-off-by: Samuel Martin <s.martin49@gmail.com> >>>> --- >>>> package/nginx-naxsi/Config.in | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/package/nginx-naxsi/Config.in b/package/nginx-naxsi/Config.in >>>> index 2f7c2da..6c175eb 100644 >>>> --- a/package/nginx-naxsi/Config.in >>>> +++ b/package/nginx-naxsi/Config.in >>>> @@ -1,5 +1,6 @@ >>>> config BR2_PACKAGE_NGINX_NAXSI >>>> bool "nginx-naxsi" >>>> + select BR2_PACKAGE_PCRE >>>> help >>>> NAXSI means Nginx Anti XSS & SQL Injection. >>> >>> I may be missing something, but shouldn't there be a _DEPENDENCIES += in .mk? >> >> Nope, this is not needed. >> naxsi is a nginx' module, built by nginx.mk, which already has an >> automatic dependency on pcre when selected. > > I don't think we should rely on transitive dependencies. naxsi.h #include's > pcre.h, so there should be an explicit dependency in the .mk file. Fair enough, I'll post the follow-up shortly.
Hello, On Tue, 13 Sep 2016 19:30:19 +0200, Arnout Vandecappelle wrote: > > Nope, this is not needed. > > naxsi is a nginx' module, built by nginx.mk, which already has an > > automatic dependency on pcre when selected. > > I don't think we should rely on transitive dependencies. naxsi.h #include's > pcre.h, so there should be an explicit dependency in the .mk file. > > Whenever you don't have that, the 'select' in Config.in should carry a comment > explaining why not (usually 'runtime dependency'). It's not a transitive dependency here. The interaction between nginx-naxsi and nginx is very special: nginx-naxsi is only in charge of downloading source code, and provide it to nginx during its build process. nginx is the one that *depends* on nginx-naxsi, so that nginx-naxsi has downloaded and extracted its source code before nginx gets configured. So, nginx-naxsi is the one that really needs pcre (so it makes sense for it to select pcre). But it terms of build ordering, pcre is only really needed before nginx starts its configuration step. So I believe the current situation is OK, with the exception that nginx-naxsi/Config.in should have a comment that explains that. Thanks, Thomas
diff --git a/package/nginx-naxsi/Config.in b/package/nginx-naxsi/Config.in index 2f7c2da..6c175eb 100644 --- a/package/nginx-naxsi/Config.in +++ b/package/nginx-naxsi/Config.in @@ -1,5 +1,6 @@ config BR2_PACKAGE_NGINX_NAXSI bool "nginx-naxsi" + select BR2_PACKAGE_PCRE help NAXSI means Nginx Anti XSS & SQL Injection.
Fixes: http://autobuild.buildroot.net/results/9c7/9c7bad6831b09251af81e2bbfc595a241df87c70/ Signed-off-by: Samuel Martin <s.martin49@gmail.com> --- package/nginx-naxsi/Config.in | 1 + 1 file changed, 1 insertion(+)