https://bugzilla.redhat.com/show_bug.cgi?id=1526768
Bug ID: 1526768 Summary: Review Request: clickhouse - a distributed column-oriented DBMS Product: Fedora Version: rawhide Component: Package Review Severity: medium Priority: medium Assignee: nobody@fedoraproject.org Reporter: roman@tsisyk.com QA Contact: extras-qa@fedoraproject.org CC: package-review@lists.fedoraproject.org
Spec URL: https://gist.githubusercontent.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/... SRPM URL: https://gist.github.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/raw/c575862... Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=23753966 Description: ClickHouse is a distributed column-oriented DBMS Homepage: https://clickhouse.yandex/ SCM: https://github.com/yandex/clickhouse Fedora Account System Username: rtsisyk
ClickHouse is an open source column-oriented database management system capable of real time generation of analytical data reports using SQL queries.
- Blazing Fast - Linearly Scalable - Hardware Efficient - Fault Tolerant - Feature Rich - Highly Reliable - Simple and Handy
ClickHouse is popular (3.3k likes on GitHub, 1.5k users in Telegram Chat) and used by multiple companies in production. I want to add add this software to Fedora Package Collection. I'm already in packager group and sponsorship is not required.
I already have access to `packager' group and don't need sponsorship.
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
--- Comment #1 from Roman Tsisyk roman@tsisyk.com --- Minor changes: - Fix paths in /etc/clickhouse-server/config.xml - Fix permissions for /etc/clickhouse-server/ - Get clickhouse-server to work out of the box
Spec URL: https://gist.githubusercontent.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/... SRPM URL: https://gist.github.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/raw/1780b8b...
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
Michael Cullen michael@cullen-online.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |michael@cullen-online.com
--- Comment #2 from Michael Cullen michael@cullen-online.com --- Ok... a few things I note:
* The BuildRequires are at the top - I'd expect the name block at the top. * This bundles cctz without any comments as to why * Have you tried unbundling boost? Although it looks for 1.60 the bundled version is 1.65 so I'd guess the version isn't actually that critical - a small patch to remove the version number would most likely be all that's required * The CMake file seems to support ARM builds - any reason to make this exclusivearch other than the docs claiming it is? * Groups aren't needed or particularly wanted [1]
[1] https://fedoraproject.org/wiki/RPMGroups
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
--- Comment #3 from Michael Cullen michael@cullen-online.com --- I tried a mock build of this in a rawhide container and got this error:
CMake Error at dbms/cmake_install.cmake:61 (file): file INSTALL cannot copy file
"/builddir/build/BUILD/ClickHouse-1.1.54326-testing/dbms/libclickhouse.so.1.1.54326" to
"/builddir/build/BUILDROOT/clickhouse-1.1.54326-1.fc28.x86_64/usr/lib64/clickhouse/libclickhouse.so.1.1.54326".
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
--- Comment #4 from Michael Cullen michael@cullen-online.com --- actually ignore me that's probably a disk space issue on my end!
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
--- Comment #5 from Roman Tsisyk roman@tsisyk.com --- Hi Michael,
Thanks for the fast response.
* The BuildRequires are at the top - I'd expect the name block at the top.
BuildRequires have been moved to a proper place.
Fixed.
* This bundles cctz without any comments as to why
According to Github [1], the latest released version of google/cctz is 2.1, but ClickHouse's scripts needs src/zone_info_source.cc which was added after 2.1 release. I added a comment about that to spec file. This package is very tricky in terms of dependencies, as you can see in the BuildRequires list.
[1] https://github.com/google/cctz/releases
* Have you tried unbundling boost? Although it looks for 1.60 the bundled version is 1.65 so I'd guess the version isn't actually that critical - a small patch to remove the version number would most likely be all that's required
Switched to boost-devel. Initially it didn't compile, but it seems that my combination of CMake flags has fixed this problem. Fixed.
* The CMake file seems to support ARM builds - any reason to make this exclusivearch other than the docs claiming it is?
Upstream claims that "Only x86_64 with SSE 4.2 is supported. Support for AArch64 is experimental." [2]. I tried to compile aarch64 on my test host and realized that some code under #ifdef..#else..#endif is completely broken. I fixed this problem and send a pull request to upstream [2], hope they will merge it soon. Using this fix I was able to reach 99% completeness in CMake. Unfortunately, linking of C++ code requires a lot of memory and fails with OOM on my coin-size aarch64 mini-PC.
[2] https://clickhouse.yandex/docs/en/development/build.html [3] https://github.com/yandex/ClickHouse/pull/1674
Plan:
- Wait until pull request is accepted or include this fix as a patch - Re-try on an aarch64 host with more RAM - Enable or disable aarch64 in ExclusiveArch depending on result
I don't think that aarch64 supports blocks review. I'll make the best efforts to fix aarch64 both in upstream and this package.
* Groups aren't needed or particularly wanted [1]
I removed `Group:` tag. Fixed.
Full changelog:
- Re-order BuildRequires - Add `sed` to BuildRequires - Add a comment about google/cctz - Remove `Group` - Use `boost-devel` instead of bundled boost - Enable experimental aarch64 support
Spec URL: https://gist.githubusercontent.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/... SRPM URL: https://gist.github.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/raw/f43fc9f...
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
Roman Tsisyk roman@tsisyk.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(michael@cullen-on | |line.com)
--- Comment #6 from Roman Tsisyk roman@tsisyk.com --- Updated to 1.1.54330
Spec URL: https://gist.github.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/raw/26e1b13... SRPM URL: https://gist.github.com/rtsisyk/1a45e69293f3e2c91126fe04a8374af8/raw/26e1b13... Koji URL: https://koji.fedoraproject.org/koji/taskinfo?taskID=24073176
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
Michael Cullen michael@cullen-online.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(michael@cullen-on | |line.com) |
--- Comment #7 from Michael Cullen michael@cullen-online.com --- Looking much better in terms of comments!
You might want to consider packaging cctz from that Git release (it doesn't seem to be in Fedora at all right now) but it isn't something I'm going to moan about too much.
Just one thing about the bundling though: the guidelines [0] require any bundled libraries to be called out in the spec file as provides - something like:
Provides: bundled(cctz) = 20171031.4f9776a
should do the trick.
[0] https://fedoraproject.org/wiki/Packaging:Guidelines#Bundling_and_Duplication... [1] https://fedoraproject.org/wiki/Packaging:Versioning#Snapshots
https://bugzilla.redhat.com/show_bug.cgi?id=1526768
Elliott Sales de Andrade quantum.analyst@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |quantum.analyst@gmail.com
--- Comment #8 from Elliott Sales de Andrade quantum.analyst@gmail.com --- cctz is now packaged for Fedora 28+ [1] if you want to depend on it.
[1] https://bugzilla.redhat.com/show_bug.cgi?id=1556661
package-review@lists.fedoraproject.org