Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Add support for not breaking build on warnings #12039

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Add support for not breaking build on warnings #12039

merged 1 commit into from
Jun 2, 2017

Conversation

omajid
Copy link
Member

@omajid omajid commented Jun 1, 2017

Add a build flag to make -Werror optional and let the build continue even in the presence of warnings.

This option is very useful for anyone compiling with a different (version of the) compiler. A different (version of the) compiler may produce a different set of warnings and a piece of code that compiles
without warnings may emit warnings with a different (version of the) compiler.

Resolves https://github.com/dotnet/coreclr/issues/8586

@omajid
Copy link
Member Author

omajid commented Jun 1, 2017

I am totally not sure of the best name for the cmake option and the flag to build.sh. Any suggestions are welcome!

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

Besides the comment, it looks good. The option name and the cmake variable name (modulo the comment) sounds good to me too.

CMakeLists.txt Outdated
@@ -32,6 +32,7 @@ if(CORECLR_SET_RPATH)
endif(CORECLR_SET_RPATH)

OPTION(CMAKE_ENABLE_CODE_COVERAGE "Enable code coverage" OFF)
OPTION(CMAKE_WARNINGS_ARE_ERRORS "Warnings are errors" ON)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please name the option CLR_CMAKE_WARNINGS_ARE_ERRORS instead? Besides that option above that I've not noticed during past PRs, we don't use options starting with CMAKE_, since that is reserved for variables of the cmake itself. And if you could also rename the CMAKE_ENABLE_CODE_COVERAGE, it would be awesome.

@omajid
Copy link
Member Author

omajid commented Jun 2, 2017

Thanks for the review. I will make the suggested changes. One more question: do I need to implement this on the Windows side as well (build.cmd)? (I dont have a Windows box to test this on).

Add a build flag to make -Werror optional and let the build continue
even in the presence of warnings.

This option is very useful for anyone compiling with a different
(version of the) compiler. A different (version of the) compiler may
produce a different set of warnings and a piece of code that compiles
without warnings may emit warnings with a different (version of the)
compiler.

Resolves https://github.com/dotnet/coreclr/issues/8586
@janvorli
Copy link
Member

janvorli commented Jun 2, 2017

@omajid while it would be nice to do it on Windows side too, someone else can do it over there if you don't have windows box. Please just create an issue for it.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@omajid
Copy link
Member Author

omajid commented Jun 2, 2017

No problem! I filed https://github.com/dotnet/coreclr/issues/12061. I will open a separate PR for CMAKE_* cmake variable names.

@janvorli janvorli merged commit f7ce377 into dotnet:master Jun 2, 2017
@omajid
Copy link
Member Author

omajid commented Jun 2, 2017

Thanks for merging this!

ellismg pushed a commit to ellismg/coreclr that referenced this pull request Jun 7, 2017
Add a build flag to make -Werror optional and let the build continue
even in the presence of warnings.

This option is very useful for anyone compiling with a different
(version of the) compiler. A different (version of the) compiler may
produce a different set of warnings and a piece of code that compiles
without warnings may emit warnings with a different (version of the)
compiler.

Resolves https://github.com/dotnet/coreclr/issues/8586
@karelz karelz modified the milestone: 2.1.0 Aug 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants