11:43 a.m.
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Bug ID: 1421245
Summary: Review Request: libcrush - C library to control
placement in a hierarchy
Product: Fedora
Version: rawhide
Component: Package Review
Severity: medium
Priority: medium
Assignee: nobody(a)fedoraproject.org
Reporter: loic(a)dachary.org
QA Contact: extras-qa(a)fedoraproject.org
CC: package-review(a)lists.fedoraproject.org
Spec URL:
https://build.opensuse.org/package/view_file/home:ldachary/libcrush/libcr...
SRPM URL:
http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/li...
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...
suggets I mention this request here). I am the upstream maintainer.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
11:57 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Loic Dachary <loic(a)dachary.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |kdreyer(a)redhat.com
--- Comment #1 from Loic Dachary <loic(a)dachary.org> ---
Note that because of my work helping with the packaging of Ceph,
kdreyer(a)redhat.com sponsored me directly, under another identity
(ldachary(a)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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
11:58 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Loic Dachary <loic(a)dachary.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks| |177841 (FE-NEEDSPONSOR)
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
9:46 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Kees de Jong <keesdejong+bugs(a)gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |keesdejong+bugs(a)gmail.com
--- Comment #2 from Kees de Jong <keesdejong+bugs(a)gmail.com> ---
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_ove...
[3] https://fedoraproject.org/wiki/How_to_create_an_RPM_package#.25files_basics
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
11:53 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #3 from Loic Dachary <loic(a)dachary.org> ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
12:34 p.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #4 from Loic Dachary <loic(a)dachary.org> ---
Rebuilding in Koji at
https://koji.fedoraproject.org/koji/taskinfo?taskID=17861439
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
5:03 p.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Michael Schwendt <bugs.michael(a)gmx.net> changed:
What |Removed |Added
----------------------------------------------------------------------------
Blocks|177841 (FE-NEEDSPONSOR) |
--- Comment #5 from Michael Schwendt <bugs.michael(a)gmx.net> ---
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
Referenced Bugs:
https://bugzilla.redhat.com/show_bug.cgi?id=177841
[Bug 177841] Tracker: Review requests from new Fedora packagers who need a
sponsor
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
5:06 p.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #6 from Michael Schwendt <bugs.michael(a)gmx.net> ---
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
6:59 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #7 from Loic Dachary <loic(a)dachary.org> ---
Spec URL:
https://build.opensuse.org/package/view_file/home:ldachary/libcrush/libcr...
SRPM URL:
http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/li...
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:29 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #8 from Loic Dachary <loic(a)dachary.org> ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
9:18 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #9 from Loic Dachary <loic(a)dachary.org> ---
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}.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
11:57 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #10 from Loic Dachary <loic(a)dachary.org> ---
Spec URL:
https://build.opensuse.org/package/view_file/home:ldachary/libcrush/libcr...
SRPM URL:
http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/li...
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
4:49 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #11 from Loic Dachary <loic(a)dachary.org> ---
SRPM URL:
http://download.opensuse.org/repositories/home:/ldachary/Fedora_25/src/li...
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
5:49 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #12 from Loic Dachary <loic(a)dachary.org> ---
Spec URL: http://dachary.org/loic/libcrush.spec
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
8:07 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Haïkel Guémar <karlthered(a)gmail.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|NEW |ASSIGNED
CC| |karlthered(a)gmail.com
Assignee|nobody(a)fedoraproject.org |karlthered(a)gmail.com
Flags| |fedora-review?
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
10 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #13 from Loic Dachary <loic(a)dachary.org> ---
SRPM URL : http://dachary.org/loic/fedora/libcrush-1.0.0-1.fc25.src.rpm
Spec URL : http://dachary.org/loic/fedora/libcrush.spec
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
10:09 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #14 from Loic Dachary <loic(a)dachary.org> ---
Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18547753
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
10:26 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #15 from Loic Dachary <loic(a)dachary.org> ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
10:49 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #16 from Loic Dachary <loic(a)dachary.org> ---
Koji build : https://koji.fedoraproject.org/koji/taskinfo?taskID=18548547
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
4:28 p.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #17 from Haïkel Guémar <karlthered(a)gmail.com> ---
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?
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
5:52 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #18 from Loic Dachary <loic(a)dachary.org> ---
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+.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:29 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #19 from Loic Dachary <loic(a)dachary.org> ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:30 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #20 from Loic Dachary <loic(a)dachary.org> ---
3. gcc-c++ and make are included in buildroot, so no need to include
them.
Removed:
BuildRequires: gcc-c++
BuildRequires: make
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:32 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #21 from Loic Dachary <loic(a)dachary.org> ---
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.*
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:40 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #22 from Loic Dachary <loic(a)dachary.org> ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:46 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #23 from Loic Dachary <loic(a)dachary.org> ---
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 ?
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
7:53 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #24 from Loic Dachary <loic(a)dachary.org> ---
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
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
2:13 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #25 from Loic Dachary <loic(a)dachary.org> ---
@karlthered(a)gmail.com I forgot to thank you for your review. Thank you :-)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
8:03 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Loic Dachary <ldachary(a)redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
CC| |ldachary(a)redhat.com
Flags| |needinfo?(karlthered@gmail.
| |com)
--- Comment #26 from Loic Dachary <ldachary(a)redhat.com> ---
@Haïkel I would very much appreciate your opinion on the above. I stand ready
to go in the right direction :-)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
3:12 p.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
--- Comment #27 from Loic Dachary <ldachary(a)redhat.com> ---
@Haïkel do you think you'll get time to provide a feedback on this package ?
I'm eager to get it moving ;-)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
3:12 a.m.
New subject: [Bug 1421245] Review Request:
libcrush - C library to control placement in a hierarchy
https://bugzilla.redhat.com/show_bug.cgi?id=1421245
Loic Dachary <ldachary(a)redhat.com> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |CLOSED
Resolution|--- |EOL
Last Closed| |2017-07-09 04:12:02
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
2510
days inactive
2659
days old
package-review@lists.fedoraproject.org
30 comments
1 participants
participants (1)
-
bugzilla＠redhat.com