[Bug 1175023] Review Request: oggify - audio conversion tool for music library conversion

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

 



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



--- Comment #20 from Gerald Cox <gbcox@xxxxxx> ---
(In reply to Jerry James from comment #19)
> Issues, in no particular order:
> 
> - I am very uncomfortable with the way the source is handled.  I see the
>   comments above about this issue, but I still don't understand why there is
> a
>   problem with the official upstream URL:
> 
>   https://github.com/spr/Oggify/archive/v2.0.7.tar.gz
> 
>   What is the problem with using that?

Jerry, I'm open to suggestions.  The problem is that the way github packages
the archives.  It doesn't populate the submodules (in this case tag_wrapper). 
You have to manually:  submodule init and update to get it included, then
re-create the archive.  It's a known issue, people have been complaining, but
it still isn't fixed.  I searched and searched and the only way I could find to
do it was to copy an approach used by another Fedora package (comment #5).  I
don't particularly like it either, it's kind of a pain.  You'd think that 
there would be a more elegant way to handle, but I haven't been able to find
it.   

> 
> - Add "%dir %{python2_sitelib}/%{name}/plugins" to %files
OK, no problem.

> 
> - The plugins invoke %{_bindir}/nice, so "Requires: coreutils" is technically
>   needed, although possibly a bit silly.
Yes, encountered that when I build for COPR.

> 
> - Neither the %clean script nor the %defattr in %files is necessary.  Please
>   remove both (unless you plan to build for EPEL5, in which case you will
> have
>   to make a few other changes to the spec file, anyway).
Thanks, I will remove.

> 
> - Consider adding "%doc README.md" to %files.  I think it contains some
> useful
>   information.
Will do.

> 
> - I notice that python-mutagen is a BuildRequires, but not a Requires.  Is
>   that intentional?
On a previous review, I was told if something was listed in BuildRequires, that
the Requires are automatically provided, and to not double-list.  Is that not
correct?

> 
> - There is no %check script.  Is there a simple test you could perform just
> to
>   verify basic functionality is working?
> 
> 
I'll look into that and provide if I can find something.

Thanks for your input.  I'll make the changes listed above.

-- 
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
_______________________________________________
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]