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

Added tests for ClickHouse apps help and fixed help issues #45819

Merged
merged 33 commits into from
Apr 18, 2023

Conversation

qoega
Copy link
Member

@qoega qoega commented Jan 31, 2023

Changelog category (leave one):

  • Improvement

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

Many issues in ClickHouse applications's help were fixed. Help is now written to stdout from all tools. Status code for clickhouse help invocation is now 0. Updated help for clickhouse-local, clickhouse-benchmark, clickhouse-client, clickhouse hash, clickhouse su, clickhouse-install.

Improved logging for clickhouse-install and improved support for installations into prefix. clickhouse stop, clickhouse start, clickhouse restart should work correctly with --prefix and in no-sudo environment

Documentation entry for user-facing changes

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

@robot-ch-test-poll2 robot-ch-test-poll2 added pr-improvement Pull request with some product improvements submodule changed At least one submodule changed in this PR. labels Jan 31, 2023
# We have to use fixed terminal width. It may break other tests results formatting.

backup_stty_size=$(stty size | awk '{print $2}')
stty columns 120
Copy link
Member

Choose a reason for hiding this comment

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

Also, there is no tty in CI,
so it's better to set it to the same value used for a fallback when there is no tty.

@alexey-milovidov alexey-milovidov self-assigned this Jan 31, 2023
@alexey-milovidov
Copy link
Member

Now it should be easier to modify Poco.

@alexey-milovidov
Copy link
Member

@qoega should we continue?

@qoega
Copy link
Member Author

qoega commented Feb 16, 2023

I will push changes after PTO. Some apps printed argv[0] with complete path to binary.

@robot-ch-test-poll2 robot-ch-test-poll2 removed the submodule changed At least one submodule changed in this PR. label Feb 23, 2023
@robot-ch-test-poll2 robot-ch-test-poll2 removed the submodule changed At least one submodule changed in this PR. label Mar 8, 2023
@qoega
Copy link
Member Author

qoega commented Apr 14, 2023

Test found another real issue with small terminal width

clickhouse-git-import --help
clickhouse-git-import: /home/ubuntu/clickhouse/contrib/boost/libs/program_options/src/options_description.cpp:316: boost::program_options::options_description::options_description(const std::string &, unsigned int, unsigned int): Assertion `m_min_description_length < m_line_length - 1' failed.
Aborted (core dumped)
(lldb) bt
* thread #1, name = 'clickhouse-git-', stop reason = hit program assert
    frame #0: 0x00007ffff7dda03b libc.so.6`raise + 203
    frame #1: 0x00007ffff7db9859 libc.so.6`abort + 299
    frame #2: 0x00007ffff7db9729 libc.so.6`___lldb_unnamed_symbol2384 + 15
    frame #3: 0x00007ffff7dcb006 libc.so.6`__assert_fail + 70
  * frame #4: 0x0000000031bc480a clickhouse-git-import`boost::program_options::(anonymous namespace)::format_description(os=0x0000000036e613b8, desc="produce help message", first_column_width=47, line_length=0) at options_description.cpp:569:13
    frame #5: 0x0000000031bc4787 clickhouse-git-import`boost::program_options::(anonymous namespace)::format_one(os=0x0000000036e613b8, opt=0x00007ffff700a960, first_column_width=47, line_length=0) at options_description.cpp:636:17
    frame #6: 0x0000000031bc406a clickhouse-git-import`boost::program_options::options_description::print(this=0x00007fffffffe128, os=0x0000000036e613b8, width=47) const at options_description.cpp:688:13
    frame #7: 0x0000000031bc3f5f clickhouse-git-import`boost::program_options::operator<<(os=0x0000000036e613b8, desc=0x00007fffffffe128) at options_description.cpp:419:14
    frame #8: 0x000000002249d9b5 clickhouse-git-import`mainEntryClickHouseGitImport(argc=2, argv=0x00007ffff7001580) at git-import.cpp:1222:13
    frame #9: 0x0000000019699aaa clickhouse-git-import`main(argc_=2, argv_=0x00007fffffffe358) at main.cpp:507:12
    frame #10: 0x00007ffff7dbb0b3 libc.so.6`__libc_start_main + 243
    frame #11: 0x000000001969946e clickhouse-git-import`_start + 46

Comment on lines 44 to 47
- src: root/usr/bin/clickhouse-static-files-disk-uploader
dst: /usr/bin/clickhouse-static-files-disk-uploader
- src: root/usr/bin/clickhouse-su
dst: /usr/bin/clickhouse-su
Copy link
Member Author

@qoega qoega Apr 14, 2023

Choose a reason for hiding this comment

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

@Felixoid can you review this change?

Copy link
Member

@Felixoid Felixoid Apr 14, 2023

Choose a reason for hiding this comment

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

Seems like the wrong package to me. All symlinks to clickhouse rather belong to the clickhouse-server package

Copy link
Member

Choose a reason for hiding this comment

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

I see the clickhouse-extract-from-config is here. Not sure why, but it looks more common than su. What about clickhouse-static-files-disk-uploader? Does it belong to client too?

Copy link
Member Author

Choose a reason for hiding this comment

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

No idea 😃 I found another small tool here and added here as well. I'm ok to move almost all the symlinks to server package. You can know the logic behind our packages. I just found those missing.

Copy link
Member

Choose a reason for hiding this comment

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

I can only guess regarding the clickhouse-static-files-disk-uploader, if it's useful in both client and server packages. The same regarding clickhouse-extract-from-config. But the clickhouse-su clearly belongs to the server package

@qoega
Copy link
Member Author

qoega commented Apr 14, 2023

Failures are unrelated

Only left questions are about symlinks from @Felixoid. When he will have time to look and say what changes are needed.

@Felixoid
Copy link
Member

The clickhouse-su belongs to the clickhouse-server package. It's needed there, not in the common package.

I can't say anything regarding clickhouse-static-files-disk-uploader. clickhouse-extract-from-config looks useful for both client and server, so let it stay in common

@qoega qoega merged commit ebb1b99 into ClickHouse:master Apr 18, 2023
136 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-improvement Pull request with some product improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants