Fix macOS build #40938

Lucky-Chang · 2022-09-02T12:08:12Z

Changelog category (leave one):

Build/Testing/Packaging Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

fix build on macOS

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CLAassistant · 2022-09-02T12:08:18Z

All committers have signed the CLA.

Lucky-Chang · 2022-09-02T12:17:48Z

#40937 (comment)

serxa

LGTM, let's look if CI agree with all typo fixes in logs and error messages

serxa · 2022-09-02T14:32:19Z

src/Storages/StorageReplicatedMergeTree.cpp

@@ -7217,7 +7217,7 @@ bool StorageReplicatedMergeTree::addOpsToDropAllPartsInPartition(
 }

 void StorageReplicatedMergeTree::dropAllPartsInPartitions(
-    zkutil::ZooKeeper & zookeeper, const Strings partition_ids, std::vector<LogEntryPtr> & entries, ContextPtr query_context, bool detach)
+    zkutil::ZooKeeper & zookeeper, const Strings & partition_ids, std::vector<LogEntryPtr> & entries, ContextPtr query_context, bool detach)


rschu1ze · 2022-09-03T16:57:56Z

utils/self-extracting-executable/post_build.sh

@@ -1,6 +1,10 @@
 padding="               "
 if [[ $OSTYPE == 'darwin'* ]]; then
-    sz="$(stat -f %z 'decompressor')"
+    if which gstat; then


The GNU and the Mac version stat support different command line arguments (-c %s vs -f %z). On Mac, we are now checking for gstat aka. the GNU version which is part of brew's coreutils package. Since installing gstat via brew does not remove or disable the build-in Mac version of stat, I wonder how this PR "fixes the macOS build" (commit message)?

as you say, i installed gnu coreutils by homebrew in my macOS. and gstat is the default alternative for stat

Instead of making the script more complex, we could continue to call stat even if the user installed gstat.

the problem is directly call stat will fail, because it's actually 'gstat', the running params are different

novikd · 2022-09-03T23:52:02Z

src/Loggers/CMakeLists.txt

 target_link_libraries(loggers PRIVATE dbms clickhouse_common_io)

 # Lightweight version doesn't work with textlog and also doesn't depend on DBMS
 add_library(loggers_no_text_log ${loggers_sources} ${loggers_headers})
+target_compile_definitions(loggers_no_text_log PUBLIC WITHOUT_TEXT_LOG=1)


@rschu1ze AFAIU this line is supposed to fix the build, but to be honest I don't understand why. For me it seems to have absolutely the same behaviour as before because #ifdef WITH_TEXT_LOG was replaced with ifndef WITHOUT_TEXT_LOG.

i figure that the main binary include 'Loggers.h' in different way, in most case, the include path is implicated by loggers library. But some directly include the 'Loggers.h'. so the WITH_TEXT_LOG definition differ in the main binary.
I just reverse WITH_TEXT_LOG to WITHOUT_TEXT_LOG. so in the main library all 'Loggers.h' has no WITHOUT_TEXT_LOG. it works out.

The fix is a bit weird but generally okay, IMHO.

It would be great to understand the problem in more detail.

@Lucky-Chang Could you please give the detailed steps for reproduction: What is the exact command line for cmake? Please try to use homebrew's LLVM/Clang/CMake as described in the documentation instead of Apple's XCode LLVM/Clang (as used in #40398).

i figure that the main binary include 'Loggers.h' in different way, in most case, the include path is implicated by loggers library. But some directly include the 'Loggers.h'. so the WITH_TEXT_LOG definition differ in the main binary.

By "main binary", do you mean programs/server/Server.cpp?

What do you mean "the loggers library implicates the include path"? More specifically, which files get included exactly and by magic exactly does that happen?

Could you give an example for the files which "directly include Loggers.h"?

I think I have a clue about the cause of the problem. Sorry for asking all these questions but depending on the details, there could be potentially a better (more consistent) fix.

It took me quite some time to understand how it fix the code. The simple answer Loggers class definition violates ODR (introduced in #35031).
Explanaition:

Loggers class is build in 2 binaries: loggers and loggers_no_text_log.

loggers_no_text_log is used only for clickhouse-keeper.

Everything else is linked against loggers, but compiled without WITH_TEXT_LOG macro.

For example, in BaseDeamon.h we have #include <Loggers/Loggers.h>. BaseDeamon.cpp is never compiled with WITH_TEXT_LOG macro defined, but linked with loggers.

Lucky-Chang · 2022-09-04T04:54:54Z

exactly the same problem with #40398

novikd · 2022-09-04T11:31:58Z

src/Loggers/Loggers.h

@@ -7,7 +7,7 @@
 #include <Poco/Util/Application.h>
 #include "OwnSplitChannel.h"

-#ifdef WITH_TEXT_LOG
+#ifndef WITHOUT_TEXT_LOG


This now leads Loggers to be expected with text log in clickhouse-keeper, but linked with loggers_no_text_log. The problem is not fixed.

Please, make a separate PR with typo fixes, because it's hard to read this PR and to understand what is the actual fix.

src/CMakeList.txt 319~320
dbms_target_include_directories (PUBLIC "${ClickHouse_SOURCE_DIR}/src" "${ClickHouse_BINARY_DIR}/src")
target_include_directories (clickhouse_common_io PUBLIC "${ClickHouse_SOURCE_DIR}/src" "${ClickHouse_BINARY_DIR}/src")

this two lines expose all .h （include Loggers.h）to all build libraries (common-io and all dmbs)。during all these libraries building they just not see 'WITH_TEXT_LOG' compile definition。HOW we could make sure some library will
inherit the compile definition from the loggers (or loggers_no_text_log) library? it must be explicit link loggers (modern cmake target link rules), in the project there only two libraries do this:
target_link_libraries (daemon PUBLIC loggers)
target_link_libraries(clickhouse-keeper PRIVATE loggers_no_text_log)

At last clickhouse-server links daemon library (with WITH_TEXT_LOG) and other comon_io / dbms libraries (without WITH_TEXT_LOG), the Loggers class definition conflicts.

Above is my explanation. as for the mini-clickhouse-keeper without text log, its link dependencies have no change, will keep small. But may still have problem, becase clickhouse-keeper links loggers_no_text_log and clickhouse_common_io, the compile definition (WITH_TEXT_LOG) in loggers_no_text_log not exposed to clickhouse_common_io.
target_link_libraries(clickhouse-keeper
PRIVATE
...
loggers_no_text_log
clickhouse_common_io
)

At last，it seems no easy way to fix this problem gracefully now, just the whole compile target dependency is not struct in modern CMake way https://gist.github.com/mbinna/c61dbb39bca0e4fb7d1f73b0d66a4fd1

i make a separate PR for typo fix here, #40984

novikd · 2022-09-19T15:22:15Z

Problem is fixed in #41060

robot-ch-test-poll added the pr-build Pull request with build/testing/packaging improvement label Sep 2, 2022

Lucky-Chang mentioned this pull request Sep 2, 2022

Build Failed on macOS 12.5.1 llvm13 #40937

Closed

Lucky-Chang changed the title ~~fix macOS compile and some typos~~ Fix macOS build and some typos Sep 2, 2022

serxa self-assigned this Sep 2, 2022

serxa added the can be tested Allows running workflows for external contributors label Sep 2, 2022

serxa approved these changes Sep 2, 2022

View reviewed changes

novikd mentioned this pull request Sep 3, 2022

Segmentation fault on my mac. #40398

Closed

rschu1ze reviewed Sep 3, 2022

View reviewed changes

novikd reviewed Sep 3, 2022

View reviewed changes

novikd requested changes Sep 4, 2022

View reviewed changes

serxa assigned novikd and unassigned serxa Sep 5, 2022

fix macOS build

db3b109

Lucky-Chang force-pushed the master branch from 34fd506 to db3b109 Compare September 6, 2022 02:44

Lucky-Chang changed the title ~~Fix macOS build and some typos~~ Fix macOS build Sep 6, 2022

novikd mentioned this pull request Sep 6, 2022

Try to fix ODR for Loggers class #41060

Merged

novikd closed this Sep 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix macOS build #40938

Fix macOS build #40938

Lucky-Chang commented Sep 2, 2022

CLAassistant commented Sep 2, 2022 •

edited

Loading

Lucky-Chang commented Sep 2, 2022

serxa left a comment

serxa Sep 2, 2022

rschu1ze Sep 3, 2022 •

edited

Loading

Lucky-Chang Sep 4, 2022

rschu1ze Sep 4, 2022

Lucky-Chang Sep 4, 2022

novikd Sep 3, 2022

Lucky-Chang Sep 4, 2022

rschu1ze Sep 4, 2022 •

edited

Loading

novikd Sep 4, 2022

Lucky-Chang commented Sep 4, 2022

novikd Sep 4, 2022

Lucky-Chang Sep 4, 2022

Lucky-Chang Sep 5, 2022

novikd commented Sep 19, 2022

Fix macOS build #40938

Fix macOS build #40938

Conversation

Lucky-Chang commented Sep 2, 2022

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

CLAassistant commented Sep 2, 2022 • edited Loading

Lucky-Chang commented Sep 2, 2022

serxa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschu1ze Sep 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rschu1ze Sep 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lucky-Chang commented Sep 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novikd commented Sep 19, 2022

CLAassistant commented Sep 2, 2022 •

edited

Loading

rschu1ze Sep 3, 2022 •

edited

Loading

rschu1ze Sep 4, 2022 •

edited

Loading