New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support building ngx_pagespeed as a dynamic module #1115

Merged
merged 1 commit into from Feb 16, 2016

Conversation

Projects
None yet
3 participants
@oschaaf
Copy link
Member

oschaaf commented Feb 10, 2016

As of 1.9.11, nginx supports loading dynamic modules
This change makes us support building ngx_pagespeed.so

Fixes #1116

@PiotrSikora

This comment has been minimized.

Copy link
Contributor

PiotrSikora commented Feb 10, 2016

This looks very wrong to me, this patch:

  1. always compilies both modules as static modules, even when compiling as a dynamic modules (using --add-dynamic-module),
  2. misuses ngx_module_order, which doesn't work with filters running after current filter, only earlier ones.

See: google/ngx_brotli@86998c6

@oschaaf oschaaf force-pushed the oschaaf-trunk-tracking-dynamic-modules branch from 87ef771 to 297cee1 Feb 10, 2016

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Feb 10, 2016

@PiotrSikora Updated and force pushed. This will need to be rebased, but does this look better?

@PiotrSikora

This comment has been minimized.

Copy link
Contributor

PiotrSikora commented Feb 10, 2016

@oschaaf: looks good, but it seems that you also need to modify config.make in order to pass --std=c++11 when compiling with clang, otherwise it doesn't build.

Also, ngx_http_brotli_filter_module and ngx_http_gzip_filter_module should switch places.

@oschaaf oschaaf force-pushed the oschaaf-trunk-tracking-dynamic-modules branch from 297cee1 to 1748d93 Feb 11, 2016

@oschaaf oschaaf changed the title dynamic-modules: changes to support building dynamic modules Support building ngx_pagespeed as a dynamic module Feb 11, 2016

config Outdated
else
module=ngx_http_range_header_filter_module
# config.make is not executed for dynamic modules
CFLAGS="$CFLAGS -Wno-c++11-extensions"

This comment has been minimized.

@oschaaf

oschaaf Feb 11, 2016

Member

@PiotrSikora config.make is not executed for dynamic modules, see https://github.com/nginx/nginx/blob/master/auto/make#L436

So this is the best I can come up with to allow us to build ngx_pagespeed.so with clang

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Feb 11, 2016

@PiotrSikora Comments addressed & force pushed

config Outdated
if [ $ngx_module_link != DYNAMIC ]; then
# ngx_module_order doesn't work with static modules,
# so we must re-order filters here.
HTTP_FILTER_MODULES=`echo $HTTP_FILTER_MODULES \

This comment has been minimized.

@PiotrSikora

PiotrSikora Feb 12, 2016

Contributor

Style: this module uses $(...) for shell commands, so we should be consistent about it.

config Outdated
HTTP_FILTER_MODULES=`echo $HTTP_FILTER_MODULES \
| sed "s/ngx_pagespeed_etag_filter//" \
| sed "s/ngx_http_range_header_filter_module/ngx_http_range_header_filter_module ngx_pagespeed_etag_filter/"`
echo "$HTTP_FILTER_MODULES"

This comment has been minimized.

@PiotrSikora

PiotrSikora Feb 12, 2016

Contributor

This probably isn't needed anymore ;)

config Outdated
. auto/module

if [ $ngx_module_link != DYNAMIC ]; then
# ngx_module_order doesn't work with static modules,

This comment has been minimized.

@PiotrSikora

PiotrSikora Feb 12, 2016

Contributor

Style: this module uses 2 spaces for indent, not 4.

CORE_LIBS="$CORE_LIBS $pagespeed_libs"
CORE_INCS="$CORE_INCS $pagespeed_include"
NGX_ADDON_SRCS="$PS_NGX_SRCS"
if [ "$position_aux" = "true" ] ; then

This comment has been minimized.

@PiotrSikora

PiotrSikora Feb 12, 2016

Contributor

This feature is ignored in nginx-1.9.11+ code path, is that desirable behavior? Also, I'm not clear on what this is supposed to achieve in the first place...

@PiotrSikora

This comment has been minimized.

Copy link
Contributor

PiotrSikora commented Feb 12, 2016

@oschaaf: thanks, LGTM (other than the stuff that I commented on, which can be mostly ignored).

Support building ngx_pagespeed as a dynamic module.
As of 1.9.11, nginx supports loading dynamic modules
This change makes us support building ngx_pagespeed.so

Fixes #1116

@oschaaf oschaaf force-pushed the oschaaf-trunk-tracking-dynamic-modules branch from 1748d93 to 09f5388 Feb 13, 2016

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Feb 13, 2016

@PiotrSikora Thanks for reviewing! I updated to address your latest comments.

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Feb 16, 2016

@oschaaf is this ready to merge?

@oschaaf

This comment has been minimized.

Copy link
Member

oschaaf commented Feb 16, 2016

@crowell yes

@crowell

This comment has been minimized.

Copy link
Contributor

crowell commented Feb 16, 2016

LGTM, backported and tested on master.

crowell added a commit that referenced this pull request Feb 16, 2016

Merge pull request #1115 from pagespeed/oschaaf-trunk-tracking-dynami…
…c-modules

 Support building ngx_pagespeed as a dynamic module

@crowell crowell merged commit d959f01 into trunk-tracking Feb 16, 2016

@oschaaf oschaaf deleted the oschaaf-trunk-tracking-dynamic-modules branch Jul 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment