Bug 1421245

Note that because of my work helping with the packaging of Ceph, kdreyer sponsored me directly, under another identity (ldachary). libcrush is intended to be primarily community oriented, reason why I created another account under my personal email. This also is the first package I'm responsible for and it seems fair that I start from scratch. I would be happy if he agrees to sponsor me in this context but since he already is very busy, I would not want to presume he is available.

I'm trying to gain some reputation to get my own package sponsored, so I will give it a go ahead to review yours. If you want you could give my much simpler package a review in return [1], if you don't see any improvements, then please leave a comment that you approve the package.

According to the Fedora package documentation, the BuildRoot should be left untouched [2].

Another remark, the use of %defattr is no longer necessary unless the permissions need to be altered [3].

Your changelog is empty, you could bump it with `rpmdev-bumpspec -c 'this is my first package'`.

The release tag should be like '1%{?dist}' [2]. The group tag is deprecated [2].

[1] https://bugzilla.redhat.com/show_bug.cgi?id=1411984
[2] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#SPEC_file_overview
[3] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics

Thanks for your review !

> According to the Fedora package documentation, the BuildRoot should be left untouched [2].

Removed.

> The group tag is deprecated [2].

Removed.

> The release tag should be like '1%{?dist}' [2].

Changed.

> Another remark, the use of %defattr is no longer necessary unless the permissions need to be altered [3].

Removed.

I'll try to contribute to your own package. Although you know more than I do and maybe there is nothing for me to criticize ;-)

Cheers

Rebuilding in Koji at https://koji.fedoraproject.org/koji/taskinfo?taskID=17861439

Since you're sponsored already despite the packaging mistakes, there's no need to block the FE-NEEDSPONSOR tracker.


SRPM URL -> 404 not found


> %define libname %{name}1
> Name:           libcrush

Please don't do that. It's a confusing mess. You're blocking the libcrush namespace for dist git, the src.rpm and the spec file, too. You also miss automatic arch-specific Provides. And worse, your virtual package naming is half-hearted as you build libcrush-devel and libcrush1. If there ever will be a separate "libcrush1" package, create it when necessary, and make that one provide a libcrush1-devel package, too.


> %package devel
> Summary:        C library to control placement in a hierarchy development files
> Requires:       %{libname} = %{version}

https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

​
> %description

Descriptions of both packages are the same. You should be explicit about the -devel package only containing files needed for development.


> # FIXME: you should use %%cmake macros
> cmake . \

Indeed. The better comment would explain why %cmake isn't used or whether it cannot be used [yet].


> %{_datadir}/pkgconfig/libcrush.pc

Wrong installation directory for arch-specific .pc files.
Wrong libdir definition for 64-bit targets.
Is relinking with libm necessary?
Adding /usr/include/crush to headers search path bears risk, since that directory includes headers with very generic names. Preferably, API users and the library headers themselves would include all libcrush headers via <crush/foo.h> from standard search path.


> %changelog

Even for the initial package, there ought to be a first entry.
https://fedoraproject.org/wiki/Packaging:Guidelines#Changelogs

https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

Spec URL: https://build.opensuse.org/package/view_file/home:ldachary/libcrush/libcrush.spec
SRPM URL: http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/libcrush-1.0.0-13.1.src.rpm

> SRPM URL -> 404 not found

Will add updated URLs to the next comment.

> %define libname %{name}1

Removed and package renamed from libcrush1 to libcrush

> Requires:       %{libname} = %{version}

Chanted to https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Requires:       %{name}%{?_isa} = %{version}-%{release}

> Descriptions of both packages are the same.

Updated the devel packages description:

%description devel
This package contains libraries and headers needed to develop programs
that use the libcrush library.

> # FIXME: you should use %%cmake macros

Changed to:

%cmake .

> Is relinking with libm necessary?

Yes.

> Wrong installation directory for arch-specific .pc files.
> Wrong libdir definition for 64-bit targets.

Both changed to use %{_libexecdir} 
https://fedoraproject.org/wiki/Packaging:Guidelines#Libexecdir

> Adding /usr/include/crush to headers search path bears risk, since that directory includes headers with very generic names.

Using pkgconfig --cflags is not mandatory, it is a convenience.

> %changelog

Added.

> Wrong libdir definition for 64-bit targets.

I don't understand, the spec file contains no %{_libdir} definition, how can it be wrong ? The zlib package also installs in %{_libdir}.

Spec URL: https://build.opensuse.org/package/view_file/home:ldachary/libcrush/libcrush.spec
SRPM URL: http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/libcrush-1.0.0-23.1.src.rpm

It has the changes mentionned above except two:

* %cmake is not used and the .spec explains why
* the libdir definition for 64-bit targets is unmodified because I believe it is correct

SRPM URL: http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/libcrush-1.0.0-23.2.src.rpm

Spec URL: http://dachary.org/loic/libcrush.spec

