Bug 1421245 - Review Request: libcrush - C library to control placement in a hierarchy [NEEDINFO]
Summary: Review Request: libcrush - C library to control placement in a hierarchy
Keywords:
Status: CLOSED EOL
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Haïkel Guémar
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-02-10 17:43 UTC by Loic Dachary
Modified: 2017-07-09 08:12 UTC (History)
6 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2017-07-09 08:12:02 UTC
Type: ---
karlthered: fedora-review?
ldachary: needinfo? (karlthered)


Attachments (Terms of Use)

Description Loic Dachary 2017-02-10 17:43:28 UTC
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-12.1.src.rpm
Description: libcrush maps an object signature to a list of devices on which to
store the object. It is implemented as a pseudo-random, deterministic
function obeying user specified placement constraints such as: the
object must be stored on devices that are on two different machines.

Fedora Account System Username: dachary

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

This is my first package and I need a sponsor (https://fedoraproject.org/wiki/Join_the_package_collection_maintainers?rd=PackageMaintainers/Join#Create_Your_Review_Request suggets I mention this request here). I am the upstream maintainer.

Comment 1 Loic Dachary 2017-02-10 17:57:31 UTC
Note that because of my work helping with the packaging of Ceph, kdreyer@redhat.com sponsored me directly, under another identity (ldachary@redhat.com). 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.

Comment 2 Kees de Jong 2017-02-13 15:46:21 UTC
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

Comment 3 Loic Dachary 2017-02-14 17:53:31 UTC
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

Comment 4 Loic Dachary 2017-02-14 18:34:50 UTC
Rebuilding in Koji at https://koji.fedoraproject.org/koji/taskinfo?taskID=17861439

Comment 5 Michael Schwendt 2017-02-20 23:03:36 UTC
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

Comment 8 Loic Dachary 2017-02-21 13:29:29 UTC
> 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.

Comment 9 Loic Dachary 2017-02-21 15:18:15 UTC
> 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}.

Comment 10 Loic Dachary 2017-02-21 17:57:59 UTC
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

Comment 12 Loic Dachary 2017-03-23 10:49:01 UTC
Spec URL: http://dachary.org/loic/libcrush.spec

Comment 14 Loic Dachary 2017-03-23 15:09:21 UTC
Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18547753

Comment 15 Loic Dachary 2017-03-23 15:26:42 UTC
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.

Comment 16 Loic Dachary 2017-03-23 15:49:14 UTC
Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18548547

Comment 17 Haïkel Guémar 2017-03-26 21:28:42 UTC
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?

Comment 18 Loic Dachary 2017-03-27 10:52:52 UTC
> 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+.

Comment 19 Loic Dachary 2017-03-27 12:29:30 UTC
> 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

Comment 20 Loic Dachary 2017-03-27 12:30:29 UTC
> 3. gcc-c++ and make are included in buildroot, so no need to include them.

Removed:

    BuildRequires:  gcc-c++
    BuildRequires:  make

Comment 21 Loic Dachary 2017-03-27 12:32:31 UTC
> 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.*

Comment 22 Loic Dachary 2017-03-27 12:40:48 UTC
> 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.

Comment 23 Loic Dachary 2017-03-27 12:46:41 UTC
> 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 ?

Comment 25 Loic Dachary 2017-04-05 07:13:56 UTC
@karlthered@gmail.com I forgot to thank you for your review. Thank you :-)

Comment 26 Loic Dachary 2017-04-12 13:03:18 UTC
@Haïkel I would very much appreciate your opinion on the above. I stand ready to go in the right direction :-)

Comment 27 Loic Dachary 2017-06-06 20:12:35 UTC
@Haïkel do you think you'll get time to provide a feedback on this package ? I'm eager to get it moving ;-)


Note You need to log in before you can comment on or make changes to this bug.