Bug 886300

doesn't build in mock :
+ CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4  -m64 -mtune=generic -fPIC'
+ make -j4 perl
make[1]: Entering directory `/builddir/build/BUILD/sino-3.1.21/src'
make[1]: warning: jobserver unavailable: using -j1.  Add `+' to parent make rule.
make[2]: Entering directory `/builddir/build/BUILD/sino-3.1.21/src'
make[2]: Nothing to be done for `cc'.
make[2]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/src'
make[2]: Entering directory `/builddir/build/BUILD/sino-3.1.21/libperl'
`./pcc` -c -I `perl -MConfig -e 'print $Config{archlib}'`/CORE sinoapi_wrap.c
sinoapi_wrap.c:702:20: fatal error: EXTERN.h: No such file or directory
compilation terminated.
make[2]: *** [sinoapi_wrap.o] Error 1
make[2]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/libperl'
make[1]: *** [perl] Error 2
make[1]: Leaving directory `/builddir/build/BUILD/sino-3.1.21/src'
make: *** [perl] Error 2
erreur : Mauvais statut de sortie pour /var/tmp/rpm-tmp.Hbmfwl (%build)
    Mauvais statut de sortie pour /var/tmp/rpm-tmp.Hbmfwl (%build)

Ooops, good catch, thanks.
Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-2.fc16.src.rpm

I'm taking this review.

Hi François, thanks for packaging!  Here are some comments:

* Note that: Explicit dependency on perl-devel is not allowed (consider
  Module::Build).  For more info [1].

* There should be also BR MODULE_COMPAT_* dependency [1]:
    perl(:MODULE_COMPAT_%(eval "`%{__perl} -V:version`"; echo $version))

* Weird thing is, that the sino does not require libsino, is it statically
  linked.  Is that necessary?

* Explicit library dependency shouldn't be needed [2].  If yes, the %{_isa}
  should be given & comment added.

* No subpackage -> subpackage dependancy is given (except the lib one).  Is that
  correct?

* I'm not sure about:
    %post & %postun -n perl-sinoAPI -p /sbin/ldconfig

  I'm not a perl guru but as this subpackage does not install libraries
  into ldconfig path, and thus it should not be needed?

* imo we don't need %posttrans * scriptlets also

* Your hacks for so library may look ugly, would it be possible to be
  solved upstream?  Just a note now for this, the 'mv -f lib/libsino.so
  lib/libsino.so.3' command seems to redundant there.

* License seems to be s/GPLv3/GPLv3+/ and license of malloc.c seems to be not
  GPLv3+ (btw. this file seems to be unused, but it is quite good - seems to be
  very similar to what tcsh has (tc.alloc.c) and it causes weird kernel
  problems #443643).

* Manual page for sinodisp is not found.  I could be considered like small
  problem, but I can't find any documentation on this!  No --help, no manual
  page, also 'rpm -qd sino-cgi' is silent.  No documentation found.

Nits:

* I would comment the patches a little.  Downstream/upstream, etc.
* consider adding -p to install command (preserve timestamps)
* IIRC, defattr is not needed even in EPEL5, version in RHEL5 is 4.4.2.3.

Thanks for working on this,
Pavel

[1] https://fedoraproject.org/wiki/Packaging:Perl
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Hi Pavel,

Thank you very much for taking this review!

* dependency on perl-devel
   => fixed.

* MODULE_COMPAT_* dependency
   => fixed.

* sino/sinomake are statically linked
   => this is how upstream builds. I plan to switch these binaries to dynamic linking at some point.

* Explicit library dependency shouldn't be needed [2].  If yes, the %{_isa}
  should be given & comment added.
   => I am not sure of the line where this should be changed :/

* No subpackage -> subpackage dependancy is given (except the lib one).  Is that
  correct?
   => I think so.

* %post, %postun, %posttrans -n perl-sinoAPI -p /sbin/ldconfig
   => removed.

* Your hacks for so library may look ugly, would it be possible to be
  solved upstream?
   => I wish, but I am pretty sure upstream needs to make sure nothing is broken on other UNIX like Solaris, and I do not have a Solaris test box. The alternative is to ship the static library and I would rather not do that... (and yes, the hack is ugly).

* 'mv -f lib/libsino.so lib/libsino.so.3' redundant
   => I needed to move it out of the way, I switched to 'rm -f'.

* license of malloc.c seems to be not GPLv3+ and this file seems to be unused
   => Good catch, added rm -f at the end of prep.

* Manual page for sinodisp is not found (...) I can't find any documentation on this!
   => Sorry, I should have fixed it earlier. The documentation is at the beginning of sinodisp.c, so I now extract that and create a sinodisp.txt file which is installed by the sino-cgi package as documentation. I moved the sinodisp binary to /var/www/cgi-bin because that's where it really belongs.

Nits:
* I would comment the patches a little.  Downstream/upstream, etc.
   => for now nothing's been upstreamed, but I noted what could, and could not be upstreamed, in the new spec.
* consider adding -p to install command (preserve timestamps)
   => done.
* IIRC, defattr is not needed even in EPEL5, version in RHEL5 is 4.4.2.3.
   => removed. thanks.

Thank you!

Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-3.fc18.src.rpm

Hi François,  thanks for the progress!

====
for the previous comments:

> * sino/sinomake are statically linked
>    => this is how upstream builds. I plan to switch these binaries to
> dynamic linking at some point.

I would suggest you to do it now, I'm not sure how big deal it is but packaging
guidelines covers this quite simply:

  Cite Fedora's Packaging:Guidelines [1]:

  -> Static linkage is a special exception and should be decided on a
     case-by-case basis.  The packager must provide rationale for linking
     statically, including precedences where available, to FESCO for approval.

  -> Programs which don't need to notify FESCo
     If a library you depend on only provides a static version your package
     can link against it provided that you BuildRequire the *-static
     subpackage.  Packagers in such a situation should be aware that if a
     shared library becomes available, that you should adjust your package to
     use the shared library.

I think that the most easy one will be to link it dynamically (not create the
*-static package).

> * Explicit library dependency shouldn't be needed [2].  If yes, the %{_isa}
>   should be given & comment added.
>    => I am not sure of the line where this should be changed :/

58 | %package -n libsino-devel
59 | Group:          Development/Libraries
60 | Summary:        Files for development using libsino
61 | Requires:       lib%{name} = %{version}-%{release}
                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
It is probably good to be there .. but it should be documented as mentioned in
guidelines [2].  And the %{_isa} should be added.

> * %post, %postun, %posttrans -n perl-sinoAPI -p /sbin/ldconfig
>    => removed.

Yet the %posttrans is probably unnecessary.

> * 'mv -f lib/libsino.so lib/libsino.so.3' redundant
>    => I needed to move it out of the way, I switched to 'rm -f'.

Sorry, I made bad statement here.  I thought the command was completely
redundant, as the following 'mv' may overwrite the target (by adding -f).  This
is nit - not a problem.

> * license of malloc.c seems to be not GPLv3+ and this file seems to be unused
>    => Good catch, added rm -f at the end of prep.

I'm not sure here if this is correct.  It looks little bit dirty because
malloc.c is in distribution.  I would agree with this step if upstream is OK to
remove it in future release, but otherwise the malloc.c is distributed (for
some compilation time purposes) and I would do the 's/GPLv3/GPLv3+ and BSD/'.
Note the '+' there!

> * Manual page for sinodisp is not found (...) I can't find any documentation
> on this!
>    => Sorry, I should have fixed it earlier. The documentation is at the
> beginning of sinodisp.c, so I now extract that and create a sinodisp.txt
> file which is installed by the sino-cgi package as documentation. I moved
> the sinodisp binary to /var/www/cgi-bin because that's where it really
> belongs.

Thanks for this!  I'm not sure if the 'head -24' auto-scripting is ok for
future releases (maintenance hell) but it is not problem for me.

====
Some new comments:

* The installation of images:
  %{_datadir}/%{name}-%{version}/images
  %{_datadir}/%{name}-%{version}/images/*

  $ rpm -qf /usr/share/sino-3.1.21
  file /usr/share/sino-3.1.21 is not owned by any package

  Consider adding '%dir %{_datadir}/%{name}-%{version}/'.

* would it be possible to split the very long lines?

[1] https://fedoraproject.org/wiki/Packaging:Guidelines
[2] https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Hello Pavel!

Thanks for your comments. :)

* Requires: %{name}%{?_isa} = %{version}-%{release}
  => fixed.

* ldconfig in posttrans
  => I think it is needed, see https://www.redhat.com/archives/rhl-devel-list/2007-June/msg02497.html for instance.

* sinodisp had no documentation
  => hehe, no problem. :)

* malloc.c
  => I am pretty sure it is not needed after looking at the sources. I am going to ask upstream to remove it from future releases.

* long lines
  => split. 

* /usr/share/sino-3.1.21 not owned by any package
  => ooops, my bad. fixed.

* explicit Requires:
  => added %{_isa} and a note. 

TODO:
* sino / sinomake / sinoshow / sinodisp are statically linked
  => this requires building the shared library first, or relinking afterwards... Hmmm. Let me think about it, but I agree, I need to fix that.

In the meantime:
Spec URL: http://fcami.fedorapeople.org/srpms/sino.spec
SRPM URL: http://fcami.fedorapeople.org/srpms/sino-3.1.21-4.fc18.src.rpm

Thanks again!

> --- Comment #7 from François Cami <fdc> ---
>
> * ldconfig in posttrans
>   => I think it is needed, see
> https://www.redhat.com/archives/rhl-devel-list/2007-June/msg02497.html for
> instance.

This thread was about removing ldconfig from %post/%postun, iirc.  The
problem was (and actually is) that each package installing some shared
library is calling ldconfig based on package guidelines.

Calling ldconfig in %posttrans seems to be redundant and unnecessarily time
consuming.

> * malloc.c
>   => I am pretty sure it is not needed after looking at the sources. I am going
> to ask upstream to remove it from future releases.

Thanks!  Some link here would be nice, in spec respectively.

> TODO:
> * sino / sinomake / sinoshow / sinodisp are statically linked
>   => this requires building the shared library first, or relinking
> afterwards... Hmmm. Let me think about it, but I agree, I need to fix that.

As you probably agree that this is not according to our Fedora's PG, it is the
MUST now, sorry for that.

Thanks for your work, otherwise it seems to be OK to me now.
Pavel

Hello Pavel,

Re shared library and must: yes, I completely agree with you. Please give me a few days for this.

Thanks a lot.
François

Ping, François, are you still interested in packaging sino?

Hi Pavel,

Sorry, this has slipped through my radar.
I'm currently not doing any packaging at the moment so I'll close this one.
Thanks for your advice so far!