| Message ID | 1480438071-11906-1-git-send-email-johan.oudinet@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
Hi, I forgot to use the new get-developers script when sending this patch. According to it, I should have put in CC Samuel Martin. I also see you're in the middle of a new release. Should I rebase this patch in next and re-send it? On Tue, Nov 29, 2016 at 5:47 PM, Johan Oudinet <johan.oudinet@gmail.com> wrote: > Nginx built-in support for webdav is missing support for two commands: > PROPFIND and OPTIONS. Add an external module and an option in the nginx > package to provide full webdav support. > > Signed-off-by: Johan Oudinet <johan.oudinet@gmail.com> > --- > package/Config.in | 1 + > package/nginx-dav-ext/Config.in | 7 +++++++ > package/nginx-dav-ext/nginx-dav-ext.hash | 2 ++ > package/nginx-dav-ext/nginx-dav-ext.mk | 12 ++++++++++++ > package/nginx/Config.in | 10 ++++++++++ > package/nginx/nginx.mk | 5 +++++ > 6 files changed, 37 insertions(+) > create mode 100644 package/nginx-dav-ext/Config.in > create mode 100644 package/nginx-dav-ext/nginx-dav-ext.hash > create mode 100644 package/nginx-dav-ext/nginx-dav-ext.mk > > diff --git a/package/Config.in b/package/Config.in > index 9ed296f..687c600 100644 > --- a/package/Config.in > +++ b/package/Config.in > @@ -1544,6 +1544,7 @@ menu "Networking applications" > source "package/nginx/Config.in" > if BR2_PACKAGE_NGINX > menu "External nginx modules" > + source "package/nginx-dav-ext/Config.in" > source "package/nginx-naxsi/Config.in" > source "package/nginx-upload/Config.in" > endmenu > diff --git a/package/nginx-dav-ext/Config.in b/package/nginx-dav-ext/Config.in > new file mode 100644 > index 0000000..bf197dc > --- /dev/null > +++ b/package/nginx-dav-ext/Config.in > @@ -0,0 +1,7 @@ > +config BR2_PACKAGE_NGINX_DAV_EXT > + bool "nginx-dav-ext" > + select BR2_PACKAGE_EXPAT > + help > + NGINX WebDAV missing commands support (PROPFIND & OPTIONS). > + > + https://github.com/arut/nginx-dav-ext-module > diff --git a/package/nginx-dav-ext/nginx-dav-ext.hash b/package/nginx-dav-ext/nginx-dav-ext.hash > new file mode 100644 > index 0000000..ea7eb86 > --- /dev/null > +++ b/package/nginx-dav-ext/nginx-dav-ext.hash > @@ -0,0 +1,2 @@ > +# Locally computed > +sha256 d428a0236c933779cb40ac8c91afb19d5c25a376dc3caab825bfd543e1ee530d nginx-dav-ext-v0.0.3.tar.gz > diff --git a/package/nginx-dav-ext/nginx-dav-ext.mk b/package/nginx-dav-ext/nginx-dav-ext.mk > new file mode 100644 > index 0000000..6c2b3a2 > --- /dev/null > +++ b/package/nginx-dav-ext/nginx-dav-ext.mk > @@ -0,0 +1,12 @@ > +################################################################################ > +# > +# nginx-dav-ext > +# > +################################################################################ > + > +NGINX_DAV_EXT_VERSION = v0.0.3 > +NGINX_DAV_EXT_SITE = $(call github,arut,nginx-dav-ext-module,$(NGINX_DAV_EXT_VERSION)) > + > +NGINX_DAV_EXT_DEPENDENCIES = expat > + > +$(eval $(generic-package)) > diff --git a/package/nginx/Config.in b/package/nginx/Config.in > index e6f2d96..6f339c7 100644 > --- a/package/nginx/Config.in > +++ b/package/nginx/Config.in > @@ -85,6 +85,16 @@ config BR2_PACKAGE_NGINX_HTTP_DAV_MODULE > help > Enable ngx_http_dav_module > > +if BR2_PACKAGE_NGINX_HTTP_DAV_MODULE > + > +config BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE > + bool "ngx_http_dav_ext_module" > + select BR2_PACKAGE_NGINX_DAV_EXT > + help > + Enable ngx_http_dav_ext_module > + > +endif # BR2_PACKAGE_NGINX_HTTP_DAV_MODULE > + > config BR2_PACKAGE_NGINX_HTTP_FLV_MODULE > bool "ngx_http_flv_module" > help > diff --git a/package/nginx/nginx.mk b/package/nginx/nginx.mk > index 6563627..c99139b 100644 > --- a/package/nginx/nginx.mk > +++ b/package/nginx/nginx.mk > @@ -167,6 +167,11 @@ else > NGINX_CONF_OPTS += --without-http_rewrite_module > endif > > +ifeq ($(BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE)$(BR2_PACKAGE_NGINX_DAV_EXT),yy) > +NGINX_DEPENDENCIES += nginx-dav-ext > +NGINX_CONF_OPTS += --add-module=$(NGINX_DAV_EXT_DIR) > +endif > + > NGINX_CONF_OPTS += \ > $(if $(BR2_PACKAGE_NGINX_HTTP_REALIP_MODULE),--with-http_realip_module) \ > $(if $(BR2_PACKAGE_NGINX_HTTP_ADDITION_MODULE),--with-http_addition_module) \ > -- > 2.7.4 >
Hello, I've applied to master, but after doing a number of changes. See below. On Tue, 29 Nov 2016 17:47:51 +0100, Johan Oudinet wrote: > diff --git a/package/nginx-dav-ext/nginx-dav-ext.mk b/package/nginx-dav-ext/nginx-dav-ext.mk > new file mode 100644 > index 0000000..6c2b3a2 > --- /dev/null > +++ b/package/nginx-dav-ext/nginx-dav-ext.mk > @@ -0,0 +1,12 @@ > +################################################################################ > +# > +# nginx-dav-ext > +# > +################################################################################ > + > +NGINX_DAV_EXT_VERSION = v0.0.3 > +NGINX_DAV_EXT_SITE = $(call github,arut,nginx-dav-ext-module,$(NGINX_DAV_EXT_VERSION)) > + LICENSE and LICENSE_FILES were missing here, so I've added them. > +NGINX_DAV_EXT_DEPENDENCIES = expat > diff --git a/package/nginx/Config.in b/package/nginx/Config.in > index e6f2d96..6f339c7 100644 > --- a/package/nginx/Config.in > +++ b/package/nginx/Config.in > @@ -85,6 +85,16 @@ config BR2_PACKAGE_NGINX_HTTP_DAV_MODULE > help > Enable ngx_http_dav_module > > +if BR2_PACKAGE_NGINX_HTTP_DAV_MODULE > + > +config BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE > + bool "ngx_http_dav_ext_module" > + select BR2_PACKAGE_NGINX_DAV_EXT > + help > + Enable ngx_http_dav_ext_module > + > +endif # BR2_PACKAGE_NGINX_HTTP_DAV_MODULE This was not really needed, and we don't do that for other external nginx modules, so I've dropped this change. Enabling BR2_PACKAGE_NGINX_DAV_EXT is enough to get this module enabled. > +ifeq ($(BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE)$(BR2_PACKAGE_NGINX_DAV_EXT),yy) So I've changed this to just: ifeq ($(BR2_PACKAGE_NGINX_DAV_EXT),y) > +NGINX_DEPENDENCIES += nginx-dav-ext > +NGINX_CONF_OPTS += --add-module=$(NGINX_DAV_EXT_DIR) > +endif and moved the whole chunk next to the "upload" external module handling, so that all external modules are handled in the same place in nginx.mk. I've also added a separate patch to add you in the DEVELOPERS file for nginx-dav-ext (even though you will never receive a build failure about it, since the build really takes place inside the nginx package). Thanks! Thomas
On Sun, Dec 4, 2016 at 11:52 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Hello, > > I've applied to master, but after doing a number of changes. See below. Thanks Thomas. > I've also added a separate patch to add you in the DEVELOPERS file for > nginx-dav-ext (even though you will never receive a build failure about > it, since the build really takes place inside the nginx package). > Oops, so many changes since I've sent a patch to BR. I should have read the documentation more carefully. Thanks.
Hello, On Mon, 5 Dec 2016 11:58:33 +0100, Johan Oudinet wrote: > > I've applied to master, but after doing a number of changes. See below. > > Thanks Thomas. > > > I've also added a separate patch to add you in the DEVELOPERS file for > > nginx-dav-ext (even though you will never receive a build failure about > > it, since the build really takes place inside the nginx package). > > Oops, so many changes since I've sent a patch to BR. I should have > read the documentation more carefully. Thanks. Seems like my changes broke the build: http://autobuild.buildroot.net/?reason=nginx-1.10.2 So, it might be that the location in which modules are added in nginx.mk might be important. Weird, because I did a number of test builds before pushing and never got a failure. Do you have the time to investigate? One of the failure, http://autobuild.buildroot.net/results/3c8/3c8798f122a421e8fae741de76b2504c20e2da91/build-end.log, is related to dav-ext. Thanks, Thomas
On Mon, Dec 5, 2016 at 12:14 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > > Seems like my changes broke the build: > > http://autobuild.buildroot.net/?reason=nginx-1.10.2 > > So, it might be that the location in which modules are added in > nginx.mk might be important. Weird, because I did a number of test > builds before pushing and never got a failure. > > Do you have the time to investigate? One of the failure, > http://autobuild.buildroot.net/results/3c8/3c8798f122a421e8fae741de76b2504c20e2da91/build-end.log, > is related to dav-ext. Indeed, you have moved this module outside the HTTP modules: ifeq ($(BR2_PACKAGE_NGINX_HTTP),y) ... endif # BR2_PACKAGE_NGINX_HTTP So now, linking fails if BR2_PACKAGE_NGINX_HTTP is not set. Should I rename this package to nginx-http-dav-ext to indicate it must be compiled with the HTTP server of nginx?
Hello, On Mon, 5 Dec 2016 14:20:43 +0100, Johan Oudinet wrote: > Indeed, you have moved this module outside the HTTP modules: > ifeq ($(BR2_PACKAGE_NGINX_HTTP),y) > ... > endif # BR2_PACKAGE_NGINX_HTTP > > So now, linking fails if BR2_PACKAGE_NGINX_HTTP is not set. > Should I rename this package to nginx-http-dav-ext to indicate it must > be compiled with the HTTP server of nginx? Hum, and the problem is I guess the same for the naxsi module. Renaming the package name is not necessary, we should handle this using dependencies. The previous solution used for the nginx-naxsi package was not good, because you could select BR2_PACKAGE_NGINX_NAXSI, but if BR2_PACKAGE_NGINX_HTTP was disabled, in fact the naxsi module was not built. So instead, I believe that nginx-naxsi/Config.in and nginx-dav-ext/Config.in should contain a: depends on BR2_PACKAGE_NGINX_HTTP Best regards, Thomas
On Mon, Dec 5, 2016 at 2:33 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Mon, 5 Dec 2016 14:20:43 +0100, Johan Oudinet wrote: > >> Indeed, you have moved this module outside the HTTP modules: >> ifeq ($(BR2_PACKAGE_NGINX_HTTP),y) >> ... >> endif # BR2_PACKAGE_NGINX_HTTP >> >> So now, linking fails if BR2_PACKAGE_NGINX_HTTP is not set. >> Should I rename this package to nginx-http-dav-ext to indicate it must >> be compiled with the HTTP server of nginx? > > Hum, and the problem is I guess the same for the naxsi module. Renaming > the package name is not necessary, we should handle this using > dependencies. > > The previous solution used for the nginx-naxsi package was not good, > because you could select BR2_PACKAGE_NGINX_NAXSI, but if > BR2_PACKAGE_NGINX_HTTP was disabled, in fact the naxsi module was not > built. > > So instead, I believe that nginx-naxsi/Config.in and > nginx-dav-ext/Config.in should contain a: > > depends on BR2_PACKAGE_NGINX_HTTP > Good point. Do you want me to submit a patch series for that, or do you take care of it? Best,
Hello, On Mon, 5 Dec 2016 14:41:29 +0100, Johan Oudinet wrote: > Good point. Do you want me to submit a patch series for that, or do > you take care of it? If you can submit a patch series, it would be good :) Thanks! Thomas
On Mon, Dec 5, 2016 at 2:53 PM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > On Mon, 5 Dec 2016 14:41:29 +0100, Johan Oudinet wrote: > >> Good point. Do you want me to submit a patch series for that, or do >> you take care of it? > > If you can submit a patch series, it would be good :) Done. Finally, I make BR2_PACKAGE_NGINX_DAV_EXT depends on BR2_PACKAGE_NGINX_HTTP_DAV_MODULE as it does not make sense if this nginx's module is not enabled. Best regards,
diff --git a/package/Config.in b/package/Config.in index 9ed296f..687c600 100644 --- a/package/Config.in +++ b/package/Config.in @@ -1544,6 +1544,7 @@ menu "Networking applications" source "package/nginx/Config.in" if BR2_PACKAGE_NGINX menu "External nginx modules" + source "package/nginx-dav-ext/Config.in" source "package/nginx-naxsi/Config.in" source "package/nginx-upload/Config.in" endmenu diff --git a/package/nginx-dav-ext/Config.in b/package/nginx-dav-ext/Config.in new file mode 100644 index 0000000..bf197dc --- /dev/null +++ b/package/nginx-dav-ext/Config.in @@ -0,0 +1,7 @@ +config BR2_PACKAGE_NGINX_DAV_EXT + bool "nginx-dav-ext" + select BR2_PACKAGE_EXPAT + help + NGINX WebDAV missing commands support (PROPFIND & OPTIONS). + + https://github.com/arut/nginx-dav-ext-module diff --git a/package/nginx-dav-ext/nginx-dav-ext.hash b/package/nginx-dav-ext/nginx-dav-ext.hash new file mode 100644 index 0000000..ea7eb86 --- /dev/null +++ b/package/nginx-dav-ext/nginx-dav-ext.hash @@ -0,0 +1,2 @@ +# Locally computed +sha256 d428a0236c933779cb40ac8c91afb19d5c25a376dc3caab825bfd543e1ee530d nginx-dav-ext-v0.0.3.tar.gz diff --git a/package/nginx-dav-ext/nginx-dav-ext.mk b/package/nginx-dav-ext/nginx-dav-ext.mk new file mode 100644 index 0000000..6c2b3a2 --- /dev/null +++ b/package/nginx-dav-ext/nginx-dav-ext.mk @@ -0,0 +1,12 @@ +################################################################################ +# +# nginx-dav-ext +# +################################################################################ + +NGINX_DAV_EXT_VERSION = v0.0.3 +NGINX_DAV_EXT_SITE = $(call github,arut,nginx-dav-ext-module,$(NGINX_DAV_EXT_VERSION)) + +NGINX_DAV_EXT_DEPENDENCIES = expat + +$(eval $(generic-package)) diff --git a/package/nginx/Config.in b/package/nginx/Config.in index e6f2d96..6f339c7 100644 --- a/package/nginx/Config.in +++ b/package/nginx/Config.in @@ -85,6 +85,16 @@ config BR2_PACKAGE_NGINX_HTTP_DAV_MODULE help Enable ngx_http_dav_module +if BR2_PACKAGE_NGINX_HTTP_DAV_MODULE + +config BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE + bool "ngx_http_dav_ext_module" + select BR2_PACKAGE_NGINX_DAV_EXT + help + Enable ngx_http_dav_ext_module + +endif # BR2_PACKAGE_NGINX_HTTP_DAV_MODULE + config BR2_PACKAGE_NGINX_HTTP_FLV_MODULE bool "ngx_http_flv_module" help diff --git a/package/nginx/nginx.mk b/package/nginx/nginx.mk index 6563627..c99139b 100644 --- a/package/nginx/nginx.mk +++ b/package/nginx/nginx.mk @@ -167,6 +167,11 @@ else NGINX_CONF_OPTS += --without-http_rewrite_module endif +ifeq ($(BR2_PACKAGE_NGINX_HTTP_DAV_EXT_MODULE)$(BR2_PACKAGE_NGINX_DAV_EXT),yy) +NGINX_DEPENDENCIES += nginx-dav-ext +NGINX_CONF_OPTS += --add-module=$(NGINX_DAV_EXT_DIR) +endif + NGINX_CONF_OPTS += \ $(if $(BR2_PACKAGE_NGINX_HTTP_REALIP_MODULE),--with-http_realip_module) \ $(if $(BR2_PACKAGE_NGINX_HTTP_ADDITION_MODULE),--with-http_addition_module) \
Nginx built-in support for webdav is missing support for two commands: PROPFIND and OPTIONS. Add an external module and an option in the nginx package to provide full webdav support. Signed-off-by: Johan Oudinet <johan.oudinet@gmail.com> --- package/Config.in | 1 + package/nginx-dav-ext/Config.in | 7 +++++++ package/nginx-dav-ext/nginx-dav-ext.hash | 2 ++ package/nginx-dav-ext/nginx-dav-ext.mk | 12 ++++++++++++ package/nginx/Config.in | 10 ++++++++++ package/nginx/nginx.mk | 5 +++++ 6 files changed, 37 insertions(+) create mode 100644 package/nginx-dav-ext/Config.in create mode 100644 package/nginx-dav-ext/nginx-dav-ext.hash create mode 100644 package/nginx-dav-ext/nginx-dav-ext.mk