phusion / passenger

A fast and robust web server and application server for Ruby, Python and Node.js
https://www.phusionpassenger.com/
MIT License
5.01k stars 547 forks source link

Fix upstream_config.limit_rate initialization for Nginx 1.27.0 compatibility #2548

Closed chenrui333 closed 3 months ago

chenrui333 commented 5 months ago

seeing some regression build failure when building against nginx 1.27.0

/private/tmp/passenger-20240529-10206-38vspk/passenger-6.0.22/src/nginx_module/Configuration.c:228:42: error: incompatible integer to pointer conversion assigning to 'ngx_http_complex_value_t *' from 'size_t' (aka 'unsigned long') [-Wint-conversion]
conf->upstream_config.limit_rate = NGX_CONF_UNSET_SIZE;

full log ref, https://github.com/Homebrew/homebrew-core/actions/runs/9291204641/job/25569305049?pr=173145

upstream commit ref, https://github.com/nginx/nginx/commit/71ca978a352e025151a78bfcedc0d64814b062cb

relates to https://github.com/Homebrew/homebrew-core/pull/173145

CamJN commented 5 months ago

The homebrew formula for passenger is very broken at this point, perhaps you should let me submit PRs again?

Either way you'd need to sign the contributor agreement to have your changes accepted.

chenrui333 commented 5 months ago

The homebrew formula for passenger is very broken at this point, perhaps you should let me submit PRs again?

what prevents you submitting the PRs? happy to help on that.

Either way you'd need to sign the contributor agreement to have your changes accepted.

Yeah, I just signed the CLA

CamJN commented 5 months ago

I made this comment on hacker news: https://news.ycombinator.com/item?id=25183679 and was banned from ever interacting with the homebrew org on github thereafter. I even think they misatributed the follow up comments by another user to me. Amusingly homebrew did wind up doing exactly what I predicted with the ARM rollout, putting x86 libs and binaries in /usr/local to be found automatically by software that uses normal search paths, and arm libs and binaries in /opt/homebrew which pretty much nothing uses. So the worst possible thing for the users.

thresheek commented 5 months ago

I don't believe the patch is fully fixing the issue, as it will introduce a new warning.

Something like the following should be applied as well:

diff --git a/src/nginx_module/Configuration.c b/src/nginx_module/Configuration.c
index 297c71ba8..737f21598 100644
--- a/src/nginx_module/Configuration.c
+++ b/src/nginx_module/Configuration.c
@@ -467,7 +467,10 @@ passenger_merge_loc_conf(ngx_conf_t *cf, void *parent, void *child)
                               prev->upstream_config.buffer_size,
                               16 * 1024);

-    #if NGINX_VERSION_NUM >= 1007007
+    #if NGINX_VERSION_NUM >= 1027000
+        ngx_conf_merge_ptr_value(conf->upstream_config.limit_rate,
+                                  prev->upstream_config.limit_rate, NULL);
+    #elif NGINX_VERSION_NUM >= 1007007
         ngx_conf_merge_size_value(conf->upstream_config.limit_rate,
                                   prev->upstream_config.limit_rate, 0);
     #endif