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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable warnings as errors for clang-cl builds #2114

Merged
merged 2 commits into from Jun 12, 2022

Conversation

Bambo-Borris
Copy link
Contributor

Thanks a lot for making a contribution to SFML! 馃檪

Before you create the pull request, we ask you to check the follow boxes. (For small changes not everything needs to ticked, but the more the better!)

  • Has this change been discussed on the forum or in an issue before?
  • Does the code follow the SFML Code Style Guide?
  • Have you provided some example/test code for your changes?
  • If you have additional steps which need to be performed list them as tasks!

Description

As highlighted in #2046 the 2.6.x & master branches both produce compile errors when using either clang++ or clang-cl. This PR supresses some warnings which are not supported by clang-cl, and also disables /WX due to an issue where dependencies from extlib/ were being compiled with /WX when they shouldn't have been. This would cause the CI to fail due to /WX, so this option has been temporarily disabled for Windows + clang-cl builds.

For CI logs showing the issue with the clang-cl + /WX see here

This does not resolve the compiler errors that originate from using clang++ on Windows (the GNU-like interface). Though I intend to add a follow up PR which will fix the clang++ build and add the necessary CI to confirm it's working.

This PR is related to the issue #

Tasks

  • Tested on Linux
  • Tested on Windows
  • Tested on macOS
  • Tested on iOS
  • Tested on Android

How to test this PR?

Compile with clang-cl on Windows.

Copy link
Member

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

After disabling warnings-as-errors for ClangCL, do we still need all the preprocessor changes you made in other files or can that wait for later once we finally fix the compiler warnings we're currently working around?

EDIT: The preprocessor changes are not required to fix the new ClangCL CI job so I'd prefer we keep those changes in a separate commit to better express why they exist.

cmake/CompilerWarnings.cmake Outdated Show resolved Hide resolved
@Bambo-Borris
Copy link
Contributor Author

Bambo-Borris commented Jun 5, 2022

After disabling warnings-as-errors for ClangCL, do we still need all the preprocessor changes you made in other files or can that wait for later once we finally fix the compiler warnings we're currently working around?

If we remove the preprocessor changes the build will still succeed, but it will leave the known warnings in place. It possible for these to be pushed in a separate commit if that's what is favoured overall. As mentioned in the description there is to be a follow up PR for this which is for the GNU-like interface to build. So maybe after the GNU-like interface these macros can go in on a third commit.

@Bambo-Borris Bambo-Borris force-pushed the bugfix/werror-disable-clangcl branch from e7c0065 to 66569af Compare June 5, 2022 19:05
Copy link
Member

@ChrisThrasher ChrisThrasher left a comment

Choose a reason for hiding this comment

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

Great work!

@Bambo-Borris
Copy link
Contributor Author

Important to note, I did go through the CI logs to verify /WX/-Werror were present on the relevant builds that should retain them. It was unlikely that they would be effected, but good to be certain.

@Bambo-Borris
Copy link
Contributor Author

@Bromeon I removed the preprocessor stuff so that this commit is just the changes required to enable the ClangCL to build. It's ready for a review again whenever you're able to

@eXpl0it3r eXpl0it3r added this to Discussion in SFML 2.6.0 via automation Jun 9, 2022
@eXpl0it3r eXpl0it3r added this to the 2.6 milestone Jun 9, 2022
@eXpl0it3r eXpl0it3r moved this from Discussion to Review & Testing in SFML 2.6.0 Jun 9, 2022
@vittorioromeo
Copy link
Member

This only affects 2.6.x, right? Will this backmerged in 3.x? If so, will it prevent any useful warning-as-error from being generated?

@Bambo-Borris
Copy link
Contributor Author

Bambo-Borris commented Jun 9, 2022

This only affects 2.6.x, right? Will this backmerged in 3.x? If so, will it prevent any useful warning-as-error from being generated?

It's targeted at 2.6.x but is applicable to 3.x but I don't know if it will be backmerged. It may prevent us spotting some useful warnings-as-errors, but it's a temporary solution to allow ClangCl to actually build 2.6.x. Currently 2.6.x won't build via ClangCl due to the issue highlighted in the description. There was discussion of this in Discord which provides additional information for those curious. I haven't tested building 3.x with ClangCl for a while so it's something I'm open to verifying if there's a problem there too.

@Bambo-Borris
Copy link
Contributor Author

Is there any additional concerns about this PR or is it good to merge?

SFML 2.6.0 automation moved this from Review & Testing to Ready Jun 12, 2022
@eXpl0it3r eXpl0it3r merged commit 12c091e into SFML:2.6.x Jun 12, 2022
SFML 2.6.0 automation moved this from Ready to Done Jun 12, 2022
@eXpl0it3r
Copy link
Member

Another 馃帀 for the Big Blue Bambooooo 馃檪

@eXpl0it3r
Copy link
Member

This only affects 2.6.x, right? Will this backmerged in 3.x? If so, will it prevent any useful warning-as-error from being generated?

It will be back merged to master.

The issue right now is that SFML doesn't compile, neither on 2.6.x nor on master with MSVC + Clang-Cl.
So in order to not block people for now, the treat warnings as errors is being disable and it gives us time to fix them either on 2.6.x or master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
SFML 2.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

5 participants