Bug#858733: RFS: clickhouse/1.1.54190-1 [ITP] (original) (raw)

Hello, Thanks for having a look at it. I uploaded a new version of the package on mentors

https://mentors.debian.net/package/clickhouse

The respective dsc file can be found at: https://mentors.debian.net/debian/pool/main/c/clickhouse/clickhouse_1.1.54284-1.dsc

My answer inline

On 28/07/2017 12:35, Andrey Rahmatullin wrote:

Control: tags -1 + moreinfo

Some preliminary remarks:

Why B-D specifically on gcc-6, g++-6? Because clickhouse didn't built with gcc-5. Recent versions now build with gcc-7 so I removed the B-D

Version in Depends: lsb-base (>= 3.2-14) can be dropped. Done

Current Standards-Version is 4.0.0. Current debian/compat is 10. Done

Are you sure this Pre-Depends usage is correct? Have you discussed it as required by the Policy? I suppose it's because clickhouse-client.postinst uses the user created in clickhouse-common.postinst but I don't think it is needed in this case. Not discussed it. I guess I took it from the package I used as an example (or even the upstream Debian work). Will see that

Besides, I think you shouldn't use chown to change ownership, see https://www.debian.org/doc/debian-policy/ch-files.html#s-permissions-owners Will see that

Besides, I don't think it's advisable to have a dir in /etc owned by a service user (while the group will remain root? why?). For now, I just mimic the upstream behaviour. Since clickhouse generate a preprocessed version of the xml config file, the user must have write access to /etc Is there any good practices for such cases ?

The current consensus is to not delete users on purge. Done

AUTHORS and README.md are included in several subpackages, this seems wrong. And in debian/clickhouse-utils.docs they are listed twice. Done

There seems to be a typo in the PID file name in debian/clickhouse-server.service Done

Some of the descriptions miss the trailing period. Done

Is debian/copy_clang_binaries.sh unused? Upstream inheritage. Removed

debian/libclickhouse1.symbols is empty. Yeah, I still have issues to manage it properly. Removed for now.

debian/copyright is very wrong with a single copyright and license for the entire project: it lacks licenses and copyrights for all bundled third-party code, there are several MIT-licensed files in the main code and docs, and that's without doing a full analysis. Fixed. I did not include embedded libs which are removed during build: in the future, I intend to remove them before merging new upstream version using repack

Now to debian/rules. Why --max-parallel=4? override_dh_auto_clean runs dh_clean, not dh_auto_clean, and it also does a bunch of rm's while dh_clean can do that with debian/clean. I think removing embedded copies should be done in clean, not in build. dh_auto_configure should run cmake, not override_dh_auto_build. Manual make -jgrep -c ^processor /proc/cpuinfo is wrong. "will wait for the ability to use system libs instead of contrib ones" comment is worrying, so did that already happen or not? copy_headers.sh seems wrong. dh_install --sourcedir=debian/tmp is the default, isn't it? In any case it shouldn't be run in override_dh_auto_install. Creating dirs should be done by dh_installdirs. Made some updates to d/rules Not sure it's perfect, but it still better than it was

And the build fails in sbuild:

./debian/tests_wrapper.sh /<> grep: ../../debian/clickhouse-server-config-tests-preprocessed.xml: No such file or directory grep: /etc/clickhouse-server/config-preprocessed.xml: No such file or directory

Running stateless tests.

00001_select_1: [ FAIL ] - return code 232 /usr/share/zoneinfo/Asia/Yekaterinburg: No such file or directory Poco::Exception: Exception: Cannot load time zone Asia/Yekaterinburg [...] 00039_inserts_through_http: [ FAIL ] - return code 7 curl: (7) Failed to connect to localhost port 8123: Connection refused [...] Having 58 errors! ./debian/tests_wrapper.sh: line 30: 31404 Aborted (core dumped) PATH=$PATH:./build/dbms/src/Server ./build/dbms/src/Server/clickhouse-server --config-file=./debian/clickhouse-server-config-tests.xml 2> /dev/null (wd: /<>) ./debian/tests_wrapper.sh: line 18: kill: (31404) - No such process debian/rules:31: recipe for target 'override_dh_auto_test' failed Done

Attachment:signature.asc
Description: OpenPGP digital signature

Reply to: