Bug 1427341

Some initial comments:

You should get rid of the macros on top of the SPEC file as these are required only for EPEL.

You should make consistent use of macros so URL should be in the form of:
https://github.com/hsnr-%{srcname}/%{srcname}

and the Source0 url in a similar fashion (don't forget the %{version} macro)

For the '%{__python2} setup.py build' there is already a macro that you can use: '%py2_build'

If the c++ sources need to be removed (what is the reason?) they should be removed during prep and not after compiling them.

Also it seems that upstream there is no activity for 3 months now and the requirements for the project seems quite low, are you sure that gamera is compatible with the latest versions of its requirements (e.g. in Fedora rawhide we have now gcc 7)?

On another note please read the Fedora Python Packaging Guidelines [0] for more info on how to structure these kind of SPEC files.

[0] https://fedoraproject.org/wiki/Packaging:Python

The compilation fails as well when trying to build the SRPM in mock.

Hi VincentS,
I'm willing to sponsor you if you finish this and will review some packages as well. Lifting FE-NEEDSPONSOR.

Charalampos, please go ahead with the review, I'll be watching ;)

Thanks Charalampos and Miro,

I achieved modifications according to comments.

Updated srpm and spec file:
Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.2-2.fc25.src.rpm

I contacted upstrem about GCC7 because it doesn't compile only with it.

Miro, I reviewed Bug 1385244 - pew: https://bugzilla.redhat.com/show_bug.cgi?id=1385244
and Bug 1333928 - python-searchlightclient: https://bugzilla.redhat.com/show_bug.cgi?id=1333928
If you have packages to review, you can ask me.

(In reply to VincentS from comment #4)
> Miro, I reviewed Bug 1385244 - pew:
> https://bugzilla.redhat.com/show_bug.cgi?id=1385244
> and Bug 1333928 - python-searchlightclient:
> https://bugzilla.redhat.com/show_bug.cgi?id=1333928
> If you have packages to review, you can ask me.

Please go on with those than.

Tip: In case you have a different opinion than the author on something but the thing os still valid (e.g. with the macros in Bug 1333928), please be less demanding when expressing your opinion - it's better to say: "I'd rather do it like this (show), because I think it would make the spec easier to understand, what do you think?" etc., than "I think you should do this and that".

Miro, thanks for your recommendation. I'll be less harsh.

While waiting for modifications on these review requests, I continue to do some informal reviews.

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

Created attachment 1260845 [details]
gcc 7 build log

So here is the build log from the failures with gcc 7.0.1, which can be sent to the upstream developers.

Also another useful document for them would be https://gcc.gnu.org/gcc-7/porting_to.html

Fedora 25 (which includes gcc 6.3.1) compiles just fine, however it would be for the best, that this compilation issue gets resolved so the package can be built against the latest Fedora branches as well.

The review will need to be stalled until the compilation issue is resolved.

As soon as this is resolved upstream, a patch with the fixes can be applied to the package (I can explain the steps if we reach that point) or upstream will release a new version, so just an update to the latest sources will be sufficient.

Please post in the bugzilla if there is some movement from upstream in that direction so the package review can proceed.

After having discussed with upstream, he told me that he couldn't solve this problem because he doesn't use GCC7. So I decided to try to find a solution.

Finally, I fixed the issue. I'm waiting the validation of the pull request.
https://github.com/hsnr-gamera/gamera/pull/7

Upstream updated to a new release with gcc7 supported.

Here are new spec file and srpm:
Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-1.fc25.src.rpm

Moreover, I build with Koji to verify if it compile for rawhide.
https://koji.fedoraproject.org/koji/taskinfo?taskID=18270494

I did another informal review.
https://bugzilla.redhat.com/show_bug.cgi?id=1429226

As this package is quite complex I will post my findings so far and after we get these out of the way, we can proceed with the rest.

All the BuildRequires are fine.

You should remove the comment after %install about the python2 and python3 install, since the package is python2 and only uses the %py2_install macro

The desktop file installation should happen after the %py2_install.

Also what is the reason for this line?
chmod 755 %{buildroot}/%{python2_sitearch}/%{srcname}/gendoc.py
Same for: %doc %attr(644,-,-) README
And: %defattr(644,root,root,755)

The devel subpackage should have a Requires tag for the main package.
See: https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

This comment is not required: # For noarch packages: sitelib

Also while it is good to keep some level of abstraction when specifying the directories and binaries at the files section, I would recommend to be more specific about some things, since it helps while working with the package. e.g.

Remove: '%{python2_sitearch}/%{srcname}*'
Add '%{python2_sitearch}/%{srcname}' and '%{python2_sitearch}/%{srcname}-%{version}-py?.?.egg-info'

Also the %{_includedir}/python2.7/%{srcname}/ should be changed to %{_includedir}/python%{python2_version}/%{srcname}/

Now as for the binaries. The package provides two of them for which it would be good practice to list them individually at the respective files sections, so basically instead of %{_bindir}/*, use %{_bindir}/gamera_gui and %{_bindir}/gamera_post_install.py

However what is the purpose of gamera_post_install.py binary? I kinda think that it might be required to be included at the devel subpackage.

Also the package ships a lot of .so libraries so you should read the respective section at the packaging guidelines: https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

(In reply to Charalampos Stratakis from comment #11)
> Also the package ships a lot of .so libraries so you should read the
> respective section at the packaging guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

In python2_sitearch or in /usr/lib64/lib...so ?

(In reply to Miro Hrončok from comment #12)
> (In reply to Charalampos Stratakis from comment #11)
> > Also the package ships a lot of .so libraries so you should read the
> > respective section at the packaging guidelines:
> > https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries
> 
> In python2_sitearch or in /usr/lib64/lib...so ?
At

/usr/lib64/python2.7/site-packages/gamera/

and

/usr/lib64/python2.7/site-packages/gamera/plugins/

Those are fine and the https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries does not apply for those.

(In reply to Charalampos Stratakis from comment #11)
> As this package is quite complex I will post my findings so far and after we
> get these out of the way, we can proceed with the rest.
> 
> All the BuildRequires are fine.
> 
> You should remove the comment after %install about the python2 and python3
> install, since the package is python2 and only uses the %py2_install macro
 
All right.

> The desktop file installation should happen after the %py2_install.

Done

> Also what is the reason for this line?
> chmod 755 %{buildroot}/%{python2_sitearch}/%{srcname}/gendoc.py
> Same for: %doc %attr(644,-,-) README
> And: %defattr(644,root,root,755)

As guidelines say, Permissions on files must be set properly.
I saw with rmplint : 
- python-gamera.x86_64: E: wrong-script-interpreter /usr/lib64/python2.7/site-packages/gamera/gendoc.py /usr/bin/env python
- python-gamera.x86_64: E: non-executable-script /usr/lib64/python2.7/site-packages/gamera/gendoc.py 644 /usr/bin/env python
- python-gamera.x86_64: W: spurious-executable-perm /usr/share/doc/python-gamera/README
I thought permissions hadn't set properly, so I added chmod and %defattr to solve these problems.
What do you think about ?
 
> The devel subpackage should have a Requires tag for the main package.
> See:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Requiring_Base_Package

Yes I forgot.

> This comment is not required: # For noarch packages: sitelib

Done

> Also while it is good to keep some level of abstraction when specifying the
> directories and binaries at the files section, I would recommend to be more
> specific about some things, since it helps while working with the package.
> e.g.
> 
> Remove: '%{python2_sitearch}/%{srcname}*'
> Add '%{python2_sitearch}/%{srcname}' and
> '%{python2_sitearch}/%{srcname}-%{version}-py?.?.egg-info'
 
Ok I understand.

> Also the %{_includedir}/python2.7/%{srcname}/ should be changed to
> %{_includedir}/python%{python2_version}/%{srcname}/

All right.

> Now as for the binaries. The package provides two of them for which it would
> be good practice to list them individually at the respective files sections,
> so basically instead of %{_bindir}/*, use %{_bindir}/gamera_gui and
> %{_bindir}/gamera_post_install.py

Great, for few binaries I need to be precise.

> However what is the purpose of gamera_post_install.py binary? I kinda think
> that it might be required to be included at the devel subpackage.

In fact, this file is only needed for Windows installation. So I removed it.

> Also the package ships a lot of .so libraries so you should read the
> respective section at the packaging guidelines:
> https://fedoraproject.org/wiki/Packaging:Guidelines#Shared_Libraries

Thanks Miro for your reply and Charalampos for your comments.

(In reply to VincentS from comment #15)
> (In reply to Charalampos Stratakis from comment #11)
> > Also what is the reason for this line?
> > chmod 755 %{buildroot}/%{python2_sitearch}/%{srcname}/gendoc.py
> > Same for: %doc %attr(644,-,-) README
> > And: %defattr(644,root,root,755)
> 
> As guidelines say, Permissions on files must be set properly.
> I saw with rmplint : 
> - python-gamera.x86_64: E: wrong-script-interpreter
> /usr/lib64/python2.7/site-packages/gamera/gendoc.py /usr/bin/env python
> - python-gamera.x86_64: E: non-executable-script
> /usr/lib64/python2.7/site-packages/gamera/gendoc.py 644 /usr/bin/env python
> - python-gamera.x86_64: W: spurious-executable-perm
> /usr/share/doc/python-gamera/README
> I thought permissions hadn't set properly, so I added chmod and %defattr to
> solve these problems.
> What do you think about ?

First, /usr/lib64/python2.7/site-packages/gamera/gendoc.py:

This is a bit more complicated.

Based on the rpmlint output you can say that:

 * the file has a shebang that says "#!/usr/bin/env python" (note that using /usr/bin/env in shebangs is forbidden in Fedora packages)
 * the file does not have executable permissions

So you have to ask yourself the question: "Shall this file be treated as executable or not?" The answer is usually "not" unless the file is in PATH (such as /usr/bin). So in this case instead of giving it permissions, remove the shebang. This is nice approach, as it checks if the shebang is there and if so, it removes it:

    head -n 1 gamera/gendoc.py | grep '#!/usr/' && sed -i '1d' gamera/gendoc.py

If the file should have been an executable (not this case), you would need to change the shebang to "#!/usr/bin/python2" and add the executable permission.

Second: README:

I'd suggest running "chmod -x README" in %prep instead, as it explicitly says "remove the executable permission" rather than "set the permissions to exactly this and that".

(In reply to Miro Hrončok from comment #16)
> First, /usr/lib64/python2.7/site-packages/gamera/gendoc.py:
> 
> This is a bit more complicated.
> 
> Based on the rpmlint output you can say that:
> 
>  * the file has a shebang that says "#!/usr/bin/env python" (note that using
> /usr/bin/env in shebangs is forbidden in Fedora packages)
>  * the file does not have executable permissions
> 
> So you have to ask yourself the question: "Shall this file be treated as
> executable or not?" The answer is usually "not" unless the file is in PATH
> (such as /usr/bin). So in this case instead of giving it permissions, remove
> the shebang. This is nice approach, as it checks if the shebang is there and
> if so, it removes it:
> 
>     head -n 1 gamera/gendoc.py | grep '#!/usr/' && sed -i '1d'
> gamera/gendoc.py
> 
> If the file should have been an executable (not this case), you would need
> to change the shebang to "#!/usr/bin/python2" and add the executable
> permission.
> 
> Second: README:
> 
> I'd suggest running "chmod -x README" in %prep instead, as it explicitly
> says "remove the executable permission" rather than "set the permissions to
> exactly this and that".

Ok, all right.

Now I'll know for future through your really good explanations.

I fixed this according to your recommendations.

Here are new SPEC and SRPM:
Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-2.fc25.src.rpm

OK, let me have a look.

1) Summary. You don't have to set the %{sum} macro, you can write a summary into the Summary field and then use %{summary} in the subpackages.

2) Description. I don't think an URL belongs there. Just use it as the URL instead of the github one.

3) Since this is Python 2 only, I think you should have python2-gamera subpackage instead of python-gamera package. Once python- will mean python3, you would have trouble this way.

4) The summary of devel subpackage should definitely not bee the same as the main package.

5) "Devel gamera package." is not verbose enough as description. "Development package for gamera. Allows you to develop your apps on top of the gamera framework." might be a little bit better. 

6) I'd add some whitespace to %install so the lines that do one thing are together

7) Add comment to the shebang removal so it's obvious what's happening there and WHY

8) I don't think the devel subpackage is a header only library per se, so the -static provide here is bogus.

s/bee/be/

Here are new links according to Miro's advises.

Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-3.fc25.src.rpm

Hello Vincent,


Please omit this sentence from the description:
"For more information about Gamera, visit the Gamera website at:"

Use the summary macro as %{summary}, not %summary

Add this description for the python2 subpackage (which is basically the same as the original description):

"Gamera is a framework for building document analysis applications.
It is not a packaged document recognition system, 
but a toolkit for building document image recognition systems."

Also a comment above 'chmod -x README' would be nice. Something like "Remove the executable bit from the README file' or something equivalent.

I believe these should be the last items required before I run the fedora-review tool for the package.

(In reply to Charalampos Stratakis from comment #21)
> Also a comment above 'chmod -x README' would be nice. Something like "Remove
> the executable bit from the README file' or something equivalent.

I disagree here. That would be "captain obvious comment". If a comment is desired, better explain why are we doing that, not what are we doing (that's clear from the command itself, it's not a super ugly complicated nested bash-fu). Something like: "The README is obviously not supposed to be executed, it doesn't even have a shebang". Also, maybe fix this upstream instead?

I followed your advises Charalampos and Miro and I will discuss with upstream about executable right on README file.

Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-4.fc25.src.rpm

So after some discussions with Miro and running the fedora-review tool we figured out some more things.

The devel subpackage should be named python2-gamera-devel and this macro should be used to provide the python namespace for it (similar to the main package):

%{?python_provide:%python_provide python2-%{srcname}-devel}

The package bundles some software as well as it can be seen from these directories:

https://github.com/hsnr-gamera/gamera/tree/master/src
https://github.com/hsnr-gamera/gamera/tree/master/include

It bundles vigra, zlib, eo, libpng and libtiff (hope I didn't miss anything).
The zlib folders should be removed during prep (from the Include and src subdirs) and instead a BuildRequires for the zlib-devel package should be used.

Same for libpng-1.2.5 and libtiff folders.

As for the rest of the bundled software, they should be provided as virtual provides according to:
https://fedoraproject.org/wiki/Bundled_Libraries?rd=Packaging:Bundled_Libraries

So you will need to add at the main package:
Provides: bundled(vigra) = 1.6.0
Provides: bundled(eo) = 1.3.1

Also there is an executable bit which should be removed during prep from:

/include/plugins/deformations.hpp

The package has documentation as well which must be built. I checked the sources and docs are being generated by executing the file gendoc.py. Then it should be be placed at a separate subpackage, named python2-gamera-doc, with a similar naming pattern as the devel subpackage.

I still haven't figured out the best way to do that, as gamera uses itself to build the documentation, so some environment variable will have to be exported during build. I will conduct some more testing and let you know.

Now about the license.

From the fedora-review output:
Licenses found: "LGPL (v2)", "*No copyright* LGPL (v2 or later)", "LGPL (v2 or later)", "GPL (v2 or later)", "CC by-sa GPL", "Unknown or generated", "MIT/X11 (BSD like)", "NTP", "MIT/X11 (BSD like) NTP", "zlib/libpng", "LGPL", "GPL (v2 or later) (with incorrect FSF address)"

The final license of the package (indicated at the License field) should be "GPLv2+ and MIT and CC BY-SA" (thanks Miro), however I will need to dig a bit deeper to verify that, so don't change the field yet.

(In reply to Charalampos Stratakis from comment #24)
> The package has documentation as well which must be built.

Should, not must.

Created attachment 1271137 [details]
licenses

After some digging it seems that the original deduction is true, so the actual license of the package is "GPLv2+ and MIT and CC BY-SA", however I would highly recommend to send an email at the fedora-legal mailing list ( legal.org ) with the attached text and a description of the situation, to ensure that this should be the final license of the package.

I have omitted the licenses of zlib/libtiff from the file, as these files should be removed during prep.

Also in regards to the documentation. I figured out how to generate it, but it will require some hacks at the SPEC file and certainly it's not a blocker for accepting the package.

If you would like to generate the documentation (before or after the review of the package) please reach out to me or Miro, so we can guide you in that respect.

Any news on the package?

The submitter hasn't answered in over a month. According to the policy for stalled package reviews [0] I'm declaring this review as stalled, and if no response is given within a week, this bugzilla will be closed.

[0] https://fedoraproject.org/wiki/Policy_for_stalled_package_reviews?rd=Extras/Policy/StalledReviews

I'm sorry about no news and thank you for all your work on it, actually I'm waiting a response from legal.org about license.

Otherwise, I modified Spec with all your recommendations. I only hadn't add documentation.

Do I should have a response before send new version of SPEC file ?

(In reply to VincentS from comment #30)
> I'm sorry about no news and thank you for all your work on it, actually I'm
> waiting a response from legal.org about license.


I don't see your e-mail there.

Really ? I don't understand.

I just resent it.

(In reply to Miro Hrončok from comment #31)
> (In reply to VincentS from comment #30)
> > I'm sorry about no news and thank you for all your work on it, actually I'm
> > waiting a response from legal.org about license.
> 
> 
> I don't see your e-mail there.

I received this answer :

Your mail to 'legal.org' with the subject

    Fwd: Difficulty about package license

Is being held until the list moderator can review it for approval.

The message is being held because:

    The message is not from a list member

Either the message will get posted to the list, or you will receive
notification of the moderator's decision.

you have to subscribe to the ML first

see "Manage subscription" button here https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/

(In reply to Matthieu Saulnier from comment #34)
> you have to subscribe to the ML first
> 
> see "Manage subscription" button here
> https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/

Thank you for your reply Matthieu.

I signed up to the list and resent the mail.

I've done all modifications recommended, here are new links.

I didn't try to generate documentation, I'm going to see this later on.

Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-5.fc25.src.rpm

Hello Vincent and thanks for continuing working on it. I currently replied to the legal mailing list as the license issue is not yet entirely clear. However I will proceed with the fedora-review tool so the review can move a bit forward.


Package Review
==============

Legend:
[x] = Pass, [!] = Fail, [-] = Not applicable, [?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
- All build dependencies are listed in BuildRequires, except for any that
  are listed in the exceptions section of Packaging Guidelines.
  Note: These BR are not needed: gcc
  See: http://fedoraproject.org/wiki/Packaging/Guidelines#Exceptions_2

Note from reviewer: gcc is now required to be listed as a dependency.
See: https://fedoraproject.org/wiki/Packaging:C_and_C%2B%2B#BuildRequires_and_Requires


===== MUST items =====

C/C++:
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Development (unversioned) .so files in -devel subpackage, if present.
     Note: Unversioned so-files in private %_libdir subdirectory (see
     attachment). Verify they are not in ld path.

Note from reviewer: The shared libraries are in /usr/lib64/python2.7/site-packages/gamera/ and upstream does not version them
however they are required for runtime, thus they are not development .so files.

They are also not included in ld path.

[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.

[?]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses
     found: "LGPL (v2)", "*No copyright* LGPL (v2 or later)", "LGPL (v2 or
     later)", "GPL (v2 or later)", "CC by-sa GPL", "Unknown or generated",
     "MIT/X11 (BSD like)", "NTP", "MIT/X11 (BSD like) NTP", "LGPL", "GPL
     (v2 or later) (with incorrect FSF address)". 295 files have unknown
     license.

Note from reviewer: Still waiting on an answer from the legal mailing list.
Thread here:
https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/AB5S7LXEVXWR4VRKYGZO3MXHGFFZJAGO/

[x]: License file installed when any subpackage combination is installed.
[?]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.

Note from reviewer: Still waiting on an answer from the legal mailing list

[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: Package contains no bundled libraries without FPC exception.

Note from reviewer: FPC involvement is not required anymore, see https://pagure.io/fesco/issue/1483

[x]: Changelog in prescribed format.
[x]: Sources contain only permissible code or content.
[x]: Development files must be in a -devel package
[x]: Package uses nothing in %doc for runtime.
[x]: Package consistently uses macros (instead of hard-coded directory
     names).
[x]: Package is named according to the Package Naming Guidelines.
[x]: Package does not generate any conflict.
[x]: Package obeys FHS, except libexecdir and /usr/target.
[-]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[!]: Requires correct, justified where necessary.

Note from reviewer: Package should have a runtime requirement for wxPython as the gamera_gui binary needs it to run.
If it's not present a traceback is shown.

You should add:

Requires: wxPython

Right after the summary tag of the python2 subpackage.

[x]: Spec file is legible and written in American English.
[-]: Package contains systemd file(s) if in need.
[x]: Useful -debuginfo package or justification otherwise.
[x]: Package is not known to require an ExcludeArch tag.
[-]: Large documentation must go in a -doc subpackage. Large could be size
     (~1MB) or number of files.
     Note: Documentation size is 10240 bytes in 1 files.

Note from reviewer: Documentation is not being built. It is not a blocker for the review
and it's up to the packager if he wants to do it at some point in the future.

[x]: Package complies to the Packaging Guidelines
[x]: Package successfully compiles and builds into binary rpms on at least
     one supported primary architecture.
[x]: Package installs properly.
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: If (and only if) the source package includes the text of the
     license(s) in its own file, then that file, containing the text of the
     license(s) for the package is included in %license.
[x]: Package requires other packages for directories it uses.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package uses either %{buildroot} or $RPM_BUILD_ROOT
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Macros in Summary, %description expandable at SRPM build time.
[x]: Package contains desktop file if it is a GUI application.
[x]: Package installs a %{name}.desktop using desktop-file-install or
     desktop-file-validate if there is such a file.
[x]: Dist tag is present.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package use %makeinstall only when make install DESTDIR=... doesn't
     work.
[x]: Package is named using only allowed ASCII characters.
[x]: Package does not use a name that already exists.
[x]: Package is not relocatable.
[x]: Sources used to build the package match the upstream source, as
     provided in the spec URL.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: File names are valid UTF-8.
[x]: Packages must not store files under /srv, /opt or /usr/local

Python:
[x]: Python eggs must not download any dependencies during the build
     process.
[x]: A package which is used by another package via an egg interface should
     provide egg info.
[x]: Package meets the Packaging Guidelines::Python
[x]: Package contains BR: python2-devel or python3-devel
[x]: Binary eggs must be removed in %prep

===== SHOULD items =====

Generic:
[-]: If the source package does not include license text(s) as a separate
     file from upstream, the packager SHOULD query upstream to include it.
[x]: Final provides and requires are sane (see attachments).

Also proposing to add two additional provides tags as the package can also be considered an application.
e.g.

Provides: gamera = %{version}-%{release}

and

Provides: gamera%{?_isa} = %{version}-%{release}

See also: https://fedoraproject.org/wiki/Packaging:Guidelines#Libraries_and_Applications

[x]: Fully versioned dependency in subpackages if applicable.
[?]: Package functions as described.

Note from reviewer: Did some initial testing and it imports successfully with python2.
Also after installing the dependency on wxPython (see above)the binary works.
Will need to do some more testing although it seems that the package is functional.
Leaving this option as "not evaluated" till I conduct some more testing.

[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed
     files.
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[x]: Packager, Vendor, PreReq, Copyright tags should not be in spec file
[x]: Sources can be downloaded from URI in Source: tag
[x]: SourceX is a working URL.
[x]: Spec use %global instead of %define unless justified.

===== EXTRA items =====

Generic:
[x]: Rpmlint is run on debuginfo package(s).
     Note: There are rpmlint messages (see attachment).
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package
     is arched.
[x]: Spec file according to URL is the same as in SRPM.


Rpmlint
-------
Checking: python2-gamera-3.4.3-5.fc27.x86_64.rpm
          python2-gamera-devel-3.4.3-5.fc27.x86_64.rpm
          python-gamera-debuginfo-3.4.3-5.fc27.x86_64.rpm
          python-gamera-3.4.3-5.fc27.src.rpm
python2-gamera.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
python2-gamera.x86_64: W: hidden-file-or-dir /usr/lib/.build-id
python2-gamera.x86_64: W: no-manual-page-for-binary gamera_gui
python2-gamera-devel.x86_64: W: no-documentation
<snip>

Too many warnings for wrong address of FSF however they are coming from the bundled libraries.

Requires
--------
python2-gamera (rpmlib, GLIBC filtered):
    /usr/bin/python2
    libc.so.6()(64bit)
    libgcc_s.so.1()(64bit)
    libgcc_s.so.1(GCC_3.0)(64bit)
    libgcc_s.so.1(GCC_4.0.0)(64bit)
    libgomp.so.1()(64bit)
    libgomp.so.1(GOMP_1.0)(64bit)
    libgomp.so.1(GOMP_2.0)(64bit)
    libgomp.so.1(GOMP_4.0)(64bit)
    libgomp.so.1(OMP_1.0)(64bit)
    libgomp.so.1(OMP_2.0)(64bit)
    libm.so.6()(64bit)
    libpng16.so.16()(64bit)
    libpng16.so.16(PNG16_0)(64bit)
    libpthread.so.0()(64bit)
    libpython2.7.so.1.0()(64bit)
    libstdc++.so.6()(64bit)
    libstdc++.so.6(CXXABI_1.3)(64bit)
    libstdc++.so.6(CXXABI_1.3.1)(64bit)
    libstdc++.so.6(CXXABI_1.3.8)(64bit)
    libstdc++.so.6(CXXABI_1.3.9)(64bit)
    libtiff.so.5()(64bit)
    libtiff.so.5(LIBTIFF_4.0)(64bit)
    python(abi)
    rtld(GNU_HASH)

python-gamera-debuginfo (rpmlib, GLIBC filtered):

python2-gamera-devel (rpmlib, GLIBC filtered):
    python(abi)
    python-gamera(x86-64)



Provides
--------
python2-gamera:
    application()
    application(python-gamera.desktop)
    python-gamera
    python-gamera(x86-64)
    python2-gamera
    python2-gamera(x86-64)
    python2.7dist(gamera)
    python2dist(gamera)

python-gamera-debuginfo:
    python-gamera-debuginfo
    python-gamera-debuginfo(x86-64)

python2-gamera-devel:
    python-gamera-devel
    python-gamera-devel(x86-64)
    python2-gamera-devel
    python2-gamera-devel(x86-64)



Unversioned so-files
--------------------
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/gameracore.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/graph.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/kdtree.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/knncore.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/knnga.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_arithmetic.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_binarization.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_color.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_contour.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_convolution.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_corelation.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_deformation.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_draw.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_edgedetect.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_features.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_fourier_features.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_geometry.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_gui_support.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_image_conversion.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_image_utilities.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_listutilities.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_logical.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_misc_filters.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_misc_free_functions.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_morphology.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_pagesegmentation.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_png_support.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_projections.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_runlength.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_segmentation.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_string_io.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_structural.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_thinning.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_threshold.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_tiff_support.so
python2-gamera: /usr/lib64/python2.7/site-packages/gamera/plugins/_transformation.so

Source checksums
----------------
https://github.com/hsnr-gamera/gamera/releases/download/3.4.3/gamera-3.4.3.tar.gz :
  CHECKSUM(SHA256) this package     : 02c194e95822e6f5499ddc4770ec5ad02ac613af274f31aa7ada158d657ac284
  CHECKSUM(SHA256) upstream package : 02c194e95822e6f5499ddc4770ec5ad02ac613af274f31aa7ada158d657ac284


Generated by fedora-review 0.6.1 (f03e4e7) last change: 2016-05-02
Command line :/usr/bin/fedora-review -b 1427341 -m fedora-rawhide-x86_64
Buildroot used: fedora-rawhide-x86_64
Active plugins: Python, Generic, Shell-api, C/C++
Disabled plugins: Java, SugarActivity, fonts, Haskell, Ocaml, Perl, R, PHP
Disabled flags: EXARCH, DISTTAG, EPEL5, BATCH, EPEL6

Conducted some additional testing and the gui works fine under wayland as well as xorg on a F25 VM.

[x]: Package functions as described.

The blockers now are adding the requires for wxPython and getting a definite answer from lega.

Thanks for the review.
I'm going to do the correction and resend an email to legal during the next week.

So since legal hasn't answered for some time now [0] I'm gonna assume that the correct license(s) should be "GPLv2 and GPLv2+ and MIT and CC-BY-SA", so the review can be continued.

Miro what would you think?

Also do not forget to add the Requires: wxPython

[0] https://lists.fedoraproject.org/archives/list/legal@lists.fedoraproject.org/thread/AB5S7LXEVXWR4VRKYGZO3MXHGFFZJAGO/

I would only use CC-BY-SA for the docs subpackage.

So, after all you said. Should I put "GPLv2 and GPLv2+ and MIT and CC-BY-SA" as package licence and only "CC-BY-SA" as docs subpackage licence ?

(In reply to VincentS from comment #42)
> So, after all you said. Should I put "GPLv2 and GPLv2+ and MIT and CC-BY-SA"
> as package licence and only "CC-BY-SA" as docs subpackage licence ?

I'd go with "GPLv2 and GPLv2+ and MIT" as the main package licence (note that it specifies the license of the built binary RPM, not the whole source RPM) and "CC-BY-SA" as docs subpackage licence.

Thanks for your help, here are new links.

Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-6.fc25.src.rpm

Hello Vincent and thanks for your work and your patience with the package review.

Technically all seems fine now. The package builds and works as expected.

[x]: License field in the package spec file matches the actual license.

[x]: Requires correct, justified where necessary.

Just one last item on the list. Please add a comment above the license tag with a small explanation of the licenses breakdown [0]. Also please mention there that the CC-BY-SA is for the documentation subpackage that hasn't been built/shipped yet.

[!]: If the package is under multiple licenses, the licensing breakdown
     must be documented in the spec.

[0] https://fedoraproject.org/wiki/Packaging:LicensingGuidelines?rd=Packaging/LicensingGuidelines#Multiple_Licensing_Scenarios

Hello Charalampos, thanks you for your time on this package review.

I've done all your recommendations.

Spec URL: https://dl.casperlefantom.net/pub/review/python-gamera.spec
SRPM URL: https://dl.casperlefantom.net/pub/review/python-gamera-3.4.3-7.fc25.src.rpm

Thanks.

Package accepted.

Waiting on Miro for the sponsorship approval and final comments.

Will try to do this by the end of this week.

Oh, sorry. I've just sponsored Vincent. Use your powers responsibly. Don't hesitate to ask me anything on #fedora-devel, or CC me in your reviews when in trouble.

Thank you very much ! Of course ! Please, may you tell me what is the next step for this package ?

Hello Vincent,

The instructions are outlined here [0]. You are at the point now for using the  fedrepo-req tool.

Please note though that due to some recent changes in the build infrastructure, there is the case that some things might not be clear or documented yet. If you stumble upon any of that, please notify us so we can clear it up for you.

[0] https://fedoraproject.org/wiki/Package_Review_Process

Hello Vincent, 
I'm setting "needinfo" on this bug to indicate it's currently waiting on you.
Please let us know if you need any info or instructions.

I'm sorry for the response time, at the moment, I'm really busy. I'll keep you in touch.

Pinging here.

The review is stalled. If there would be no response within one week I'll close the review request.