Bug 436677 - Review Request: xxdiff
Review Request: xxdiff
Status: CLOSED CURRENTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity low
: ---
: ---
Assigned To: Thomas Moschny
Fedora Extras Quality Assurance
: Reopened
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-09 01:10 EST by Russell Cattelan
Modified: 2008-08-07 05:27 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-08-07 05:27:14 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
thomas.moschny: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Russell Cattelan 2008-03-09 01:10:10 EST
Package Review for graphical diff tool xxdiff

http://xfs.org/~cattelan/xxdiff-3.2-1.fc9.src.rpm
http://xfs.org/~cattelan/xxdiff.spec
Comment 1 Eric Sandeen 2008-03-09 12:59:35 EDT
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


Comment 2 Thomas Moschny 2008-03-11 06:30:25 EDT
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?
Comment 3 Russell Cattelan 2008-03-17 13:34:06 EDT
The tools sounds like a good idea I'll do that.

I'll try your suggestion for the QT vars

thanks
Comment 4 Russell Cattelan 2008-03-28 21:56:59 EDT
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

Comment 5 manuel wolfshant 2008-03-28 22:43:00 EDT
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
Comment 6 Russell Cattelan 2008-03-29 18:17:49 EDT
Ok made the requested changes.

Not sure I got the rpm_opts stuff right tho.
Let me know.

Thanks.
Comment 7 manuel wolfshant 2008-03-30 21:17:19 EDT
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)
Comment 8 Russell Cattelan 2008-04-01 20:09:42 EDT
Sorry my bad.

The files have been updated.

Comment 9 Thomas Moschny 2008-04-05 05:28:22 EDT
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?
Comment 10 Thomas Moschny 2008-04-15 13:19:08 EDT
(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.
Comment 11 Thomas Moschny 2008-04-23 05:04:14 EDT
Ping?
Comment 12 Russell Cattelan 2008-04-23 17:46:00 EDT
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?


Comment 13 Thomas Moschny 2008-04-23 18:55:39 EDT
(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.
Comment 14 Russell Cattelan 2008-04-28 15:14:42 EDT
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/
Comment 15 Thomas Moschny 2008-05-06 04:31:23 EDT
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.
Comment 16 Russell Cattelan 2008-05-06 16:58:26 EDT
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
Comment 17 Thomas Moschny 2008-05-10 16:01:15 EDT
(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.
Comment 18 Russell Cattelan 2008-05-28 17:40:22 EDT
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
Comment 19 Russell Cattelan 2008-05-28 21:23:34 EDT
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.

Comment 20 Eric Sandeen 2008-05-28 22:25:12 EDT
> 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.  :)
Comment 21 Paul Howarth 2008-05-29 05:43:18 EDT
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.
Comment 22 Thomas Moschny 2008-05-29 05:51:52 EDT
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).
Comment 23 Thomas Moschny 2008-05-29 05:57:03 EDT
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@furius.ca 

Does that cause any problems for us?
Comment 24 Eric Sandeen 2008-05-29 08:54:43 EDT
> 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?
Comment 25 Russell Cattelan 2008-05-29 14:29:08 EDT
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
Comment 26 Thomas Moschny 2008-05-29 17:11:02 EDT
(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.
Comment 27 Thomas Moschny 2008-05-29 17:12:43 EDT
(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.
Comment 28 Russell Cattelan 2008-05-29 23:16:43 EDT
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
Comment 29 Tom "spot" Callaway 2008-05-30 09:52:00 EDT
(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@furius.ca 
> 
> Does that cause any problems for us?

Yes. Either get written permission to freely redistribute and modify or pull out
these docs/images.
Comment 30 Russell Cattelan 2008-05-31 14:53:13 EDT
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.
Comment 31 Tom "spot" Callaway 2008-06-04 16:57:56 EDT
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).
Comment 32 Russell Cattelan 2008-06-09 16:42:13 EDT
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
Comment 33 Thomas Moschny 2008-06-18 03:42:37 EDT
[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.
Comment 34 Thomas Moschny 2008-06-20 03:33:23 EDT
Forgot to update the fedora-review flag.
Comment 35 Russell Cattelan 2008-06-20 11:09:45 EDT
New Package CVS Request
=======================
Package Name: xxdiff
Short Description: Graphical file/directory viewer
Owners: cattelan
Branches: F-8 F-9
InitialCC:
Cvsextras Commits: yes
Comment 36 Kevin Fenzi 2008-06-20 11:26:34 EDT
cvs done.
I assume the legal blocker is solved with those removals...

Spot: Can you confirm? 
Comment 37 Tom "spot" Callaway 2008-06-20 11:39:20 EDT
Yes, he took out the screenshots directory and is using a clean tarball. Lifting
FE-Legal.
Comment 38 Thomas Moschny 2008-07-06 17:15:03 EDT
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.

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