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 build errors when linking OpenSSL dynamically #62888
Fix build errors when linking OpenSSL dynamically #62888
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This is an automated comment for commit 2f0fb73 with description of existing statuses. It's updated for the latest CI running ❌ Click here to open a full report in a separate page
Successful checks
|
@@ -93,6 +93,7 @@ enable_language(ASM) | |||
|
|||
if(COMPILER_CLANG) | |||
add_definitions(-Wno-unused-command-line-argument) | |||
set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -fuse-ld=lld") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Setting a linker manually seems weird, tbh. This will potentially cause that different libraries of ClickHouse are linked by different linkers which is in the general case troublesome (as far as I know).
Including or not including l. 96 did not make a difference for me in local testing with static/dynamic link. I tested on x86 though, perhaps things are different on s390x.
I am not totally against explicitly specifying a linker but perhaps we can do so only for platforms where this makes a difference (s390x)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I observed higher versions of CMake fixed the issue(which is your case), but lower versions of CMake(my CMake version is 3.23.2 for example) has this issue. I suggest we keep the change for lower version CMake users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, okay.
9c27cb6
@rschu1ze @alexey-milovidov Can this pr be backported to 24.3LTS? This PR and #63135 are required to enable 24.3 in s390x |
It is not a production build, and dynamic linking is strictly prohibited in production. If you need it for your own purposes, you can backport on your own. |
Thanks @rschu1ze we are all good. |
(see #59870 (comment))
The build for OpenSSL dynamic linking(
mkdir build; cd build; cmake .. -DENABLE_OPENSSL_DYNAMIC=1
) is broken due to the following reasons:Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fixed build errors when OpenSSL is linked dynamically (note: this is generally unsupported and only required for s390x platforms).
Documentation entry for user-facing changes
Modify your CI run:
NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step
Include tests (required builds will be added automatically):
Exclude tests:
Extra options:
Only specified batches in multi-batch jobs: