Some build improvement#33695
Conversation
robot-clickhouse
added
the
pr-build
base/base/CMakeLists.txt
Outdated
There was a problem hiding this comment.
It takes me more than one hour to find the SHELL: functionality. The cmake language is almost esoteric as the Brainfuck language...
amosbird
force-pushed
the
buildimprove
branch
from
358b199 to
a3c1869
Compare
amosbird
force-pushed
the
buildimprove
branch
from
a3c1869 to
6d62060
Compare
|
FYI I also want to remove all CMake checks like try_compile from the libraries, because they make cross-compiling more difficult. |
| set (CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} ${INCLUDE_DEBUG_HELPERS}") | ||
| # CMake generator expression will do insane quoting when it encounters special character like quotes, spaces, etc. | ||
| # Prefixing "SHELL:" will force it to use the original text. | ||
| set (INCLUDE_DEBUG_HELPERS "SHELL:-I\"${ClickHouse_SOURCE_DIR}/base\" -I\"${MAGIC_ENUM_INCLUDE_DIR}\" -include \"${ClickHouse_SOURCE_DIR}/src/Core/iostream_debug_helpers.h\"") |
There was a problem hiding this comment.
It's interesting how many people are using debug helpers. I don't use them.
Most likely some people do... but it is quite obscure and difficult to find (although the implementation is very smart).
There was a problem hiding this comment.
It's quite handy. I see the debug helpers get turned on by default when build type is DEBUG, which indicates some people do)
| uint64_t doubleToUInt64(double d) | ||
| { | ||
| if (d >= std::numeric_limits<uint64_t>::max()) | ||
| if (d >= double(std::numeric_limits<uint64_t>::max())) |
There was a problem hiding this comment.
What motivated to make this change?
If we can enable some new warning or clang-tidy check, let's do that.
There was a problem hiding this comment.
The clang-tidy rule is cppcoreguidelines-narrowing-conversions. It might be too strict for us now.
alexey-milovidov
left a comment
alexey-milovidov
left a comment
There was a problem hiding this comment.
What tools did you use to find missing or unused headers?
|
Looks like this broke the build on head: https://github.com/ClickHouse/ClickHouse/runs/4861238785?check_suite_focus=true#step:6:10321 Seeing similar issues while trying to build on dev too. |
It would be perfect to remove those stuff, as the |
It's discovered by manually optimize some header files to |
4 participants
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Some improvement over current build system.
Detailed description / Documentation draft:
CMAKE_*_FLAGSas it will interfere withtry_compilerelated stuff, like check_type_size, check_functions...