Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 200666 - Review Request: theora-exp - Experimental theora decoder
Summary: Review Request: theora-exp - Experimental theora decoder
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Dmitry Butskoy
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-07-30 10:07 UTC by Hans de Goede
Modified: 2007-11-30 22:11 UTC (History)
4 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-11-10 17:25:53 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)
suggestions for the spec file (deleted)
2006-11-01 15:57 UTC, Dmitry Butskoy
no flags Details | Diff
Updated specfile (deleted)
2006-11-09 19:33 UTC, Hans de Goede
no flags Details

Description Hans de Goede 2006-07-30 10:07:09 UTC
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.

Comment 1 Parag AN(पराग) 2006-08-02 03:54:57 UTC
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.

Comment 2 Hans de Goede 2006-08-02 13:18:15 UTC
(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.


Comment 3 Jason Tibbitts 2006-08-19 03:55:29 UTC
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.

Comment 4 Jason Tibbitts 2006-09-08 04:35:32 UTC
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.

Comment 5 Hans de Goede 2006-09-08 04:39:48 UTC
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.


Comment 6 Jason Tibbitts 2006-09-08 04:44:59 UTC
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.

Comment 7 Dmitry Butskoy 2006-11-01 15:57:23 UTC
Created attachment 139996 [details]
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?

Comment 8 Hans de Goede 2006-11-07 21:38:46 UTC
(In reply to comment #7)
> Created an attachment (id=139996) [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

Comment 9 Dmitry Butskoy 2006-11-09 15:06:42 UTC
* 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... :)

Comment 10 Hans de Goede 2006-11-09 19:33:51 UTC
Created attachment 140813 [details]
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.

Comment 11 Dmitry Butskoy 2006-11-10 14:50:06 UTC
> 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.


Comment 12 Hans de Goede 2006-11-10 15:34:12 UTC
(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.




Comment 13 Dmitry Butskoy 2006-11-10 16:09:43 UTC
> 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!




Comment 14 Hans de Goede 2006-11-10 17:21:57 UTC
Thanks, imported and build, closing.



Note You need to log in before you can comment on or make changes to this bug.