Skip to content

1.9.0 Compatibility Fixes (See #497, #498, #499) #500

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

Closed
wants to merge 1 commit into from

Conversation

charlesportwoodii
Copy link

Fixes compilation issue outlined in #499.

Edit

Just saw #497 and #498. This patch might be a little cleaner than that as it maintains compatibility with 1.3->1.8, rather than just dropping support for everything less.

@charlesportwoodii
Copy link
Author

@msva,

See the second commit, cddb16d. ngx_http_set_connection_log() is still present for nginx_version < 100900 && nginx_version >= 1003014

The full code block is:

#if defined(nginx_version) && nginx_version >= 100900

    ngx_set_connection_log(r->connection, clcf->error_log);

#elif defined(nginx_version) && nginx_version < 100900 && nginx_version >= 1003014

        ngx_http_set_connection_log(r->connection, clcf->error_log);

#else

    c->log->file = clcf->error_log->file;

    if (!(c->log->log_level & NGX_LOG_DEBUG_CONNECTION)) {
        c->log->log_level = clcf->error_log->log_level;
    }

#endif

@msva
Copy link

msva commented Apr 28, 2015

Ok then ;)

@msva
Copy link

msva commented Apr 28, 2015

Btw, as reference for @agentzh : http://hg.nginx.org/nginx/rev/187aa751ad62

@msva
Copy link

msva commented Apr 28, 2015

btw, @charlesportwoodii
Before @agentzh merged this PR, can you make a code a bit nice-looking and strip unneded whitespaces and newlines (and you can use commit --amend and push -f to do not make additional commits in PR)? :)

@msva
Copy link

msva commented Apr 28, 2015

Although, you can combine them both in the single commit with the same way ;)

@charlesportwoodii
Copy link
Author

@msva,

Rebased to a single commit, and cleaned up some newline/whitespace as requested. Let me know if you would like to see anything else.

@charlesportwoodii
Copy link
Author

Ah, I see what you're talking about now. I was following the coding style @agentzh had in both files:

#if defined(nginx_version) && nginx_version >= 1003014

    ngx_http_set_connection_log(r->connection, clcf->error_log);

#else

    c->log->file = clcf->error_log->file;

    if (!(c->log->log_level & NGX_LOG_DEBUG_CONNECTION)) {
        c->log->log_level = clcf->error_log->log_level;
    }

#endif

To

#if defined(nginx_version) && nginx_version >= 100900

    ngx_set_connection_log(r->connection, clcf->error_log);

#elif defined(nginx_version) && nginx_version < 100900 && nginx_version >= 1003014

    ngx_http_set_connection_log(r->connection, clcf->error_log);

#else

    c->log->file = clcf->error_log->file;

    if (!(c->log->log_level & NGX_LOG_DEBUG_CONNECTION)) {
        c->log->log_level = clcf->error_log->log_level;
    }

#endif

Let me know if you want to keep it as is, or strip out all the newlines from the pre-processor block. I'm fine with either - just trying to following whatever convention is already setup. =)

#if defined(nginx_version) && nginx_version >= 100900
    ngx_set_connection_log(r->connection, clcf->error_log);
#elif defined(nginx_version) && nginx_version < 100900 && nginx_version >= 1003014
    ngx_http_set_connection_log(r->connection, clcf->error_log);
#else
    c->log->file = clcf->error_log->file;
    if (!(c->log->log_level & NGX_LOG_DEBUG_CONNECTION)) {
        c->log->log_level = clcf->error_log->log_level;
    }
#endif

@charlesportwoodii
Copy link
Author

Flattened down as requested in 29898f5

#if defined(nginx_version) && nginx_version >= 100900
    ngx_set_connection_log(r->connection, clcf->error_log);
#elif defined(nginx_version) && nginx_version < 100900 && nginx_version >= 1003014
    ngx_http_set_connection_log(r->connection, clcf->error_log);
#else
    c->log->file = clcf->error_log->file;
    if (!(c->log->log_level & NGX_LOG_DEBUG_CONNECTION)) {
        c->log->log_level = clcf->error_log->log_level;
    }
#endif

@teward
Copy link

teward commented Apr 29, 2015

(#503 is a duplicate of this.)

@blablacio blablacio mentioned this pull request May 4, 2015
@teward
Copy link

teward commented May 4, 2015

Any update on the status of this ever being included/merged?

@BotoX
Copy link

BotoX commented May 6, 2015

Still waiting for this to be fixed... :(

@agentzh
Copy link
Member

agentzh commented May 7, 2015

@charlesportwoodii Thanks for the patch! But will you restore the original empty lines (which were there for better readability)? Thanks!

@agentzh
Copy link
Member

agentzh commented May 7, 2015

Oh, BTW, sorry for the delay on my side. I've been on my long vacation in China where I don't have sane Internet access for most of the time. Alas.

@msva
Copy link

msva commented May 7, 2015

Btw, @charlesportwoodii, as I previously said, please don't restore trailing whitespaces on empty lines (that, actually, was the original issue) 😉
// btw, In line-comments in the amended commit I also suggested to remove only whitespaces and wait with newlines for @agentzh judgement, but nvm. 😉
// btw2, it seems there is some more changes that can affect ngx_lua after next release.

@agentzh
Copy link
Member

agentzh commented May 7, 2015

@msva Yeah, line-trailing whitespaces should always be removed :)

agentzh added a commit that referenced this pull request May 7, 2015
@agentzh
Copy link
Member

agentzh commented May 7, 2015

Applied a modified version of the patch to master. Thank you all, guys.

@agentzh agentzh closed this May 7, 2015
charlesportwoodii added a commit to charlesportwoodii/nginx-build that referenced this pull request May 7, 2015
phunehehe added a commit to phunehehe/nixpkgs that referenced this pull request Sep 22, 2015
@msva
Copy link

msva commented Sep 24, 2015

it is very strange, because I just built it fine on about 10 servers in a row
:)

@yurevich1
Copy link

@msva my fault.UPdate - all works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants