-
Notifications
You must be signed in to change notification settings - Fork 354
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
Enforce Werror when building samples on Linux #3816
base: master
Are you sure you want to change the base?
Enforce Werror when building samples on Linux #3816
Conversation
bors try |
tryBuild failed: |
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
-fno-builtin>) | ||
-fno-builtin> | ||
# Treat warnings as errors by default. Application based on OE SDK | ||
# will inhere this flag, which is consistent to the behavior of the |
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.
# will inhere this flag, which is consistent to the behavior of the | |
# will inherit this flag, which is consistent to the behavior of the |
@@ -275,7 +275,11 @@ if (OE_SGX) | |||
# user. There are valid reasons for an end user to use built-ins. | |||
$<BUILD_INTERFACE:-fno-builtin-malloc | |||
-fno-builtin-calloc | |||
-fno-builtin>) | |||
-fno-builtin> | |||
# Treat warnings as errors by default. Application based on OE SDK |
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.
Is this desirable? This means that every application that uses liboecore via CMake import or pkgconfig will get -Werror
by default, not just the samples. It's not generally my experience that SDKs set this on behalf of a dev (folks should feel to correct this if I am mistaken.)
This is also potentially a public build breaking change and should at least be documented in the CHANGELOG.md.
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.
Good point. I'm unapproving till we are sure that we don't force these flags on the users.
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.
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.
There is a distinction:
- We want the samples to be built using -Werror
- However, we don't want to force -Werror on the user.
My understanding is that, today, we don't force the flags on the user on both Linux and Windows.
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.
No. -Werror is added in the clangw by default, so samples on Windows are always compiled with the flag, which is what the #3773 about.
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.
Thanks for the clarification. We were forcing the flag on Windows, whereas we weren't on Linux.
I'm removing my blocker.
It is a matter of consistency then.
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.
Exactly. Given that @CodeMonkeyLeet pointed out that we shouldn't enforce -Werror on behalf of users, I'm wondering how should we address the consistency? This PR currently follows the conclusion from triaging #3733. We could also remove the enforcement on the Windows side. Thoughts?
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.
There are convincing arguments for both choices. Thinking about it bit more, my preference would be to enforce -Werror since it is generally considered a good programming practice to compile code with high warning levels.
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.
+1 to Anand's summary before about what is desired state:
- We want the samples to be built using -Werror
- However, we don't want to force -Werror on the user.
Above should be true for both Linux and Windows.
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.
We need to make sure that we are not forcing these flags on the users.
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.
Reapproving.
Signed-off-by: Ming-Wei Shih <mishih@microsoft.com>
e5424c4
to
704c93c
Compare
@dthaler, please review |
bors try |
tryBuild failed: |
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.
Samples are failing. We should not change the flags that samples are built with since that could cause breaks in current code.
This PR fixes #3773, which enforces
Werror
flag when building samples on Linux.Signed-off-by: Ming-Wei Shih mishih@microsoft.com