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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix enable LLVM for JIT compilation in CMake #35683

Merged
merged 2 commits into from
Mar 29, 2022

Conversation

kitaisreal
Copy link
Collaborator

@kitaisreal kitaisreal commented Mar 28, 2022

Changelog category (leave one):

  • Bug Fix (user-visible misbehaviour in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Enable build with JIT compilation by default.

@kitaisreal kitaisreal added the pr-must-backport Pull request should be backported intentionally. Use this label with great care! label Mar 28, 2022
@@ -2,11 +2,12 @@
# tools to be build for host architecture and everything else for target architecture (e.g. AArch64)
# Possible workaround is to use llvm-tblgen from some package...
# But lets just enable LLVM for native builds
if (CMAKE_CROSSCOMPILING OR SANITIZE STREQUAL "undefined")
if (NOT (${CMAKE_SYSTEM_NAME} MATCHES ${CMAKE_HOST_SYSTEM_NAME}) OR SANITIZE STREQUAL "undefined")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

CMAKE_CROSSCOMPILING can be true even without cross compilation

This variable will be set to true by CMake if the [CMAKE_SYSTEM_NAME]
(https://cmake.org/cmake/help/latest/variable/CMAKE_SYSTEM_NAME.html#variable:CMAKE_SYSTEM_NAME) 
variable has been set manually (i.e. in a toolchain file or as a cache entry from the [cmake]

https://cmake.org/cmake/help/latest/variable/CMAKE_CROSSCOMPILING.html

@kitaisreal
Copy link
Collaborator Author

kitaisreal commented Mar 28, 2022

One of latest builds on master:
https://s3.amazonaws.com/clickhouse-builds/22.4/2db8d354c6f1b46968619acd0ae8f7f80cfbdb53/package_release/build_log.log

Mar 28 13:25:31 -- Adding contrib module msgpack-c (configuring with msgpack-c-cmake)
Mar 28 13:25:31 -- Adding contrib module cityhash102 (configuring with cityhash102)
Mar 28 13:25:31 -- Adding contrib module libfarmhash (configuring with libfarmhash)
Mar 28 13:25:31 -- Adding contrib module icu (configuring with icu-cmake)
Mar 28 13:25:31 -- Adding contrib module h3 (configuring with h3-cmake)
Mar 28 13:25:31 -- Adding contrib module mariadb-connector-c (configuring with mariadb-connector-c-cmake)
Mar 28 13:25:31 -- Adding contrib module llvm (configuring with llvm-cmake)
Mar 28 13:25:31 -- Not using LLVM
Mar 28 13:25:31 -- Adding contrib module libxml2 (configuring with libxml2-cmake)

@kitaisreal
Copy link
Collaborator Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2022

update

✅ Branch has been successfully updated

@kitaisreal
Copy link
Collaborator Author

@Felixoid our previous wrong fix affects only v22.3.2.2-lts v22.3.1.1262-prestable.
I will backport it to 22.3, will you please make release ?

@kitaisreal kitaisreal added bug Confirmed user-visible misbehaviour in official release and removed bug Confirmed user-visible misbehaviour in official release labels Mar 28, 2022
@nikitamikhaylov nikitamikhaylov self-assigned this Mar 28, 2022
@Felixoid
Copy link
Member

Yep, we can release a new version

@kitaisreal
Copy link
Collaborator Author

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Mar 28, 2022

update

✅ Branch has been successfully updated

@kitaisreal kitaisreal merged commit c566c52 into ClickHouse:master Mar 29, 2022
@tavplubix
Copy link
Member

@kitaisreal you forgot to take a look at failed tests before merging again:
Stateless tests (release, DatabaseReplicated, actions) - 02152_http_external_tables_memory_tracking
Stateless tests (release, actions) - 02152_http_external_tables_memory_tracking
Stateless tests (release, s3 storage, actions) - 02152_http_external_tables_memory_tracking
Stateless tests (release, wide parts enabled, actions) - 02152_http_external_tables_memory_tracking

This test was not flaky for last 3 weeks: http://a137ed393136a448a8a0b796e91304c4-2000240599.us-east-1.elb.amazonaws.com:8123/play?user=default#c2VsZWN0IAp0b1N0YXJ0T2ZEYXkoY2hlY2tfc3RhcnRfdGltZSkgYXMgZCwKY291bnQoKSwgZ3JvdXBVbmlxQXJyYXkocHVsbF9yZXF1ZXN0X251bWJlcikKZnJvbSBgZ2gtZGF0YWAuY2hlY2tzIHdoZXJlICcyMDIyLTAxLTAxJyA8PSBjaGVja19zdGFydF90aW1lIGFuZCB0ZXN0X25hbWU9JzAyMTUyX2h0dHBfZXh0ZXJuYWxfdGFibGVzX21lbW9yeV90cmFja2luZycgYW5kIHRlc3Rfc3RhdHVzIGluICgnRkFJTCcsICdGTEFLWScpIGdyb3VwIGJ5IGQgb3JkZXIgYnkgZA==

So seems like it was broken here somehow. I will try to revert.

@kitaisreal
Copy link
Collaborator Author

@tavplubix I take a look this change just revert other pull request that breaks LLVM compilation, everything must work as before.

@azat
Copy link
Collaborator

azat commented Mar 30, 2022

Stateless tests (release, actions) - 02152_http_external_tables_memory_tracking

The problem was in settings randomization, in particular max_block_size=67990 for 02152_http_external_tables_memory_tracking, fixed in #35745

@Felixoid Felixoid added the pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore label Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-backports-created Backport PRs are successfully created, it won't be processed by CI script anymore pr-must-backport Pull request should be backported intentionally. Use this label with great care!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants