clickhouse 21.7 (new formula) #79378

qoega · 2021-06-15T14:47:56Z

Have you followed the guidelines for contributing?
Have you ensured that your commits follow the commit style guide?
Have you checked that there aren't other open pull requests for the same formula update/change?
Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
Does your build pass brew audit --strict <formula> (after doing brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

qoega · 2021-06-15T15:05:01Z

Last try to add ClickHouse was in 2016 #7222
Closes: ClickHouse/ClickHouse#235

carlocab · 2021-06-15T16:26:29Z

Formula/clickhouse.rb

+class Clickhouse < Formula
+  desc "Free analytics DBMS for big data with SQL interface"
+  homepage "https://clickhouse.tech"
+  url "https://github.com/ClickHouse/ClickHouse/releases/download/v21.6.3.14-stable/ClickHouse_sources_with_submodules.tar.gz"


This seems to vendor a whole lot of dependencies. We should avoid that and use a Homebrew/core dependency whenever one is available.

See https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae.

Thank you for your fast review.
Actually we have very thorough CI process in ClickHouse and we test master and stable versions that occur as packages and binaries for our users. You can read more about CI here https://clickhouse.tech/docs/en/development/continuous-integration/
We do not use dynamic libraries and almost even minimize glibc dependences for maximum portability.
As ClickHouse is written in C++ there is no easy way for dependencies check and updates like in Java/etc. Our community and team updates libraries and this process usually needs.
If we start to support separate build process for brew it will be complex as lots of libraries do not release official updates(for example Poco) and we have to add bugfixes in forked repos. All changes are published in https://github.com/ClickHouse-Extras. Moreover we cannot test such releases with unbundled versions of libraries the same way as we test upstream and stable releases.
Separate build for brew means we provide our users not the same ClickHouse that is available in official builds. I understood https://docs.brew.sh/Acceptable-Formulae#we-dont-like-binary-formulae as it is better to build everything in your envionment and not just reuse build artifacts from our CI process. As you can see we do not use any system libraries and just need working compiler with c++17 support.

I see it is not an easy question and we should find solution that will benefits our communities.

From that same page: https://docs.brew.sh/Acceptable-Formulae#stuff-that-requires-vendored-versions-of-homebrew-formulae

You might have your own testing infrastructure for your dependencies, but I think your coverage is also a bit limited here.

For example, it's clear that you don't test building FlatBuffers on macOS Mojave since the Mojave build fails at building FlatBuffers. If you used Homebrew FlatBuffers instead, this would not be a problem.

Actually there is a lot of work done to make binaries which are portable. For example we check that you can use the exact same binary on centos:5/ubuntu:12.04 with really old glibc 2.4. We have cross-compile builds for aarch64, freebsd and macOS. Currently we do not have macOS servers to be able to run all necessary checks for that platform(and for different macOS versions).
We do have precompiled versions(https://clickhouse.tech/docs/en/getting-started/install/#from-binaries-non-linux) - it should be trivial to check that it works on Mojave. Probably we will get Apple servers in future and add them to CI.

FlatBuffers here is not a single problem - actually I'm almost sure it would successfully build if I add llvm dependency and build it without AppleClang on Mojave. When I encountered the same error on Catalina, I found that Xcode developer tools allow to complile project and CLT fails. Build environment should not be somehow related to functionality and we allow to use c++17 and c++20 features if it is justifiable. So, yes - we do not test building on old platforms, but support running ClickHouse on older operating systems.

Imagine we will get it somehow working. Rewrite all cmake files. Add 75 dependencies to libraries that are present in Homebrew or those that are not. Export all patches from 60+ libraries that we had to write to fix bugs in those libraries.

We can't control that all 75 libraries have mainatainers that update brew formula for them.

I can't understand how we can patch library source code and not only our sources. For example we have 85 commits in Poco, that was not yet taken to upstream(example TCPServerDispatcher.h: missing <atomic> pocoproject/poco#2961)

In normal way we have just make security fix to upstream(probably just checkout newer submodule version), test this build and switch version in brew to more recent release. Here I don't understand who exactly updates library. You force dynamic linkage or you want to rebuild all packages that depend on library in build stage?

We know several use cases when using other versions of dynamic libraries lead to problems in stability. If we do not know exactly what versions will be user by a user we cannot check this environment. And considering we have 75 submodules, it is almost unthinkable.

And to understand what is the reason of this policy. Is it just for security reason only or there are some other ideas?

Actually there is a lot of work done to make binaries which are portable.

Sure; I don't doubt that. That doesn't mean your testing coverage isn't finite.

FlatBuffers here is not a single problem - actually I'm almost sure it would successfully build if I add llvm dependency and build it without AppleClang on Mojave.

I've done a bit of work on using LLVM Clang in place of Apple Clang in order to build a formula on Mojave (see, e.g., grpc, mavsdk, bear, folly, etc), and I'm almost sure it would not build successfully just by using LLVM Clang in place of Apple Clang.

An unguarded aligned_alloc call just isn't something you can fix by using a newer compiler, because it likely involves making system calls that just aren't implemented. I'm happy to be proven wrong here, however, since this would allow me to fix workarounds I've put in place elsewhere.

Export all patches from 60+ libraries that we had to write to fix bugs in those libraries.

You should be upstreaming these patches anyway, regardless of whether you support building ClickHouse with system-provided libraries or not.

We can't control that all 75 libraries have mainatainers that update brew formula for them.

Sure. This is true about any software that uses an external dependency. I would argue that attempting to exert control over external dependencies isn't really something you should be trying to do anyway.

I can't understand how we can patch library source code and not only our sources. For example we have 85 commits in Poco, that was not yet taken to upstream(example pocoproject/poco#2961)

Why haven't these patches been upstreamed?

In normal way we have just make security fix to upstream(probably just checkout newer submodule version), test this build and switch version in brew to more recent release. Here I don't understand who exactly updates library. You force dynamic linkage or you want to rebuild all packages that depend on library in build stage?

We link libraries dynamically.

We know several use cases when using other versions of dynamic libraries lead to problems in stability. If we do not know exactly what versions will be user by a user we cannot check this environment. And considering we have 75 submodules, it is almost unthinkable.

What problems are these exactly?

And to understand what is the reason of this policy. Is it just for security reason only or there are some other ideas?

Security is one reason. De-duplication is another. It kinda defeats a big part of the use of a package manager if a package's build system includes its own specialised package manager for its vendored dependencies.

carlocab · 2021-06-15T16:26:48Z

Formula/clickhouse.rb

+  version "21.6"
+  sha256 "696bbcdef4796bfdf1becdb78abb5e97c0220acc9863f5a821193dd297ce428b"
+  license "Apache-2.0"
+  head "https://github.com/ClickHouse/ClickHouse.git"


Suggested change

head "https://github.com/ClickHouse/ClickHouse.git"

head "https://github.com/ClickHouse/ClickHouse.git"

Let's make the formatting here consistent with other formulae.

Got it. I'll add it after I'll get current build results.

carlocab · 2021-06-15T16:27:02Z

Formula/clickhouse.rb

+    mkdir_p bin
+    mkdir_p "#{HOMEBREW_PREFIX}/etc/clickhouse-server"
+    mkdir_p "#{var}/log/clickhouse-server"
+    mkdir_p "#{var}/lib/clickhouse-server"
+    mkdir_p "#{var}/run/clickhouse-server"


What happens when you don't do this?

Install procedure will not find a directory.
At least I had to fix creation of ones like Cellar/clickhouse/21.6/bin that has version number in them.
By the way, it was really exhausting to debug build failures in brew install --build-from-source -v clickhouse as it required to rebuild everything without cache. And it took more than 3 hours on each try. Is there a way to retry one step in noninteractive way?

carlocab · 2021-06-15T16:27:33Z

Formula/clickhouse.rb

+    args = std_cmake_args
+    args.delete("-DCMAKE_BUILD_TYPE=Release")
+    args << "-DCMAKE_BUILD_TYPE=RelWithDebInfo"
+
+    system "cmake", ".", *args


Suggested change

args = std_cmake_args

args.delete("-DCMAKE_BUILD_TYPE=Release")

args << "-DCMAKE_BUILD_TYPE=RelWithDebInfo"

system "cmake", ".", *args

system "cmake", ".", *std_cmake_args

We don't support building with debug symbols.

Haven't met this in Cookbook. I can change it.
Can you elaborate what is the idea? I don't know how I can help a user to report a bug without symbols and I don't know a way to provide separate package with symbols in brew. In Debian packages we can split artifacts to binary and symbols – do we have alternative there?

Not really, though I'm personally open to entertain the possibility of externalising debug information and packaging them separately. (I don't speak for other maintainers on this, though.)

At the moment, however, we don't support building with debug info at all. Indeed, you'll find that most attempts to build with debug info will either break the build, or just not produce any debug info (since brew will intercept any -g flags passed to the compiler).

carlocab · 2021-06-15T16:27:56Z

Formula/clickhouse.rb

+
+  plist_options manual: "clickhouse-server --config-file #{HOMEBREW_PREFIX}/etc/clickhouse-server/config.xml"
+
+  def plist


Can we use a service block instead of manually creating a plist?

I guess sure we can. But I can't find any documentation about this and probably try to catch the idea from #79366

qoega · 2021-06-15T18:03:34Z

Concerning 10.14 build – it is not possible to fix this check as newer c++ standard is required. I was able to find ones in Xcode developer tools for 10.15(not in CLT) and it is probably available in 11 CLT, but I have no Big Sur environment to test it locally.
About relocatable check - is it a problem due to debug symbols or something similar? We definitely do not use clang inside ClickHouse, but probably store information about flags and some build environment.

==> Detecting if clickhouse--21.6.arm64_big_sur.bottle.tar.gz is relocatable...
73
/opt/homebrew/Cellar/clickhouse/21.6/bin/clickhouse
74
 --> match '/opt/homebrew/Library/Homebrew/shims/mac/super/clang' at offset 0x77f2e86
75
 --> match '/opt/homebrew/Library/Homebrew/shims/mac/super/clang++' at offset 0x77f2eeb
76
Warning: String '/opt/homebrew/Library' still exists in these files:
77
Error: Bottle contains non-relocatable reference to /opt/homebrew/Library!

Probably I understood incorrectly where I should place binary? It should be placed in Keg /usr/local/Cellar/clickhouse/21.6/bin? I took it from here https://docs.brew.sh/Formula-Cookbook#variables-for-directory-locations. Probably I need to find more information about Homebrew shims – where is the correct place to read?

Files were found with references to the Homebrew shims directory.
48
    The offending files are:
49
      bin/clickhouse

carlocab · 2021-06-17T11:27:33Z

Concerning 10.14 build – it is not possible to fix this check as newer c++ standard is required. I was able to find ones in Xcode developer tools for 10.15(not in CLT) and it is probably available in 11 CLT, but I have no Big Sur environment to test it locally.

This comes from the unguarded use of aliged_alloc, which is not portable. Depending on Homebrew FlatBuffers instead of your vendored one should fix this particular build failure, I think.

github-actions · 2021-07-09T00:07:43Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

BrewTestBot added the new formula PR adds a new formula to Homebrew/homebrew-core label Jun 15, 2021

qoega mentioned this pull request Jun 15, 2021

An official OS X Homebrew formula/tap or Docker image ClickHouse/ClickHouse#235

Closed

clickhouse 21.6 (new formula)

c26a8be

qoega force-pushed the clickhouse-tap branch from 8ff0358 to c26a8be Compare June 15, 2021 15:04

carlocab reviewed Jun 15, 2021

View reviewed changes

github-actions bot added the stale No recent activity label Jul 9, 2021

github-actions bot closed this Jul 16, 2021

qoega changed the title ~~clickhouse 21.6 (new formula)~~ clickhouse 21.7 (new formula) Jul 23, 2021

github-actions bot added the outdated PR was locked due to age label Aug 23, 2021

github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clickhouse 21.7 (new formula) #79378

clickhouse 21.7 (new formula) #79378

qoega commented Jun 15, 2021

qoega commented Jun 15, 2021

carlocab Jun 15, 2021

qoega Jun 15, 2021

SMillerDev Jun 16, 2021

carlocab Jun 17, 2021

qoega Jun 17, 2021

carlocab Jun 17, 2021 •

edited

Loading

carlocab Jun 15, 2021

qoega Jun 15, 2021

carlocab Jun 15, 2021

qoega Jun 15, 2021

carlocab Jun 15, 2021

qoega Jun 15, 2021

carlocab Jun 17, 2021

carlocab Jun 15, 2021

qoega Jun 15, 2021

qoega commented Jun 15, 2021

carlocab commented Jun 17, 2021

github-actions bot commented Jul 9, 2021

	head "https://github.com/ClickHouse/ClickHouse.git"
	head "https://github.com/ClickHouse/ClickHouse.git"


		plist_options manual: "clickhouse-server --config-file #{HOMEBREW_PREFIX}/etc/clickhouse-server/config.xml"

		def plist

clickhouse 21.7 (new formula) #79378

clickhouse 21.7 (new formula) #79378

Conversation

qoega commented Jun 15, 2021

qoega commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlocab Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qoega commented Jun 15, 2021

carlocab commented Jun 17, 2021

github-actions bot commented Jul 9, 2021

carlocab Jun 17, 2021 •

edited

Loading