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.
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.
(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.
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.
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.
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.
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.
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?
(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
* 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... :)
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.
> 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.
(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.
> 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!
Thanks, imported and build, closing.