[Bug 843997] Review Request: mlpack - scalable C++ machine learning library

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



https://bugzilla.redhat.com/show_bug.cgi?id=843997

--- Comment #2 from Michael Schwendt <mschwendt@xxxxxxxxx> ---
An honest attempt at trying to review your own package could be enlightening:

  https://fedoraproject.org/wiki/Packaging:ReviewGuidelines

If you're unsure about the possibly overwhelming list of MUST/SHOULD items on
that list, feel free to ask.

Just a few comments after a brief look at the spec file:


> Summary:        A scalable, fast C++ machine learning library

Not covered by any guidelines, but in many cases the leading article is
superfluous and doesn't increase conciseness.

  Summary: Scalable, fast C++ machine learning library


> Group:          Development/Libraries

"Group: System Environment/Libraries" for run-time libs.


> # More recent Armadillo versions require specific calls to arma::as_scalar().
> Patch3:         fully_qualified_as_scalar.patch

It's good that there are comments on these patch files, but even better would
be a comment on the upstream status. That's a "SHOULD" item, however.

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


> # Don't perform tests on sparse matrices.
> Patch4:         no_sparse_tests.patch

A redundant comment IMO. Better tell _why_ those tests get disabled. ;o)


> Requires:       armadillo >= 2.4.0
> Requires:       libxml2
> Requires:       boost, boost-program-options, boost-math
> Requires:       lapack

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

Which may read as if it is specific to library packages, but basically can be
understood as "there should be a spec file comment justifying" the explicit
dependency. For example, highlight whether the required packages are strictly
required or why they are needed.


> %package bin
> Summary:        Command-line executables for mlpack (machine learning library)
> Group:          Development/Libraries

Questionable group for executables.

> Requires:       %{name} = %{version}-%{release}

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


> %files
> %defattr(-,root,root,-)
> %{_libdir}/libmlpack.so

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



> %files bin
> %doc %{_mandir}/man1/allknn.1.gz

%_mandir implicitly is marked as documentation. Also, the gzip compression may
change or be disabled within a buildsystem, so using wildcards can be
beneficial.

  %{_mandir}/man1/allknn.1*


> %files devel
> %{_includedir}/mlpack/
> %{_includedir}/mlpack/core.hpp

The first entry already includes the directory *and* everything in it. The
second line is superfluous.
https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership

-- 
You are receiving this mail because:
You are on the CC list for the bug.
_______________________________________________
package-review mailing list
package-review@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/package-review



[Index of Archives]     [Fedora Legacy]     [Fedora Desktop]     [Fedora SELinux]     [Yosemite News]     [KDE Users]     [Fedora Tools]