-
Notifications
You must be signed in to change notification settings - Fork 210
Respecting user provided CFLAGS. #993
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
Conversation
|
As far as I understand, the compiler warnings (not I think a better approach is to have certain required CFLAGS, like |
|
What I imagine is something like |
that is exactly what this patch does, it also respects flags provided by "NJS_REQUIRED_CFLAGS" - is provided through After it cc flags are provided as |
|
Oh yes true. What about warnings then? I would imagine you would want to keep those even if CFLAGS are set |
Reading https://www.gnu.org/prep/standards/html_node/Command-Variables.html, I conclude that we should not include |
It is customary for many projects to include
Their sole effect is to make the logs from building, more verbose if warnings are produced. Also it makes it easier for users to set -Werror in their CFLAGS and have it combine with the project's "expected list of warnings to care about". Opinions therefore vary. Many projects unconditionally set -Wall, while others only set it when CFLAGS is not set. Both interpretations have been passionately defended by citing the GNU standards manual as a source. If you are interested in the alternative interpretation I can provide some details. |
|
Hi @eli-schwartz, Thank you for the feedback.
Yes, I am interested. |
See https://www.gnu.org/prep/standards/html_node/Command-Variables.html and discussion in nginx#990 issue on Github.
VadimZhestikov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
BalkanMadman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thank you.
Previously, when js_body_filter was used inside an if block that evaluated to true, the data parameter received Buffer type instead of the expected String type. This happened because buffer_type field in ngx_http_js_loc_conf_t was not properly initialized, causing the configuration merge to fail when nginx created a new location context for if blocks. This fixes #999 issue on Github.
No description provided.