Bug 3429

Hi,

I've had this package on my list of interesting pkgs to review for a while now. So here we are almost a year later and I finally have some time to review this. I should have a review done for this pkg soon,

Note that currently rpmfusion infra is being migrated to git + koji, so atm new pkg requests are not being processed. So once this pkg passes review you unfortunately still need to wait a bit longer... But first lets get the review done.

Regards,

Hans

Full review done:

Good:
====
- rpmlint checks return:
[hans@shalem rpmbuild]$ rpmlint SRPMS/* RPMS/x86_64/*
frobtads.src: W: invalid-license non-commercial
frobtads.src:12: W: unversioned-explicit-provides bundled(md5-deutsch-c++)
frobtads.src:13: W: unversioned-explicit-provides bundled(sha2-gladman)
frobtads.x86_64: W: invalid-license non-commercial
frobtads-debuginfo.x86_64: W: invalid-license non-commercial
frobtads-devel.x86_64: W: invalid-license non-commercial
4 packages and 0 specfiles checked; 0 errors, 6 warnings.
 These can all be ignored
- package meets naming guidelines
- package meets packaging guidelines
- license (non-commercial) OK, matches source
- spec file legible, in am. english
- source matches upstream
- no missing BR
- no unnecessary BR
- no locales
- not relocatable
- owns all directories that it creates
- no duplicate files
- permissions ok
- macro use consistent
- code, not content
- no need for -docs
- nothing in %doc affects runtime
- no need for .desktop file

Needs work:
========
-license text not in %license, the actual license text is in tads3/LICENSE.TXT so the %license line should read:
 %license doc/COPYING tads3/LICENSE.TXT
-Package does not build, this is caused by you moving the installed tads manual to %{_pkgdocdir}-devel
 but not having a matching %doc line in %files. For the review I've fixed this by replacing the mkdir + mv
 you do after make install with:
rm -rf %{buildroot}%{_datadir}/frobtads/tads3/doc
 and adding the docs in question to "%files devel" like this:
%doc doc/COMPILERS tads3/doc/*
 Letting rpmbuild take care if the installation of the docs, which usually is the best way to deal with docs
-Why is %{_datadir}/frobtads/tads2 owned by the -devel pkg and not by the main pkg ?

Thanks for the reviews! Both packages have some complications - like the complex licensing and bundled stuff...

(In reply to comment #2)
> 
> Needs work:
> ========
> -license text not in %license, the actual license text is in tads3/LICENSE.TXT
> so the %license line should read:
>  %license doc/COPYING tads3/LICENSE.TXT

I overlook that, good catch. Fixed.

> -Package does not build, this is caused by you moving the installed tads manual
> to %{_pkgdocdir}-devel
>  but not having a matching %doc line in %files. For the review I've fixed this
> by replacing the mkdir + mv
>  you do after make install with:
> rm -rf %{buildroot}%{_datadir}/frobtads/tads3/doc
>  and adding the docs in question to "%files devel" like this:
> %doc doc/COMPILERS tads3/doc/*
>  Letting rpmbuild take care if the installation of the docs, which usually is
> the best way to deal with docs

Right, fixed this way.

> -Why is %{_datadir}/frobtads/tads2 owned by the -devel pkg and not by the main
> pkg ?

These files are include files used only by Tads 2 compiler. I placed header files and the compilers (+ the TADS includes) into the -devel subpackage.

On other hand, tads3/res/charmap/cmaplib.t3r is needed for runtime and in the main package.


Updated version:

Spec URL: http://scientific.zcu.cz/fedora/frobtads-1.2.3-1b/frobtads.spec
SRPM URL: http://scientific.zcu.cz/fedora/frobtads-1.2.3-1b/frobtads-1.2.3-1.fc24.src.rpm

Hi,

(In reply to comment #3)
> > -Why is %{_datadir}/frobtads/tads2 owned by the -devel pkg and not by the main
> > pkg ?
> 
> These files are include files used only by Tads 2 compiler. I placed header
> files and the compilers (+ the TADS includes) into the -devel subpackage.
> 
> On other hand, tads3/res/charmap/cmaplib.t3r is needed for runtime and in the
> main package.

Ok, thanks for the explanation. 

> Updated version:
> 
> Spec URL: http://scientific.zcu.cz/fedora/frobtads-1.2.3-1b/frobtads.spec
> SRPM URL:
> http://scientific.zcu.cz/fedora/frobtads-1.2.3-1b/frobtads-1.2.3-1.fc24.src.rpm

Looks good now. Next time you do a new version for a pkg-review please bump the release field and at a changelog with the changes you made, just like you would do in a Fedora pkg review. No need to respin for that now.

This package is approved.

Regards,

Hans

Package CVS request
======================
Package Name: frobtads
Short Description: Text interpreter for Tads games
Owners: valtri
Branches: f23
InitialCC:
----------------------
License tag: nonfree

https://pkgs.rpmfusion.org/cgit/?q=frobtads this package wasn't created , this is correct or should we block #33 again ?

Yes, 'rfpkg clone frobtads' still fails (checkout of the other packages works fine, like 'qtads'). -- Thanks

(In reply to comment #7)
> Yes, 'rfpkg clone frobtads' still fails (checkout of the other packages works
> fine, like 'qtads'). -- Thanks

https://pkgs.rpmfusion.org/cgit/?q=frobtads 

All should be fixed: 

rfpkg clone nonfree/frobtads 

works here 

You should got all instruction here: 

http://rpmfusion.org/Contributors#Import_your_package
http://rpmfusion.org/Contributors#Request_a_build
Note , just work with rfpkg >= 1.23.4 doesn't work with 1.23.3

Added -fpermissive C++ build flag yet - required by 32-bit platforms (ix86) and arm.

The build has been successful (branches: master, f25, f24, f23).