Package Review for graphical diff tool xxdiff http://xfs.org/~cattelan/xxdiff-3.2-1.fc9.src.rpm http://xfs.org/~cattelan/xxdiff.spec
From just a quick look; minor rpmlint warnings: [mockbuilder@neon tmp]$ rpmlint /var/lib/mock/fedora-development-x86_64/result/*.rpm xxdiff.src: W: non-standard-group Application/Text xxdiff.x86_64: W: non-standard-group Application/Text Koji scratch build completed: http://koji.fedoraproject.org/koji/taskinfo?taskID=504451
You could simply use pkg-config, instead of sourcing a shell rc file: export QTDIR=%(pkg-config --variable=prefix qt-mt) export QTLIB=%(pkg-config --variable=libdir qt-mt) export QTINC=%(pkg-config --variable=includedir qt-mt) Did you consider also packaging xxdiff-tools?
The tools sounds like a good idea I'll do that. I'll try your suggestion for the QT vars thanks
Ok finally got the tools packaged. http://xfs.org/~cattelan/xxdiff-3.2-1.fc9.src.rpm Note the pkg-config did not work on all builds so when back to using the /etc/profile.d/qt.sh
I assume you meant http://xfs.org/~cattelan/xxdiff-3.2-2.fc9.src.rpm :) Mind that a bug should be moved to ASSIGNED state when someone actually claims the bug for review. Here are a couple of things I've noticed after examining the scratch build from http://koji.fedoraproject.org/koji/taskinfo?taskID=537125 and your spec: minor: python-devel will pull in python, so there is no need to BR python too. important: Please replace the occurrence of $RPM_BUILD_ROOT with %{buildroot} (you've used the last one 3 times and the packaging policy mandates using one or another but not both in the same spec) important: your %build step does not use the RPM_OPT flags mandated by fedora medium: rpmlint has complains about your src.rpm: W: non-standard-group Application/Text
Ok made the requested changes. Not sure I got the rpm_opts stuff right tho. Let me know. Thanks.
Would you mind posting the new spec and src.rpm, please ? ( Each modification should be followed by a new release, i.e. bump the release field and create a new src.rpm)
Sorry my bad. The files have been updated.
Ok, I can do the review. Some remarks: - you probably should move the xxdiff-tools to a subpackage, because they require python, whereas xxdiff itself does not - please use the standard %python_sitelib macro definition, see http://fedoraproject.org/wiki/Packaging/Python - build the python files in %build %{__python} setup.py build - no need to record INSTALLED_FILES. something like %{__python} setup.py install -O1 --skip-build --root %{buildroot} should do - consider creating an egg-info for f7 and f8, see http://fedoraproject.org/wiki/Packaging/Python/Eggs#head-4ac98208bf2f5a13b9cd997c91e2e424f67a7e35 - instead of "cd src; make ...; cd -", use "make -C src ..." - clean the buildroot at the beginning of %install - license is "GPLv2+", no? - please remove the shebang lines from the .py files in %python_sitelib, using 'find .. -exec %{__sed} -i "1{/^#!/d}" {} \;' or similar, to make rpmlint quiet - qt4-devel doesn't provide qt-devel, so (imho) no need for a versioned BR on qt-devel Is there a reason why this review request is non-public?
(In reply to comment #9) > - consider creating an egg-info for f7 and f8 I'll take this one back. As it turns out, eggs are entirely optional for f7 and f8, unless no other package requires them.
Ping?
Sorry I've been out of town and busy with some other things the last few weeks. I have an updated spec file but am still unclear about a few things. specifically the removal of the "shebang lines" not sure why that needs to be done?
(In reply to comment #12) > specifically the removal of the "shebang lines" not sure why that needs > to be done? A text file meant to be sourced and not executed simply shouldn't have a #! in the first line. Seems common though for python files, and should be fixed upstream. Meanwhile, a simple cmd like the one I gave earlier fixes this for the package and keeps rpmlint quiet.
Ok finally got the magic find worked out to remove the #! Turns out svnforeign.py turns into svn-foreign so can't just process *.py. Anyways updated xxdiff.spec and src.rpm http://xfs.org/~cattelan/
Some problems are still left. - CFLAGS/CFLAGS: The standard flags defined by the rpmbuild environment are not properly passed to the build process. As far as I see, this can't be done from the specfile, but needs patching the makefiles (sth. like passing additional QMAKE_CXXFLAGS and QMAKE_CFLAGS to qmake when bootstrapping the main makefile). This is a blocker issue imho. (If time permits I'll try to come up with a patch.) - Requirements: The main xxdiff packages should'nt depend on python. - Why don't you install the tool scripts into %python_sitelib (the commented out lines)? The tools package is architecture independent.
I think the CFLAGS are correctly set now (with the help of some sed commands) I'm not sure CXXFLAGS should be set the same as CFLAGS? Removed python from xxdiff requires (my bad was on the list of things to fix). The python_sitelib stuff was driving me crazy, it was always coming up as /usr/lib/etc even on the 64 bit platform. The missing bit of info not on the wiki was the 1 argument to the get_python_lib() call. (found example in the firstboot package) Updated spec and src rpms on xfs.org
(In reply to comment #16) > I think the CFLAGS are correctly set now (with the help of some sed commands) Seems to work. If upstream is still active, it would be good to ask them to support passing additional compilation flags. See also http://fedoraproject.org/wiki/PackagingDrafts/PatchUpstreamStatus, which meanwhile got accepted, if I understand correctly. > Removed python from xxdiff requires (my bad was on the list of things to fix). Ok. Please also remove "Requires: python" from the -tools subpackage; rpm will bring in python(abi) and /usr/bin/python requirements automagically (tested). Furthermore, as fc7, f8 and f9 all have qt >= 3.3, you can drop the "qt >= 3.2" requirement. rpm automatically adds libqt-mt.so.3 as a requirement. > The python_sitelib stuff was driving me crazy, it was always coming up as > /usr/lib/etc even on the 64 bit platform. But this is intentional, and exactly the difference between %python_sitearch and %python_sitelib. As xxdiff-tools only contains architecture-independent scripts (no compiled stuff), %python_sitelib (i.e. /usr/lib) is just fine, please use that. See http://fedoraproject.org/wiki/Packaging/Python#head-875cc97c2232a5b3ceda75ea41eed525da7d3929 . There's another thing I totally forgot, and which I should have mentioned earlier (sorry about that): we need a .desktop file as xxdiff is an application with a gui. It's not that complicated, see #426611#c4 for an example.
ok finally got around to adding the icon to the desktop ... I was hoping for some input from the author as to what would be a good icon, but no response. FWIW I don't really think of this as desktop app so it seems silly to have it show up in a menu, but whatever. -Russell http://xfs.org/~cattelan/xxdiff-3.2-6.fc9.src.rpm
One more note xxdiff does show up in the app menu now but it won't launch since it requires at least 2 files as arguments.
> One more note xxdiff does show up in the app menu now but it won't launch since > it requires at least 2 files as arguments. heh, awesome ;) Reviewers, can we just drop the desktop entry / icon requirements... this isn't really helping, and isn't the way the app was intended to be used, based on the fact that it won't even start this way. :)
My gtkwave package is a GUI application with no desktop entry for the same reason - it requires at least one file argument. I just added a comment in the spec file about why there was no desktop entry and that was OK. See Bug #172579.
Well. If you add %F to the Exec line of the .desktop file, xxdiff can be started dropping two files on its icon. I've tested that and it works. So, I'd vote for keeping the desktop file. (Note that in the specfile, % has to be quoted as %%). Russel, please remove the "Requires: qt' line, it's not necessary as far as I can tell. rpmlint has a minor complaint: xxdiff.spec: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 1).
Finally, can someone please comment on the copyright notice at the bottom of /usr/share/doc/xxdiff-3.2/doc/screenshots/allindex.html (and other files in that directory). It says: All images and material are Copyright © 2001 Martin Blais, Montreal, Canada. All Rights Reserved. Images may not be used without written permission. If you would like to license or use an image, please contact blais Does that cause any problems for us?
> Well. If you add %F to the Exec line of the .desktop file, xxdiff can be started > dropping two files on its icon. I've tested that and it works. So, I'd vote for > keeping the desktop file. But really, truly and honestly, this is not how the application will be used 99.99% of the time. Really. :) And if you don't drop files on it is there any warning, or does it just fail to launch?
I have a hard time believing Requires: qt isn't needed? localhost[12:33pm]-=>ldd /usr/bin/xxdiff linux-gate.so.1 => (0x00110000) libqt-mt.so.3 => /usr/lib/qt-3.3/lib/libqt-mt.so.3 (0x057af000) . . . localhost[12:35pm]-=>rpm -qf /usr/lib/qt-3.3/lib/libqt-mt.so.3 qt-3.3.8b-2.fc8 as far as the desktop stuff goes I don't like the idea of providing a menu option that does not work unless you know the secret or dropping 2 files on it. And as Eric points out probably nobody is ever going to use xxdiff via some desktop point click interface. -Russell
(In reply to comment #25) > I have a hard time believing Requires: qt isn't needed? > > % ldd /usr/bin/xxdiff > libqt-mt.so.3 => /usr/lib/qt-3.3/lib/libqt-mt.so.3 (0x057af000) > > % rpm -qf /usr/lib/qt-3.3/lib/libqt-mt.so.3 > qt-3.3.8b-2.fc8 Exactly! Here is why you don't need to (should not) specify this requirement in the specfile: rpmbuild looks at the xxdiff binary (like you did), also detects that it (amongst others) requires libqt-mt.so.3, and adds that automatically as a dependency to the package. And yum or similar tools in turn will resolve that dependency on libqt-mt.so.3 with the qt (or qt3) package. It is generally better to have a dependency on the library than on some package name (and note that the package is named qt3 on fc9+). Try "rpm -q --provides xxdiff" and "rpm -q --whatprovides libqt-mt.so.3", or "yum install libqt-mt.so.3", if you don't believe me ;) > as far as the desktop stuff goes I don't like the idea of providing a menu > option that does not work unless you know the secret or dropping 2 files > on it. > > And as Eric points out probably nobody is ever going to use xxdiff via some > desktop point click interface. Ok, ok. You convinced me ;) You might want to add a comment, though, as suggested by Paul.
(In reply to comment #26) > Try "rpm -q --provides xxdiff" and "rpm -q --whatprovides libqt-mt.so.3", or > "yum install libqt-mt.so.3", if you don't believe me ;) "rpm -q --requires xxdiff", of course.
Ah right... Eric reminded me about library auto depends. dropped the desktop stuff added change log entry stating why desktop entry should not be there dropped the Requires: qt fixed the spaces vs tab. I sent an email to the author about the copyright issue. Version 7 of the src rpm is on xfs.org
(In reply to comment #23) > Finally, can someone please comment on the copyright notice at the bottom of > /usr/share/doc/xxdiff-3.2/doc/screenshots/allindex.html (and other files in that > directory). It says: > > All images and material are Copyright © 2001 Martin Blais, Montreal, Canada. > All Rights Reserved. Images may not be used without written permission. > If you would like to license or use an image, please contact blais > > Does that cause any problems for us? Yes. Either get written permission to freely redistribute and modify or pull out these docs/images.
Do we need to pull them from the original tarball? so that we are not shipping the images as part of the source RPM. Or can we simply exclude them from the binary RPM? note: I have not received a response from the author.
Yes, that would be the best approach here, to clean them from the tarball. Just add comments to the spec file explaining how you get from the original tarball to the clean tarball (or write a little script if you're feeling ambitious).
Well still no response from author, I guess it's time to simply remove the screenshots from tarball. http://xfs.org/~cattelan/xxdiff-3.2-8.fc9.src.rpm http://xfs.org/~cattelan/xxdiff.spec
[x],[~] = ok, [!] = problem, [-] = not applicable [x] package meets naming guidelines [x] specfile is encoded in ascii or utf-8 [x] specfile matches base package name [~] specfile uses macros consistently could use %{name} in some more places [~] specfile is written cleanly could have some more explaining comments, e.g. - upstream status for patch? - removal of shebang lines [x] specfile is written in AE [x] changelog is present and has correct format [x] license matches actual license [x] license is open source-compatible [x] license text is included in package [~] source tag has correct url ok, has been discussed in the review [~] source files match upstream ok, has been discussed [x] latest version is packaged [x] summary is concise [x] dist tag is present [x] buildroot is correct [x] buildroot is prepped [x] %clean is present [x] proper build requirements [x] proper requirements [x] uses %{?_smp_mflags} [x] uses %{optflags} [x] doesn't use %makeinstall [x] package builds at least on one architecture tested on: fedora-9-x86_64 [x] packages installs and runs at least on one architecture tested on: fedora-9-x86_64 [x] rpmlint is quiet [x] final provides/requires look sane [-] ldconfig called in %post and %postun if required [x] code, not content [x] file permissions are appropriate [x] debuginfo package looks usable [-] config files marked as %config(noreplace) [x] owns all the directories it creates [-] static libraries in -devel subpackage [-] header files in -devel subpackage [-] development .so files in -devel subpackage [-] pkgconfig files in -devel subpackage, requires pkgconfig [-] no .la files [-] doesn't need a -docs subpackage [x] relevant docs are included [x] doc files are not needed at runtime [~] provides a .desktop file, build-requires desktop-file-utils ok, has been discussed in the review [-] uses %find_lang, build-requires gettext APPROVED.
Forgot to update the fedora-review flag.
New Package CVS Request ======================= Package Name: xxdiff Short Description: Graphical file/directory viewer Owners: cattelan Branches: F-8 F-9 InitialCC: Cvsextras Commits: yes
cvs done. I assume the legal blocker is solved with those removals... Spot: Can you confirm?
Yes, he took out the screenshots directory and is using a clean tarball. Lifting FE-Legal.
Russell, please don't forget to close this bug when the package finally hit the stable repositories. Note that bodhi could have done that for you if you told it this bug's number.
https://admin.fedoraproject.org/updates/F9/FEDORA-2008-5639