Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
Summary: Review Request: theora-exp - Experimental theora decoder Product: Fedora Extras Version: devel Platform: All OS/Version: Linux Status: NEW Severity: normal Priority: normal Component: Package Review AssignedTo: bugzilla-sink@leemhuis.info ReportedBy: j.w.r.degoede@hhs.nl QAContact: fedora-package-review@redhat.com
Spec URL: http://people.atrpms.net/~hdegoede/theora-exp.spec SRPM URL: http://people.atrpms.net/~hdegoede/theora-exp-0.0-0.1.20060730.src.rpm Description: Experimental theora decoder with full support for the complete theora specification (libtheora only supports a subset).
---
Note this can be installed in parallel with libtheora without any problems.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
panemade@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |panemade@gmail.com
------- Additional Comments From panemade@gmail.com 2006-08-01 23:54 EST ------- You missed libtheora-devel to be included in BR. Otherwise package looks ok(SPEC is correct and rpmlint is silent for nondevel RPM) after i add libtheora-devel to BR. Mockbuild then gave me many warnings which are itelsf explanatory about how to remove them.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-08-02 09:18 EST ------- (In reply to comment #1)
You missed libtheora-devel to be included in BR.
I just checked this and you're right, strange.
Mockbuild then gave me many warnings which are itelsf explanatory about how to remove them.
Could you post those warnings here? I don't use mock myself because of bandwidth limitations.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
tibbs@math.uh.edu changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |tibbs@math.uh.edu
------- Additional Comments From tibbs@math.uh.edu 2006-08-18 23:55 EST ------- I didn't see any warnings from mockbuild. I do see a pile of compiler warnings of the form: dump_video.c:98: warning: suggest parentheses around + or - inside shift but I'm sure you see those yourse.f
The only rpmbuild warning is: W: theora-exp-devel no-documentation which is fine. I'll give this a full review soon if nobody else takes it. It may be a couple of days, though.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From tibbs@math.uh.edu 2006-09-08 00:35 EST ------- Now this is failing to build in rboth FC5 and awhide for me:
apiwrapper.c:7:27: error: theora/theora.h: No such file or directory
and various other errors cascading from that one.
I'm not sure what has happened to cause the same package to fail to build.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-09-08 00:39 EST ------- Do you have libtheora-devel installed, or in the case of mock added to the BuildRequires, as discussed in comment 2 that is needed to build and currnetly missing from the BR.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From tibbs@math.uh.edu 2006-09-08 00:44 EST ------- You're right, I must have added that and built a fresh package but I didn't save it. I just pulled down the SRPM in comment 1 to have another look and start on a review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From dmitry@butskoy.name 2006-11-01 10:57 EST ------- Created an attachment (id=139996) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=139996&action=vie...) suggestions for the spec file
- Maybe better to use "svn release" instead of "cvs date" ? - The "man version" seems to be 0.0.1 (atleast according to result library's .so.0.0.1 ) - The "doc/" subdir can be removed from svn source too (it can significantly decrease the size of srpm :) ) - Maybe enable encoding support too? And how about tools/ subdir? - Maybe include "examples/" dir into %doc for devel subpackage?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-11-07 16:38 EST ------- (In reply to comment #7)
Created an attachment (id=139996)
--> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=139996&action=vie...) [edit]
suggestions for the spec file
- Maybe better to use "svn release" instead of "cvs date" ?
- The "man version" seems to be 0.0.1 (atleast according to result library's
.so.0.0.1 )
- The "doc/" subdir can be removed from svn source too (it can significantly
decrease the size of srpm :) )
All good ideas, thus I've applied your patch, thanks!
- Maybe enable encoding support too?
No lets not the docs clearly state that this is experimental, so that is an experimental part of an experimental version, bad idea! Also the docs state that there are no guarantees files created with the encoder will keep working with newer theora versions!
- And how about tools/ subdir?
Those don't look very usefull for generic purposes
- Maybe include "examples/" dir into %doc for devel subpackage?
This is commonly not done unless upstream really intends for the examples to get installed, iow "make install" installs them.
A new version with the suggested improvements and updated to a newer snapshot it available here: Spec File: http://people.atrpms.net/~hdegoede/theora-exp.spec SRPM File: http://people.atrpms.net/~hdegoede/theora-exp-0.0.1-0.1.svn12061.src.rpm
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
dmitry@butskoy.name changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED OtherBugsDependingO|163776 |163778 nThis| |
------- Additional Comments From dmitry@butskoy.name 2006-11-09 10:06 EST ------- * the "-devel" package owns "/usr/include/theora" directory, which is questionable. It seems that libtheora-devel should own this, but currently this dir is not owned at all. Anyway, IMO, an Extras package should not own anything already present in the Core...
* "-devel" package must "Requires: pkgconfig" as it has .pc file
* Suggestion: add some words to %description that "theora" is related to "video codec" etc. It could help newbies a little... :)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
dmitry@butskoy.name changed:
What |Removed |Added ---------------------------------------------------------------------------- AssignedTo|bugzilla-sink@leemhuis.info |dmitry@butskoy.name
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Attachment #139996|0 |1 is obsolete| |
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-11-09 14:33 EST ------- Created an attachment (id=140813) --> (https://bugzilla.redhat.com/bugzilla/attachment.cgi?id=140813&action=vie...) Updated specfile
(In reply to comment #9)
- the "-devel" package owns "/usr/include/theora" directory, which is
questionable.
It seems that libtheora-devel should own this, but currently this dir is not owned at all. Anyway, IMO, an Extras package should not own anything already present in the Core...
Since theora-exp-devel and theora-exp themselves can be installed without having libtheora(-devel) installed, I believe that theora-exp-devel is correct in owning this dir, otherwise it would be unowned when libtheora(-devel) isn't installed. The fact that libtheora-devel doesn't own it is a bug. Will you file this, or shall I?
- "-devel" package must "Requires: pkgconfig" as it has .pc file
You're correct there.
- Suggestion: add some words to %description that "theora" is related to
"video
codec" etc. It could help newbies a little... :)
Good idea! I've attached a new specfile, as for some reason I cannot access people.atrpms.net ATM.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From dmitry@butskoy.name 2006-11-10 09:50 EST -------
The fact that libtheora-devel doesn't own it is a bug.
Yep.
Will you file this, or shall I?
Better you... ;)
I think it would be better to not own /usr/include/theora/ dir by this package. First, in hope that libtheora-devel will fix this issue soon (and after the fixing no any changes for theora-exp-devel's %files section will be required). Second, let's consider such Core's package behaviour as a precedent, which allows us to do the same (at least for a while). Otherwise this issue is a blocker, which prevents the including of theora-exp until libtheora-devel will be fixed, which might require unpredictable amount of time.
In other parts, new .spec file is OK.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-11-10 10:34 EST ------- (In reply to comment #11)
I think it would be better to not own /usr/include/theora/ dir by this package. First, in hope that libtheora-devel will fix this issue soon (and after the fixing no any changes for theora-exp-devel's %files section will be required). Second, let's consider such Core's package behaviour as a precedent, which allows us to do the same (at least for a while). Otherwise this issue is a blocker, which prevents the including of theora-exp until libtheora-devel will be fixed, which might require unpredictable amount of time.
I don't see how this is a blocker, independent of libtheora-devel being fixed this package must still own /usr/include/theora, because quoting from the review guidelines: "MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory."
And since this package does not and will not Require libtheora-devel, because it simply doesn't need it, it thus MUST own that directory. And having multiple owners for directories, although undesirable is not a blocker.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
dmitry@butskoy.name changed:
What |Removed |Added ---------------------------------------------------------------------------- OtherBugsDependingO|163778 |163779 nThis| |
------- Additional Comments From dmitry@butskoy.name 2006-11-10 11:09 EST -------
And having multiple owners for directories, although undesirable is not a blocker.
Oh, yes, I just forgot about this recent policy change. :)
rpmlint OK Must/should items OK
APPROVED!
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
------- Additional Comments From j.w.r.degoede@hhs.nl 2006-11-10 12:21 EST ------- Thanks, imported and build, closing.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=200666
j.w.r.degoede@hhs.nl changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |NEXTRELEASE
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug report.
Summary: Review Request: theora-exp - Experimental theora decoder
https://bugzilla.redhat.com/show_bug.cgi?id=200666
bugzilla@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |medium Priority|normal |medium Product|Fedora Extras |Fedora Version|devel |rawhide
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla