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

C compilation fixes and hardening #501

Closed
wants to merge 1 commit into from
Closed

Conversation

tiran
Copy link
Member

@tiran tiran commented Feb 24, 2017

Fix "implicit declaration of function ‘strlen’" in ipa_pwd_ntlm.c,
credits to Lukas.

Add -Werror=implicit-function-declaration to CFLAGS to point developers
to missing includes. It causes compilation to fail when a developer
forgets to add a required include. The problem is no longer hidden in a
massive wall of text from make.

Signed-off-by: Christian Heimes cheimes@redhat.com

Fix "implicit declaration of function ‘strlen’" in ipa_pwd_ntlm.c,
credits to Lukas.

Add -Werror=implicit-function-declaration to CFLAGS to point developers
to missing includes. It causes compilation to fail when a developer
forgets to add a required include. The problem is no longer hidden in a
massive wall of text from make.

Silence a harmless error from 389-DS slapi.h until the bug is fixed in
downstream, https://pagure.io/389-ds-base/issue/48979

Signed-off-by: Christian Heimes <cheimes@redhat.com>
@stlaz
Copy link
Contributor

stlaz commented Mar 1, 2017

I agree that C compilation should be hardened for FreeIPA, seeing warnings is nothing unusual here. This builds fine. ACK.

@stlaz stlaz added the ack Pull Request approved, can be merged label Mar 1, 2017
@MartinBasti
Copy link
Contributor

master:

  • 2828a2b C compilation fixes and hardening

@MartinBasti MartinBasti added the pushed Pull Request has already been pushed label Mar 1, 2017
@MartinBasti MartinBasti closed this Mar 1, 2017
@lslebodn
Copy link
Contributor

lslebodn commented Mar 1, 2017

FYI; it is far far away from best practice to modify CFLAGS and configure time; unless you test compiler options. Such change should be in makefile AM_CFLAGS

In the future, try to address comments in PR (#364) rather then ignoring them and then fixing in new PR

@tiran tiran deleted the compile_fix branch March 1, 2017 10:03
@tiran
Copy link
Member Author

tiran commented Mar 1, 2017

@lslebodn feel free to open a new PR. This PR and #364 are closed.

@lslebodn
Copy link
Contributor

lslebodn commented Mar 1, 2017

I know it's closed.

It was just a kindly reminder to address obvious problems as part of review process.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
4 participants