Unformal quick review because NEEDSPONSOR:

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

-> install -m 755 build/libdlib.so.%{version}.* %{buildroot}%{_libdir}
isnt this managed by cmake? If not I would prefer to patch cmake instead of monkey-copying. If you will still use install you should keep timestamps by using '-p'.

-> %{python2_sitearch}/dlib/
-> %{python2_sitearch}/dlib-*.egg-info/
%{python2_sitearch}/%{name}*

Description for py2/py3 subpkgs not correlates with python stuff, please adjust. The same for -devel subpackage

I dont see that python subpackages requires main package (please check it) and if it requires - no need to duplicate license.

(In reply to Igor Gnatenko from comment #1)

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

Thnk you! Fixed.
> 
> -> install -m 755 build/libdlib.so.%{version}.* %{buildroot}%{_libdir}
> isnt this managed by cmake?

No.

> If you will still use install you should keep timestamps by
> using '-p'.

cmake does not preserve timestamps when installing files. Should we really take care of this when installing manually?

> Description for py2/py3 subpkgs not correlates with python stuff, please
> adjust. The same for -devel subpackage

Fixed.

> I dont see that python subpackages requires main package (please check it)
> and if it requires - no need to duplicate license.

They don't require it. Python modules are linked statically and don't use the shared library. Probably this is incorrect, but I couldn't find an easy way to fix this, dynamic linking would require heavy rewriting of build scripts.

New URLs:
Spec: http://copr-dist-git.fedorainfracloud.org/cgit/bizdelnick/neuro/dlib.git/plain/dlib.spec?id=350ddd8c6cc0eb57720e565e83ecf75b2cdd59ff
SRPM: https://copr-be.cloud.fedoraproject.org/results/bizdelnick/neuro/fedora-rawhide-x86_64/00145335-dlib/dlib-18.18-1.fc24.src.rpm

A tiny nitpick: "accompanied by ... thorough debugging modes" — something is wrong with that sentence.

I'll finish the review of this package, but you need to find a sponsor separately. Please do some (informal) reviews of other packages, etc.

In the %description, please say what kind of a software library it is, and what algorithms it offers (not an exhaustive list, but a few words of overview). The description can be more than one paragraph, and this way it will show up in searches, and potential users will be able to tell if this is useful for them much more easily.

- license is most OK (boost)
Change to: License: Boost and Public Domain

- license file is present, %license is used
Also add LICENSE_FOR_EXAMPLE_PROGRAMS.txt to %license.
- latest version
- requires/provides look OK, but see below
- builds and installs fine
- scriplets look sane
- devel is split out correctly, has a versioned and arched requires on main package

rpmlint:
dlib.x86_64: W: no-documentation
dlib-devel.x86_64: W: summary-not-capitalized C dlib development files
Please change to "Development files for dlib".

dlib-devel.x86_64: W: only-non-binary-in-usr-lib
OK.

dlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/dlib-devel/docs/vs-cmake-gui.png
dlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/dlib-devel/docs/vs_mode_3.png
dlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/dlib-devel/docs/vs_mode_1.png
dlib-devel.x86_64: W: spurious-executable-perm /usr/share/doc/dlib-devel/docs/vs_mode_2.png
Please fix.

dlib-devel.x86_64: W: hidden-file-or-dir /usr/share/doc/dlib-devel/docs/python/.doctrees
dlib-devel.x86_64: W: hidden-file-or-dir /usr/share/doc/dlib-devel/docs/python/.doctrees
dlib-devel.x86_64: W: hidden-file-or-dir /usr/share/doc/dlib-devel/docs/python/.buildinfo
Just remove those in %install or %exclude them.

python2-dlib.x86_64: W: no-documentation
python3-dlib.x86_64: W: no-documentation
6 packages and 0 specfiles checked; 0 errors, 12 warnings.
OK.

The examples in python2-dlib and python3-dlib use #!/usr/bin/python. For python2- this is OK, but for python3 it is not, because it introduces a dependency on /usr/bin/pytyhon, i.e. the python2 package.
Best option is to sed the header to usr %{__python2} and %{__python3} respectively.

Thank you for the review!

I hope I fixed everything, but probably I misunterstood you and used too complicated way to remove dotfiles from documentation. Is it possible to do it easier? I did not find out how to use %exclude in this case.

Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/bizdelnick/neuro/dlib.git/tree/dlib.spec?id=102f10d750dbf33c62cb770d40ddf7d5191acf0e
SRPM URL: https://copr-be.cloud.fedoraproject.org/results/bizdelnick/neuro/fedora-rawhide-x86_64/00147435-dlib/dlib-18.18-1.fc24.src.rpm

Should I close it?


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

Hi Cristopher!

I'm sorry, I had to search for duplicates better... We can cooperate working on thiis package. Unfortunately your spec and srpm files are currently unavailable. Was there something that I missed?

*** Bug 969631 has been marked as a duplicate of this bug. ***

Please link to the raw spec file in the 'Spec URL' field. Otherwise fedora-review and other automated tools (or even running wget to get the file) don't work.


The License field needs further correction (sorry, what I said above wasn't fully correct). The "and Public Domain" part only applies to the examples. If the examples were included e.g. in the -devel subpackage, than that subpackage would have a different license from the main subpackage. But I see that the examples are not packaged at all. So...

1. You should split out a -doc subpackage. The documentation is pretty big, and there's no need to install it everywhere.

2. You should include the examples in -doc. They will be pretty useful for users of the library. They don't have to be compiled.

3. Finally have License:Boost at the top of the spec file, and then License:Boost and Public Domain in the -doc subpackage.

4. Python packages include the examples, under the Public Domain license, so they should have License:Boost and Public Domain. You should also include LICENSE_FOR_EXAMPLE_PROGRAMS.txt in the %license field for those packages.

> I hope I fixed everything, but probably I misunterstood you and used too 
> complicated way to remove dotfiles from documentation. Is it possible to
> do it easier? I did not find out how to use %exclude in this case.
What you did is fairly straightforward. You can simplify it a bit by
doing the removal directly in %build using relative path:
rm -r docs/python/.{buildinfo,doctrees}

Using %exclude would look like
%files devel
...
%exclude %{_docdir}/%{name}-devel/docs/python/.buildinfo
%exclude %{_docdir}/%{name}-devel/docs/python/.doctrees
but I think that removing them in %install is better (simpler and less error prone) and removing them in %build is even better.


In the build I see the following:
-- Found BLAS library
-- Looking for cblas_ddot
-- Looking for cblas_ddot - not found
-- BLAS library does not have cblas symbols, so dlib will not use BLAS or LAPACK
 *****************************************************************************
 *** No BLAS library found so using dlib's built in BLAS.  However, if you ***
 *** install an optimized BLAS such as OpenBLAS or the Intel MKL your code ***
 *** will run faster.  On Ubuntu you can install OpenBLAS by executing:    ***
 ***    sudo apt-get install libopenblas-dev liblapack-dev                 ***
 *** Or you can easily install OpenBLAS from source by downloading the     ***
 *** source tar file from http://www.openblas.net, extracting it, and      ***
 *** running:                                                              ***
 ***    make; sudo make install                                            ***
 *****************************************************************************
Most likely the test is wrong. It is possible that you might need add more '-lxxx' compilation options.


rpmlint:
dlib.src:90: W: mixed-use-of-spaces-and-tabs (spaces: line 90, tab: line 1)

Looks good otherwise.

--

Regarding sponsorship: I'd be happy to sponsor you. Can you do two or three
reviews of packages from http://fedoraproject.org/PackageReviewStatus/NEW.html
and post the links here?

Thank you Zbigniew!

> The "and Public Domain" part only applies to the examples.
There are also data files under CC-BY-SA. I added them to -doc package too as well as license.

> 1. You should split out a -doc subpackage.
Should the %doc macro be used in this case? I thought that it is only necessary for installing with --excludedocs option, but using it for *-doc package is nonsense. However this macro is used in Fedora packages that I looked at.

> Most likely the test is wrong.
No, it is BR that was wrong. I added openblas-devel and removed blas-devel and lapack-devel that are linked statically into openblas.

SRPM URL: https://copr-be.cloud.fedoraproject.org/results/bizdelnick/neuro/fedora-rawhide-x86_64/00149626-dlib/dlib-18.18-1.fc24.src.rpm
Spec URL: http://copr-dist-git.fedorainfracloud.org/cgit/bizdelnick/neuro/dlib.git/plain/dlib.spec?id=be566ba74ab3cd34234bd9935c9cd25f931587f1

A couple of reviews:
https://bugzilla.redhat.com/show_bug.cgi?id=1279112#c1
https://bugzilla.redhat.com/show_bug.cgi?id=1293735#c6

(In reply to Dmitry Mikhirev from comment #10)
> > The "and Public Domain" part only applies to the examples.
> There are also data files under CC-BY-SA. I added them to -doc package too
> as well as license.
Ack.

> > 1. You should split out a -doc subpackage.
> Should the %doc macro be used in this case? I thought that it is only
> necessary for installing with --excludedocs option, but using it for *-doc
> package is nonsense. However this macro is used in Fedora packages that I
> looked at.

Using %doc still makes sense, because documentation packages are sometimes
pulled in by dependencies (including Recommends). Sometimes -doc packages
might be installed on upgrade, when a -doc subpackages is split out of
the main package. Using %doc even in -doc subpackage makes rpm --excludedocs
work consistently.

> > Most likely the test is wrong.
> No, it is BR that was wrong. I added openblas-devel and removed blas-devel
> and lapack-devel that are linked statically into openblas.
> 
> SRPM URL:
> https://copr-be.cloud.fedoraproject.org/results/bizdelnick/neuro/fedora-
> rawhide-x86_64/00149626-dlib/dlib-18.18-1.fc24.src.rpm
> Spec URL:
> http://copr-dist-git.fedorainfracloud.org/cgit/bizdelnick/neuro/dlib.git/
> plain/dlib.spec?id=be566ba74ab3cd34234bd9935c9cd25f931587f1

Looks good.

rpmlint:

dlib.i686: W: no-documentation
dlib-devel.i686: W: no-documentation
python2-dlib.i686: W: no-documentation
python3-dlib.i686: W: no-documentation
That is OK.

dlib-doc.i686: W: hidden-file-or-dir /usr/share/doc/dlib-doc/docs/python/.buildinfo
dlib-doc.i686: W: hidden-file-or-dir /usr/share/doc/dlib-doc/docs/python/.doctrees
dlib-doc.i686: W: hidden-file-or-dir /usr/share/doc/dlib-doc/docs/python/.doctrees
Yeah, see comment #9. Please fix that up in the initial build.

7 packages and 0 specfiles checked; 0 errors, 7 warnings.

Package is APPROVED.

(In reply to Dmitry Mikhirev from comment #11)
> A couple of reviews:
> https://bugzilla.redhat.com/show_bug.cgi?id=1279112#c1
> https://bugzilla.redhat.com/show_bug.cgi?id=1293735#c6

Ack. I've added you to the packagers group. Welcome!
I'm happy to help with any questions or issues you might have.
It would be great if you could now finish those reviews ;)

Thank you Zbigniew!

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/dlib