Bug 221405
| Summary: | Review Request: dirac - Dirac is an open source video codec | ||
|---|---|---|---|
| Product: | [Fedora] Fedora | Reporter: | Nicolas Chauvet (kwizart) <kwizart> |
| Component: | Package Review | Assignee: | Nobody's working on this, feel free to take it <nobody> |
| Status: | CLOSED NEXTRELEASE | QA Contact: | Fedora Package Reviews List <fedora-package-review> |
| Severity: | medium | Docs Contact: | |
| Priority: | medium | ||
| Version: | rawhide | CC: | mgarski |
| Target Milestone: | --- | Flags: | kwizart:
fedora-review+
kevin: fedora-cvs+ |
| Target Release: | --- | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Whiteboard: | |||
| Fixed In Version: | Doc Type: | Bug Fix | |
| Doc Text: | Story Points: | --- | |
| Clone Of: | Environment: | ||
| Last Closed: | 2007-03-26 07:41:07 UTC | Type: | --- |
| Regression: | --- | Mount Type: | --- |
| Documentation: | --- | CRM: | |
| Verified Versions: | Category: | --- | |
| oVirt Team: | --- | RHEL 7.3 requirements from Atomic Host: | |
| Cloudforms Team: | --- | Target Upstream Version: | |
| Embargoed: | |||
| Bug Depends On: | |||
| Bug Blocks: | 163779 | ||
|
Description
Nicolas Chauvet (kwizart)
2007-01-04 15:13:20 UTC
I'm not a sponsor, but I can do a non-binding review. I'm not a sponsor, but I can do a non-binding review. Some initial remarks: - Don't build the static libs (Append --disable-static to %configure). - Don't run the autotools as part of building. Autotool-generated files should be part of the source code. (Running the autotools as part of building exposes a package to subtile incompatiblities between different versions of the autotools. In fact, this package really suffers from some (however minor) issues). - The package doesn't honour RPM_OPT_FLAGS correctly: It plays dirty games on CFLAGS/CXXFLAGS (E.g. it must not append -O3 and -mmx). - The *-devel subpackage ships a *.pc. Therefore it must Requires: pkgconfig The release 3 will come soon... 1- OK for now (but if a program need to link with the static version?) (.la and .a removed from the package) 2- KO - Don't know how to do since ./configure will not be found. 3- OK 4- OK (5)- I've also add some BR (mock tries!) (6)- doxygen -u when required! I wonder these message: (install step) libtool: install: warning: remember to run `libtool --finish /usr/lib64' Should i do it do it myself ? (libtool is now in BR!) Release 3: Spec URL: http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.spec SRPM URL: http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-3.cvs20070104.kwizart.fc6.src.rpm Description: Dirac is an open source video codec Build Log: http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.log 2 - OK understood, the bootstrap in included in the diract-snapshoot.sh (but should it use direct url to download the package with next snapshoot? for exemple on my website? - *-3 still doesn't honour RPM_OPT_FLAGS and still plays dirty games with CFLAGS/CXXFLAGS. /me thinks you misunderstood my earlier remark. The problem is not "make RPM_OPT_FLAGS="$RPM_OPT_FLAGS"" (Which doen't make any sense), but the configure script mis-handling CFLAGS/CXXFLAGS and appending compiler flags to them it MUST not add to them, such as "-O3" and "-mmmx". - Why the dependency on valgrind? AFAIS, this package only uses valgrind in some part of its testsuite, but not as part of building. I.e. this dependency on valgrind only makes sense if your are running the testsuite as part of building. - Instead of directly using a cvs snapshot, you'd better base your package on a released version and apply a patch if you really think shipping a bleeding edge version is useful (I don't know, I am not familiar with libdirac's stability). - Finally, the "autotool" correct way to generate a cvs snapshot-tarball would be to run cvs co && autoreconf -fi && ./configure && make dist -1 (old 3) Don't know much about compile flag but since diract in a codec,
compiling it with -mmx seems not too much!? (searching for aims of -03...) I
expect it deals with i586/i686 ? Since it is an encoder and requires hight
performances (to encode and certainly also to decode), i wonder if it is not
better to stay with it?
Tryed do do the changes but this seems not to work (searching...wip):
%configure --enable-overlay --disable-static CXXFLAGS="${RPM_OPT_FLAGS}"
CFLAGS="${RPM_OPT_FLAGS}"
2 OK (commented...)
3 The project is now involved in the hardware implementation. Encode/Decode
Software "should" work! There is some enancement to wait
http://dirac.sourceforge.net/todo.html! ... I have browsed the CVS and the last
commit was 4 weeks ago. (the last release is 0.6.0 and was out 6 months ago)
I cannot see a better time to make a snapshot ? what do you feel?
4 OK (added to diract-snapshot and updated to 20070105)
(In reply to comment #7) > -1 (old 3) Don't know much about compile flag but since diract in a codec, > compiling it with -mmx seems not too much!? Nicolas the problem lays in configure.ac file, which is used to generate configure and later Makefile. If you view this file and jump to "enable debug flags" and "enable MMX optimization flag" section, you will notice that the first one sets -O3 and the second -mmmx. Probably these sections needs to be clean up, then you have to do autoreconf to generate updated configuration files. SPEC : http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac.spec SRPMS: http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-5.cvs20070108.kwizart.fc6.src.rpm Description: Dirac is an open source video codec Build Log: (2> .log) http://kwizart.free.fr/fedora/6/testing/libdirac/libdirac-0.6.0-5.log It seems that using configure can disable theses unwanted FLAGs without autoreconf. I think it is a better solution. I wonder why the src.rpm has grown up. Since i was using make tag, the produced release dirac-0.6.0.tar.gz was bundled with the sources but no used. The only thing that wasn't bundled in this version was the qt4 gui encoder. But anyway it can be renabled later... I should have : (but since it is not release...) provides: dirac-qt4 obsolete: dirac-qt4 < %{version}-5.cvs20070108 The only rpmlint error i have is : E: libdirac configure-without-libdir-spec A configure script is run without specifying the libdir. configure options must be augmented with something like --libdir=%{_libdir}. But the %configure script bundle it so i wonder if it is not a problem with the ./configure in the snapshot step Per http://fedoraproject.org/wiki/Packaging/NamingGuidelines#head-cfd71146dbb6f00cec9fe3623ea619f843394837 the release field should be: Release: 5.%{cvs}cvs%{?dist} Why is the testsuite disabled? I don't like /usr/bin/ binaries in a package called libwhatever. Moreover, if the source tarball is called dirac, why is the main package called libdirac? Please put the binaries in a package named dirac There are two redundant BuildRequires: libtool requires automake and automake requires autoconf. Builds fine in mock/x86_64/devel. Shouldn't package docs be in %{_datadir}/doc/dirac-docs-%{version} ? SRPM: http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-6.20070108cvs.kwizart.fc6.src.rpm SPEC: http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec Description: Dirac is an open source video codec Test-suite is not provided with this snapshoot scheme (done from remark in #6 ) same problem with qt4-encodeur gui (anyway i'm not sure we need an encoder gui for each codec !?). if encoder_gui and test-suite are really needed the best way to do this would be to make another snaphoot and a tarball with only theses two wanted parts! Encoding seems to work fine but isn't useable since playbak need to be tested... Looking good now.
Small issue: it is preferable to ship the desktop file separately instead of
creating it in %install.
These files have names that are IMHO too generic. Maybe rename them to dirac_*?
%{_bindir}/BMPtoRGB
%{_bindir}/RGBtoBMP
%{_bindir}/RGBtoUYVY
%{_bindir}/RGBtoYUV*
%{_bindir}/UYVYtoRGB
%{_bindir}/YUV*toRGB
(Removing NEEDSPONSOR by bug 225119) 1. package mostly meets naming and packaging guidelines.
-> See comment #12.
-> You could leave MMX optimizations enabled on x86_64 (assuming they work)
2. specfile is properly named, is mostly cleanly written and uses macros
consistently.
-> specfile contains a lot of commented-out code, please remove those parts
3. dist tag is present.
4. build root is sane.
5. license field matches the actual license.
6. license is open source-compatible (MPL). License text included in package.
7. source files match upstream.
Unable to verify md5sum because it's a snapshot, but diff -Nru between
packaged tree and one checked out locally using the included script
shows no differences.
8. latest version is not being packaged, but I think it should be left
at packager's discretion, as he should know which snapshot is stable.
9. BuildRequires are proper.
10. package builds in mock (fc6/x86_64).
11. rpmlint is not silent, but these can be ignored:
W: dirac-devel no-documentation
E: dirac configure-without-libdir-spec
12. final provides and requires are sane:
dirac-0.6.0-6.20070108cvs.fc6.x86_64.rpm
libdirac_decoder.so.0()(64bit)
libdirac_encoder.so.0()(64bit)
dirac = 0.6.0-6.20070108cvs.fc6
=
/sbin/ldconfig
/usr/bin/perl
libc.so.6()(64bit)
libdirac_decoder.so.0()(64bit)
libdirac_encoder.so.0()(64bit)
libgcc_s.so.1()(64bit)
libm.so.6()(64bit)
libstdc++.so.6()(64bit)
perl(File::Basename)
perl(Getopt::Long)
perl(strict)
dirac-devel-0.6.0-6.20070108cvs.fc6.x86_64.rpm
dirac-devel = 0.6.0-6.20070108cvs.fc6
=
dirac = 0.6.0-6.20070108cvs.fc6
libdirac_decoder.so.0()(64bit)
libdirac_encoder.so.0()(64bit)
pkgconfig
13. shared libraries are present and handled properly
14. package is not relocatable.
15. owns the directories it creates.
16. doesn't own any directories it shouldn't.
17. no duplicates in %files.
18. file permissions are appropriate.
19. %clean is present.
20. test suite not included with snapshot, hence no %check
21. proper %post/%postun scriptlets present.
22. code, not content.
23. documentation is not small, -docs subpackage present.
24. %docs are not necessary for the proper functioning of the package.
25. headers in -devel
26. pkgconfig file in -devel, pkgconfig in Requires:
27. no libtool .la droppings.
28. GUI is not built/shipped, hence no .desktop file is necessary.
29. not a web app.
NEEDSWORK: 1, 2.
SRPM: http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-7.20070108cvs.kwizart.fc6.src.rpm SPEC: http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec Description: Dirac is an open source video codec Fixed 1 and 2 I've tryed to test in a same directory a Test_Video.yuv in YUV (analogic source from tv tuner), encoding it to drc (Test1.drc) and then decoding it to yuv (Test1.yuv) I didn't get then resulting Test1.yuv playing. I was using dirac_encoder and dirac_decoder tools from theses packages... I will try to do more tests... The videolan project seems not to support dirac 0.6 (prefered version seems to be 5.4 ). They are trying to support it throught ffmpeg. (discution on irc) Ekhm...
%ifarch x86_64 ppc64 sparc64 \
--enable-mmx=yes \
%else \
--enable-mmx=no \
%endif
MMX should be enabled unconditionally for x86_64 ONLY.
SRPM: http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-8.20070108cvs.kwizart.fc6.src.rpm SPEC: http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec Description: Dirac is an open source video codec Fixed yes actually i didn't know it was only for x86 intel or amd. i've only know that some via cpu didn't use mmx so that it needs to be removed for x86. New rpmlint errors: E: dirac-libs library-without-ldconfig-postin /usr/lib64/libdirac_encoder.so.0.0.0 E: dirac-libs library-without-ldconfig-postun /usr/lib64/libdirac_encoder.so.0.0.0 E: dirac-libs library-without-ldconfig-postin /usr/lib64/libdirac_decoder.so.0.0.0 E: dirac-libs library-without-ldconfig-postun /usr/lib64/libdirac_decoder.so.0.0.0 W: dirac mixed-use-of-spaces-and-tabs (spaces: line 3, tab: line 73) Looks like you forgot to move ldconfig calls to -libs, i.e.: ... %post libs -p /sbin/ldconfig %postun libs -p /sbin/ldconfig (I hit send too quickly) SRPM: http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-8.20070108cvs.kwizart.fc6.src.rpm SPEC: http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec Description: Dirac is an open source video codec Yes just saw that too - This is fixed with release 8 SRPM: http://kwizart.free.fr/fedora/6/testing/dirac/dirac-0.6.0-9.20070325cvs.kwizart.fc7.src.rpm SPEC: http://kwizart.free.fr/fedora/6/testing/dirac/dirac.spec Description: Dirac is an open source video codec Build fine in mock for fc7 logs: http://kwizart.free.fr/fedora/6/testing/dirac/build.log http://kwizart.free.fr/fedora/6/testing/dirac/root.log http://kwizart.free.fr/fedora/6/testing/dirac/mockconfig.log Looks fine. This package is APPROVED. New Package CVS Request ======================= Package Name: dirac Short Description: Dirac is an open source video codec Owners: kwizart Branches: FC-5 FC-6 devel InitialCC: branched Package Change Request ====================== Package Name: dirac New Branches: EL-4 EL-5 I'v made some mistake with request Flags here! cvs done. |