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

Add -Werror as a default. #1193

Merged
merged 1 commit into from Jun 1, 2023
Merged

Add -Werror as a default. #1193

merged 1 commit into from Jun 1, 2023

Conversation

yossigo
Copy link
Member

@yossigo yossigo commented Jun 1, 2023

Addresses the valid concerns raised in #1190 (comment).

The other changes resolve minor issues which fail the current CI on different platforms.

Copy link
Collaborator

@michael-grunder michael-grunder left a comment

Choose a reason for hiding this comment

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

I think the only real risk here is whether it's going to cause some heartburn for package maintainers, but I'm sure they'll let us know!

@yossigo yossigo merged commit 5cbd1f2 into redis:master Jun 1, 2023
11 checks passed
@yossigo yossigo deleted the add-werror branch June 1, 2023 16:03
@Romain-Geissler-1A
Copy link
Contributor

Normally packages which ensure -Werror also do provide a way to explicitly disable this, as warnings are something "volatile" which can happen with new compilers (including non released ones), and also with LTO combined with warnings that generate quite some false positives, it makes the build quite error prone for packagers.

Would you be ok to add an opt-out option to optionally (and explicitly) turn off warnings for packagers ?

@yossigo
Copy link
Member Author

yossigo commented Jul 13, 2023

@Romain-Geissler-1A This is a valid point, but can't you already do that by overriding the WARNINGS Makefile variable to whatever suits your platform? This is even more powerful than dropping -Werror, as you can adjust specific flags as needed.

@Romain-Geissler-1A
Copy link
Contributor

This hardly scales. When you package many packages, and each of them decide to go in its own way, it makes it more difficult for packaging maintainers to keep up with upstream changes. Even if it looks trivial for one project (like hiredis), it's for me not a solution.

You only want -Werror in your own CI by the way, or in "development mode" when you built it locally. You don't want -Werror in general when people only consume hiredis and don't intend to develop/fix anything in hiredis. As said, other projects just ensure this -Werror for "development" mode and don't enforce it by default in their builds (or if they do, they provide a way to explicitly disable -Werror). Typically this is what glibc does, with a configure flag --disable-werror.

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

Successfully merging this pull request may close these issues.

None yet

3 participants