SRPM URL : http://dachary.org/loic/fedora/libcrush-1.0.0-1.fc25.src.rpm
Spec URL : http://dachary.org/loic/fedora/libcrush.spec

Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18547753

There is a rpmlint warning that I don't quite understand:

sh-4.3# rpmlint /root/rpmbuild/RPMS/x86_64/libcrush-devel-1.0.0-1.fc25.x86_64.rpm
libcrush-devel.x86_64: W: only-non-binary-in-usr-lib
1 packages and 0 specfiles checked; 0 errors, 1 warnings.
sh-4.3# rpmlint -I only-non-binary-in-usr-lib
only-non-binary-in-usr-lib:
There are only non binary files in /usr/lib so they should be in /usr/share.
sh-4.3# rpm -qpl /root/rpmbuild/RPMS/x86_64/libcrush-devel-1.0.0-1.fc25.x86_64.rpm 
/usr/include/crush
/usr/include/crush/acconfig.h
/usr/include/crush/builder.h
/usr/include/crush/crush.h
/usr/include/crush/crush_compat.h
/usr/include/crush/crush_ln_table.h
/usr/include/crush/hash.h
/usr/include/crush/int_types.h
/usr/include/crush/mapper.h
/usr/include/crush/types.h
/usr/lib64/libcrush.so
/usr/lib64/pkgconfig/libcrush.pc
/usr/share/man/man3/libcrush.3.gz
sh-4.3# cp /root/rpmbuild/SRPMS/libcrush-1.0.0-1.fc25.src.rpm /home/loic/software/libcrush/fedora/ ; cp /root/rpmbuild/SPECS/\
libcrush.spec /home/loic/software/libcrush/fedora/
sh-4.3# 

I must be missing something because it looks like there are no files in /usr/lib and the /usr/lib64/libcrush.so file  in /usr/lib64 is binary.

Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18548547

1. licensing seems to be LGPLv2.1 according README so short identifier should LGPLv2 (or GPLv3 if it's an alternative provided by upstream)
2. license file is not included in package
3. gcc-c++ and make are included in buildroot, so no need to include them.
4. for your rpmlint error, likely a false positive
https://github.com/rpm-software-management/rpmlint/issues/73
5. include README.md using %doc in main package
6. I'm not sure to understand why not using %cmake macros, you can add missing parameters in the end. At least set build type to debug and shared libs parameters.
7. I suspect that googletest/googlemock bundled are only used in tests that are not shipped. Is it possible to build/run tests using system copies?

> 1. licensing seems to be LGPLv2.1 according README so short identifier should LGPLv2 (or GPLv3 if it's an alternative provided by upstream)

The README has a reminder that part of the files are LGPLv2.1. But the upstream distribution, as a whole, is indeed GPLv3+.

> 2. license file is not included in package

I added both GPLv3 and LGPLv2.1 as recommended at https://fedoraproject.org/wiki/Packaging:LicensingGuidelines

    %license LICENSE-GPL-3 LICENSE-LGPL-2.1

> 3. gcc-c++ and make are included in buildroot, so no need to include them.

Removed:

    BuildRequires:  gcc-c++
    BuildRequires:  make

> 5. include README.md using %doc in main package

I think I already did. But maybe it's misplaced and I'm not seeing things clearly ?

    %files -n libcrush
    %doc README.md
    %{_libdir}/libcrush.so.*

> 6. I'm not sure to understand why not using %cmake macros, you can add missing parameters in the end. At least set build type to debug and shared libs parameters.

I don't understand exactly what messes things up when using %cmake. The build type is None and includes debug information that is necessary to the debug packages. The shared libraries are built by default.

The documentation for %cmake is terse https://fedoraproject.org/wiki/Packaging:Cmake and when I tried to use it there were problems with files ending up in the wrong directories (/lib instead of /lib64, this kind of thing). After some trial and error I got to the point where I figured out a set of options that are sensible and do the right thing.

If this really is a problem I can dive back into this and find a combination of %cmake and additional options that achieve the desired result.

> 7. I suspect that googletest/googlemock bundled are only used in tests that are not shipped. Is it possible to build/run tests using system copies?

The googletest package include sources that need to be compiled and linked to the main test program, they are unfortunately not just header files. I could try to add a dependency to googletest, copy the sources in the build directory, compile them and throw the result away after the tests are run. However, since they are already in the source tarball and they are solely used to verify the library at compile time, it seems rather redundant.

What do you think ?

Koji : https://koji.fedoraproject.org/koji/taskinfo?taskID=18631659
SRPM URL : http://dachary.org/loic/fedora/libcrush-1.0.0-1.fc25.src.rpm
Spec URL : http://dachary.org/loic/fedora/libcrush.spec

@karlthered I forgot to thank you for your review. Thank you :-)

@Haïkel I would very much appreciate your opinion on the above. I stand ready to go in the right direction :-)

@Haïkel do you think you'll get time to provide a feedback on this package ? I'm eager to get it moving ;-)

The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days