Skip to content
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

port ngx_udp_connect() to fix compilation errors with nginx 1.12.0+ #30

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

trungtm
Copy link

@trungtm trungtm commented May 24, 2017

Since nginx 1.12.0, ngx_udp_connect() is never publicly available, and never have public declaration.
So I backport the function from nginx source code to fix compilation errors

Issue: #28

more info:
https://trac.nginx.org/nginx/ticket/1244

else
HTTP_MODULES="$HTTP_MODULES ngx_http_statsd_module"
NGX_ADDON_SRCS="$NGX_ADDON_SRCS $ngx_addon_dir/ngx_http_statsd.c"
CORE_LIBS="$CORE_LIBS -lssl"
Copy link

@levb levb Jul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you still need -lssl? I believe that USE_OPENSSL=YES already accomplishes that. Or is there an issue with older nginx versions?

I had to remove -lssl in my fork anyway since we try to link with openssl statically, and this -lssl was screwing it up on CentOS/RedHat.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I just converted the old config to the new one
USE_OPENSSL=YES is better option.

@Eriten
Copy link

Eriten commented Jun 21, 2019

Just wondering what else is getting blocked from merging this PR? It'd be nice to have this fixed, so we can get it supported for new nginx versions!

@sane4ek-2
Copy link

@levb could you tell us if this PR could be merged into the main branch? This fix would help us to get rid of ngx_udp_connect problem.

@levb
Copy link

levb commented Mar 25, 2022

@sane4ek-2 I am not working on nginx modules any longer, so do not have a strong opinion. However, I think my comment about USE_OPENSSL=YES being sufficient was accurate at the time, so perhaps the (simple?) change can still be made.

@sane4ek-2
Copy link

@zacman85 hello. Could you merge this fix into the main branch? It looks like everything works fine. I could build this module with Nginx 1.20.2 without any problems.

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.

None yet

4 participants