Re: [PATCH 0/3] Implement hello <-> libhello pattern

[Baurzhan Ismagulov <ibr@radix50.net>]

On Thu, Jan 25, 2018 at 11:38:48AM +0300, Alexander Smirnov wrote:
> This series intended to add Isar build dependencies between recipes.

Some comments on the hello / libhello themselves.


> [PATCH 1/2] Rename binary package to avoid correlations with Debian hello.
> [PATCH 2/2] Add dependency from 'libhello'.

Please remove the trailing dot from the subjects.


> diff --git a/debian/changelog b/debian/changelog
...
> +hello (0.2) unstable; urgency=low

I'd suggest to tag:

v0.1 7f35942 Add autotools
v0.2 ffca497 Add dependency from 'libhello'

Otherwise, Acked-by: Baurzhan Ismagulov <ibr@ilbers.de>.


> [PATCH] libhello: First implementation

> diff --git a/Makefile.am b/Makefile.am
...
> +DB2MAN = docbook-to-man

I think that should be in Build-Depends: (the same applies to hello).


> +man1_MANS = hello.1

I suggest say_hello.3.


> +clean-local:
> +	-$(RM) -f $(man1_MANS)

I suggest removing the leading "-" (the same applies to hello; not sure why I
used that at the time).


> diff --git a/debian/.control.swp b/debian/.control.swp

Please remove.


> diff --git a/debian/changelog b/debian/changelog
...
> +libhello (0.1) unstable; urgency=low

Suggest to tag v0.1 1671f02 libhello: First implementation.


> diff --git a/debian/control b/debian/control
...
> +Build-Depends: debhelper (>= 9), autotools-dev

Strictly speaking, autotools-dev is not needed. However, in that case the build
may fail due to timestamp mess after a git checkout (if configure.ac happens to
be newer than configure, the latter would try to rebuild itself).

Depending on autotools-dev workarounds that for the same build suite, but not
for a different one (configure generated on jessie would rebuild fine on
jessie, but possibly not on other version, since exactly the same autotools
versions are required).

That led to the following hack:

https://github.com/ilbers/isar/blob/2d56fc1242ef3e3041eb248942324eb67e9b1319/meta/recipes-devtools/buildchroot/files/build.sh#L17

I suggest:

* Dropping the autotools-dev build-dependency.

* Adding AM_MAINTAINER_MODE([disable]) to configure.ac to avoid rebuilding
  configure, Makefile.in, etc. For development, one may enable it with
  ./configure --enable-maintainer-mode.

* Dropping the mentioned touch hack.

That would apply also to hello.

That is documented in:

https://www.gnu.org/software/automake/manual/html_node/CVS.html#CVS
https://www.gnu.org/software/automake/manual/html_node/maintainer_002dmode.html#maintainer_002dmode


> diff --git a/debian/copyright b/debian/copyright
...
> +Copyright: 2016 ilbers GmbH

2018?


> diff --git a/debian/menu b/debian/menu

I don't think we need a GUI menu entry for libhello. Suggest dropping the file.


> diff --git a/hello.h b/hello.h
...
> +#ifndef __HELLO_H__

According to C99, identifiers that begin with an underscore are reserved
(typically for compiler or OS use). Suggest HELLO_H.


> diff --git a/hello.sgml b/hello.sgml
...
> +    <refentrytitle>HELLO</refentrytitle>
> +    <manvolnum>1</manvolnum>
...
> +    <refname>hello</refname>
...
> +    <title>EXIT STATUS</title>
> +
> +    <para><application>libhello</application> always returns zero.</para>

Suggest providing info applicable for say_hello(3).


As we have a backlog of patches and the issues are minor, feel free to create
github bugs for issues that would take longer and mention them as # TODO:
https://github.com/ilbers/isar/issues/NN in the code.


With kind regards,
Baurzhan.