Skip to content

Fix build of mysqlxx_pool_test#82167

Merged
azat merged 2 commits intoClickHouse:masterfrom
azat:build/fix-mysqlxx_pool_test
Jun 19, 2025
Merged

Fix build of mysqlxx_pool_test#82167
azat merged 2 commits intoClickHouse:masterfrom
azat:build/fix-mysqlxx_pool_test

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Jun 19, 2025

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Previously due to dbms it requires clickhouse_functions_text, but it does not need dbms at all.

Follow-up: for #80641

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh bot commented Jun 19, 2025

Workflow [PR], commit [3e25692]

Summary:

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Jun 19, 2025
@azat azat enabled auto-merge June 19, 2025 12:06
@ahmadov
Copy link
Copy Markdown
Member

ahmadov commented Jun 19, 2025

I wonder why this executable was not build in CI when I opened a PR for my changes.

Previously only clickhouse-bundle was build for almost all builds,
except for tidy, but tidy uses dummy linker, that's why we cannot rely
on it.
@azat
Copy link
Copy Markdown
Member Author

azat commented Jun 19, 2025

I wonder why this executable was not build in CI when I opened a PR for my changes.

Indeed. Will be fixed here 3e25692

@@ -1,2 +1,2 @@
clickhouse_add_executable (mysqlxx_pool_test mysqlxx_pool_test.cpp)
target_link_libraries (mysqlxx_pool_test PRIVATE mysqlxx dbms clickhouse_common_config loggers_no_text_log)
target_link_libraries (mysqlxx_pool_test PRIVATE mysqlxx clickhouse_common_config loggers_no_text_log)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

SO finally dbms is not needed? Not even the 02443_detach_attach_partition?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

For this target - yes, not sure that I got part about 02443_detach_attach_partition

@azat azat added this pull request to the merge queue Jun 19, 2025
Merged via the queue into ClickHouse:master with commit 0fd44b2 Jun 19, 2025
121 checks passed
@azat azat deleted the build/fix-mysqlxx_pool_test branch June 19, 2025 15:52
@robot-ch-test-poll2 robot-ch-test-poll2 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 19, 2025
Comment on lines +170 to +171
elif build_type == BuildTypes.AMD_DEBUG:
targets = "-k0 all"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is completely ignored because amd_debug is within the BUILD_TYPE_TO_DEB_PACKAGE_TYPE dict.

We can check in the log of this same PR that it didn't work: https://s3.amazonaws.com/clickhouse-test-reports/PRs/82167/3e25692c37fbf9e4fdd922cee43c3b538cfc61d7//build_amd_debug/job.log

> Start execution for [Build ClickHouse]
Run command: [ninja clickhouse-bundle]

If we want AMD_DEBUG to build all targets, we should move the if statement prior to the if build_type in BUILD_TYPE_TO_DEB_PACKAGE_TYPE

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hi @pamarcos

I don't know is we want to build on amd, but in any case please open a new issue referencing this (or reopen this one, as you prefer)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we want AMD_DEBUG to build all targets, we should move the if statement prior to the if build_type in BUILD_TYPE_TO_DEB_PACKAGE_TYPE

Sigh, thanks for noticing, it looks error prone to check first this set and only after exceptions, anyway fix is here - #82723

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants