[Bug 19] Review request: blcr - Berkeley Lab Checkpoint/Restart for Linux

[RPM Fusion Bugzilla]

http://bugzilla.rpmfusion.org/show_bug.cgi?id=19





--- Comment #13 from Neal Becker <ndbecker2 at gmail.com>  2008-12-18 16:45:45 ---
(In reply to comment #10)
> This package surely needs some work. To start with:
> 
> * mock build fails on my x86_64. This is because you are trying to build and
> include 32 bit libraries in a 64 bit package, which is not allowed. If one
> needs 32 bit libraries (s)he can install blcr-libs.i386 in addition to
> blcr_libs.x86_64 . So you should remove the "libdir32" bits from the SPEC file.

Fixed

> * Leave a comment in the SPEC file for why you are using ExclusiveArch.

Done

> 
> * Try to avoid mixed ${ } %{_ } notation

Do you mean:

chrpath -d ${RPM_BUILD_ROOT}/%{_bindir}/cr_checkpoint
Suggested alternative?

> 
> * BR: "perl" and "sed" are not required since they are in the minimum build
> environment.

Done

> 
> * Please remove the static library bits from the SPEC file.

I assume you mean to unconditionally build static libs for devel.
Done.

> 
> * rpmlint complains:
>    blcr-devel.x86_64: W: no-documentation
>    blcr-testsuite.x86_64: W: no-documentation
>    blcr-testsuite.x86_64: E: script-without-shebang
> /usr/libexec/blcr-testsuite/shellinit
> For the first two, at least put the license file(s) in those packages.
> The last one is actually about an empty files. Well it is not empty but when
> you open it, it says "#empty". Do you think we should include that file?
> 

Fixed

> * Patches should be explained and be submitted to upstream if they are not
> strictly Fedora specific.

Done.

> 
> * The file tests/CountingApp.class is binary and should be removed during %prep

It isn't installed, do we care?

> 
> * The file README.devel is not and should be packaged.

Why?

> 
> * Buildroot should be one of these:
>    %(mktemp -ud %{_tmppath}/%{name}-%{version}-%{release}-XXXXXX)
>    %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)
>    %{_tmppath}/%{name}-%{version}-%{release}-root
> 

Done.


> * Why do you have:
>    # Ensure we don't build for a i386
>    %ifarch i386
>      set +x
>      echo
> "=========================================================================="
>      echo "ERROR: Cannot build BLCR for a generic i386." >&2
>      echo "ERROR: Add \"--target `uname -p`\" (or similar) to the rpmbuild
> command line." >&2
>      echo
> "=========================================================================="
>      exit 1
>    %endif
> in the SPEC file? Just remove i386 from ExclusiveArch and you should be fine.

Fixed.

> 
> * Please use
>   %post libs -p /sbin/ldconfig
>   %postun libs -p /sbin/ldconfig
> Afaik, they'll work more efficient.

Done.

> 
> * We prefer %defattr(-,root,root,-)
> 

Done.

> * Each package must consistently use macros, as described in the macros section
> of Fedora Packaging Guidelines . Avoid inconsistencies such as:
>    %clean
>    rm -rf ${RPM_BUILD_ROOT}
> 
>    %install
>    rm -rf $RPM_BUILD_ROOT

Done.

> * Disttag is missing.

What is this?

> 
> * The Fedora-specific compilation flag -fstack-protector is not passed to the
> compiler. For a list of flags that should be passed to the compiler, please do
> a
>    rpm --eval %optflags
> 

I believe all flags are passed, because %configure is used.  I just tested it,
and I believe -fstack-protector is passed.

> * Parallel make must be supported whenever possible. If it is not supported,
> this should be noted in the SPEC file as a comment.

Seems to break on this package, comment added.

> 
> * Shall we package the examples, tests directories?
> 

I think it's good to have the testsuite.


-- 
Configure bugmail: http://bugzilla.rpmfusion.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.