Cleanup static checks and code samples #181
Conversation
* Enable ASAN and UBSAN by default * Remove std::endl in sample code * Remove project specific clang-tidy checks
* These are checks that are contradictory or do not necessarily aid in writing good code
…efticus/cpp_starter_project into cleanup_static_checks_and_code_samples
There was a problem hiding this comment.
Enabling WARNINGS_AS_ERRORS means that cppcheck and clang-tidy need to be installed. We need to use setup-cpp to install them on all operating systems.
I added an example for automating the addition of sanitizers when running tests (see the bottom of the readme of project_options). I excluded Windows in that example.
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
Co-authored-by: Amin Yahyaabadi <aminyahyaabadi74@gmail.com>
|
I just realized that this was not a Draft PR, which was the intention for now. |
Codecov Report
@@ Coverage Diff @@
## main #181 +/- ##
============================================
+ Coverage 71.42% 100.00% +28.57%
============================================
Files 3 2 -1
Lines 28 20 -8
Branches 14 0 -14
============================================
Hits 20 20
+ Misses 8 0 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
I made the PR I had promised regarding the CI. #183 |
Awesome, thank you @aminya The work that you have done in getting everything modular and reusable is amazing. I think it really will now let us focus on making the cpp_starter_project itself what I always intended for it to be. That is: "the ultimate CMake and C++ quick start" I want a user with no previous C++ experience to be able to pull this up and in a few minutes be going with
I just put the template to use for myself for the first time in awhile, and it's this last part that I'm hoping to tweak. I never intended this to be the default settings for a library developer. Also, after having used it recently, I don't see a strong need to remove the sample bits in there. We can still let users pull them in as desired (indeed, I used the imgui example for my current project). I can see a desire to possibly have a "cpp_library_starter_project" that has different defaults, perhaps? tl;dr the point of this PR will be to make Best Practices the default and refine the user experience |
|
Thank you so much! I agree with setting good defaults as long as it considers all the platforms. I highly suggest that you edit the project_options source instead of adding your edits to this CMakeLists.txt here. You should be able to add these same options in GlobalOptions file. https://github.com/cpp-best-practices/project_options/blob/main/src/GlobalOptions.cmake |
|
I'm going to take several more passes at working out the ergonomics for windows/linux cross development and settle on some settings that work consistently before I worry about what should end up where. But I do think it makes sense to keep your work as reusable as possible. Just have to ask "where does it make the most sense for defaults to live?" In this project, or in the reusable bits? We've already made the decision to put a bunch of warnings into the reusable parts... |
|
We have tried to automate setting the good defaults in For examples:
I think we should automate as much as possible. If something is not automatable, I do not think setting it in For example, if CMake had a standard # ❗ enable sanitizers if running the tests
option(FEATURE_TESTS "Enable the tests" OFF)
if(FEATURE_TESTS)
if(CMAKE_SYSTEM_NAME STREQUAL "Windows")
set(ENABLE_SANITIZER_ADDRESS OFF)
set(ENABLE_SANITIZER_UNDEFINED_BEHAVIOR OFF)
else()
set(ENABLE_SANITIZER_ADDRESS "ENABLE_SANITIZER_ADDRESS")
set(ENABLE_SANITIZER_UNDEFINED_BEHAVIOR "ENABLE_SANITIZER_UNDEFINED_BEHAVIOR")
endif()
endif()
|
conan's documenation on cmake integration recommends using the `CONFIG` parameter when using find_package
But this is one particular point where our opinions seem to differ. I want ASAN enabled by default for all builds on all platforms, not just when testing is enabled. And UBSAN by default on gcc and clang targets. This will go long way towards teaching people how to have a safe C++ setup by default, and they can easily disable it when they want to. I do already have ASAN working with no problem in MSVC in the IDE. There was just one setting left to set that none of the Microsoft documentation tells you about. |
CMakeLists.txt
Outdated
| ${ENABLE_USER_LINKER} | ||
| ${ENABLE_BUILD_WITH_TIME_TRACE} # generates report of where compile-time is spent | ||
| ${ENABLE_UNITY} # merge C++ files into larger C++ files, can speed up compilation sometimes | ||
| ${ENABLE_SANITIZER_ADDRESS} # make memory errors into hard runtime errors (windows/linux/macos) |
There was a problem hiding this comment.
This is all it takes to enable ASAN by default. There is no need for adding all these global options.
| ${ENABLE_SANITIZER_ADDRESS} # make memory errors into hard runtime errors (windows/linux/macos) | |
| ENABLE_SANITIZER_ADDRESS # make memory errors into hard runtime errors (windows/linux/macos) |
There was a problem hiding this comment.
That does not enable asan by default. Instead, it hardcodes the ENABLE_SANITIZER_ADDRESS and gives the user a button in the cmake-guis that does not do anything.
The work I have prototyped in here is so that a user of one of the GUIs for changing CMake settings has buttons that work how you would expect them to work. All the buttons do something and properly reflect the state of the system.
I just did 3 different clean (full delete of the build folder) builds with your suggestions to verify that what I did was necessary to make both worlds work, but I may still be missing something.
There was a problem hiding this comment.
Either way, I'm still prototyping here, which is why I made it a draft PR. I think once I come up with something that I think works, it would be wise for us to have a zoom chat or something and discuss the best ways for it to work within the framework you've mad.e
There was a problem hiding this comment.
it would be wise for us to have a zoom chat or something and discuss the best ways for it to work within the framework you've made
Sure. That's a good idea! I am available for Zoom chats in the evenings after 5 PM (PST time). Send me an invitation to my email, and I will put it on my calendar.
hardcodes
That's why I call this a declarative build system that does not depend on external CLI flags and environment variables. For sanitizers, it is hard to decide in advance (that's why I automated it for tests as an example). But for things like using Conan, this decision can be definitely made and declared in the build.
Side note rant:
I cannot tell how many times I could not use a C++ library because of how broken their build script has been. A gazillion number of flags that I need to configure so I can use a library. This is in my opinion one of the biggest flaws of the current ecosystem. The C++ code even does not find a chance to run. People already give up on compiling. Either they switch to dynamic languages that do not require building or go to declarative style languages like cargo rust.
gives the user a button in the cmake-guis that does not do anything.
That's an issue we need to fix. I overlooked the GUI aspect of the GlobalOptions.cmake file. I think if that is the case we can remove this file, and just let the user define the dynamic options they want to change on the fly. It seems the options file creates more confusion than the things it provides.
* It's unfortunate that it uses a global setting, but it is helpful for making sure all tools stay consistent
* I think this is best option. This should likely be moved into project_options if we agree * Definitely saves time in the CI if we are building only 1 target of a multi-config generator
|
Ok. I need to add support for Visual Studio 2022 to setup-cpp. The code is not able to find it by itself. |
It can wait for a future PR. I just thought it would be an easy thing to add here |
|
It seems Visual Studio 2022 requires passing |
Odd, it was officially released > 2 months ago. |
The reason is that they have not updated vswhere! 🤷♂️ I need to work on this a bit. For now, I suggest removing |
@lefticus / @aminya : I am seeing something different.
|
|
Windows 2022 support was added in aminya/setup-cpp#25 I still recommend testing for only one version of Visual Studio (preferably the latest). |
|
Are there still unanswered questions? I agree some of the utility I have put in CMakeLists.txt needs to be moved into a utility function. (project_options?) should we do that pre or post merge? |
We can do it in another PR. |
This has a few minor updates to apply what I think should be the default for best practices getting started C++ code:
Plus I tweaked a few of the clang-tidy warnings that are enabled to make them easier to work with and disable warnings that are google project specific.
To do list:
ENABLE_DEVELOPER_MODE=ONbuilds fail because the environment does not contain clang-tidy, so it fails at configure time