Bug 182306

Hi Zack, this package needs a bit of cleanup before it goes into Fedora Extras.
 You should read these web pages and apply some of the guidelines to the package:

  http://www.fedoraproject.org/wiki/Packaging/Guidelines  -- This is the master
list of standard things the package will be reviewed for.

  http://www.fedoraproject.org/wiki/Packaging/ReviewGuidelines  -- This is a
checklist approach to the packaging guidelines.  A good starting point is to go
through the items on this list and reference the Packaging/Guidelines page for
explanations.  The Packaging/Guidelines page contains some things that aren't
listed here, though.

  http://www.fedoraproject.org/wiki/Packaging/Python  -- Some helpful tips on
packaging python programs.

  http://www.fedoraproject.org/wiki/Extras/SIGs/Python  -- the Python Packaging
Special Interest Group page in case you want to influence how python programs
are packaged for Extras.

There's a review day on Sunday, March 4, if you want to drop by #fedora-extras
on irc.freeode for some additional help.

Looks like someone on the pyspi bug is willing to sponsor you.  Here's a full
review for dogtail to work on while you get through the paperwork.

rpmlint
W: dogtail no-version-in-last-changelog
  Add the version information after each Changelog entry:
  * Fri Feb  17 2006 Zack Cerza <zcerza> - 0.5.0-2

E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/i18n.py 0644
E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/rawinput.py 0644
E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/logging.py 0644
E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/utils.py 0644
E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/path.py 0644
  The files in question are not intended to be executables but they have
  #!/usr/bin/python lines so rpmlint is complaining.  Removing the #! line
  upstream is probably the best thing to do.

E: dogtail standard-dir-owned-by-package /usr/bin
E: dogtail standard-dir-owned-by-package /usr/lib
  These are blockers.  See comments under Needswork.

E: dogtail hardcoded-library-path in /usr/lib/
  This is a blocker.  See comments under Needswork.

E:dogtail script-without-shellbang
/usr/share/doc/dogtail-0.5.0/examples/dogtail/apps/wrappers/evolution.py
  evolution.py has the executable bit set when it shouldn't.
  Actually, though, the examples/dogtail directory should be deleted upstream.

E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/config.py 0644
E: dogtail non-executable-script
/usr/lib/python2.4/site-packages/dogtail/tree.py 0644
  Safe to ignore.  These have test cases that can be run.

W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/abiword-test.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-composing-html.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-configuring-exchange.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-configuring-imap-smtp.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-first-time-wizard.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-sending-email.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-survives-email-CAN-2005-0806.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/evolution-test-switching-components.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/filechooser-stress-test.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/firefox-test-browsing-local-html-file.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/frysk-click-processes.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/gcalctool-test-fibonacci.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/gedit-test-utf8-procedural-api.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/gedit-test-utf8-tree-api.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/gnome-panel-test-starting-every-app.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/google-search.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/i18n-test.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/nautilus-test-icon-view-collage.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/no-help-at-all.py /usr/bin/env
W: dogtail doc-file-dependency /usr/share/doc/dogtail-0.5.0/examples/recorder.py
/usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/rhythmbox-test.py /usr/bin/env
W: dogtail doc-file-dependency
/usr/share/doc/dogtail-0.5.0/examples/test-events.py /usr/bin/env
  There has been talk of removing execute permissions on anything in %{_docdir}.
 I don't believe this has been made a hard requirement yet but you can see
  it causes rpm to auto-require dependencies that the package without %doc
  does not.  If you remove the execute permissions on examples this goes away:
    %install
    find examples -type f -exec chmod 0644 \{\} \;

W: dogtail dangerous-command-in-%post rm
  There doesn't seem to be a good reason for this.  Remove it.

W: dogtail strange-permission dogtail.spec 0600
  You can prevent this by changing the spec to 0644 but it isn't strictly
  necessary.

Needswork:
* Source0: should be a complete URL:
  http://people.redhat.com/zcerza/dogtail/releases/%{name}-%{version}.tar.gz
* The upstream source has a problem.  There are two versions of
  dogtail-0.5.0.tar.gz  One version is in dogtail/releases/.  The other is in
  dogtail/releases/rpm_inst/.  Since you are upstream, it would probably be
  best if you create a dogtail 0.5.1 that includes whichever is the preferred
  version.
* The included tarball has two copies of the dogtail/ directory.  One at the
  toplevel and a second one that I think is in error in dogtail/examples.  If
  your spinning a new upstream release based off this source you probably want
  to address this as well.
* Should BuildRequires: python even though it's already in the build
  environment
* I see an explicit ChangeLog saying you needed to add at-spi-devel in order to
  build.  Can you explain that?  I've built the package with at-spi-devel
  installed and uninstalled/BuildRequires taken out and diff -ur does not show
  any differences.  Unless there's something I'm missing, you should take this
  out.
* The example files and scripts should be created non-executable.
* At least on FC4, brp-python-bytecompile does not include documentation.
  Unless you know of a place where this fails, the definition of
  __os_install_post should be removed.
* Macros should be used in place of directory names.  The python package is
  compiled using directory macros like %{_libdir}, %{_bindir}, etc.  When you
  use python's distutils via the setup.py script, python will substitute these
  paths.  If the path definitions ever change, your hardcoded definitions would
  have to be manually changed whereas %{_bindir}, etc will continue to work.
  - For the python files in /usr/lib, it's an even better idea to use python's
    arch independent libdir.  A macro to ca[ture this is:
    %{!?python_sitelib: %define python_sitelib %(%{__python} -c "from
distutils.sysconfig import get_python_lib; print get_python_lib()")}
    Then you can use  %{python_sitelib}/dogtail/ in your %files section.
    This protects you in case python's site independent directory changes in
    the future (moves to /usr/share/python3.0, for instance)
* The .desktop file must be installed via desktop-file-utils:
  http://www.fedoraproject.org/wiki/Packaging/Guidelines#desktop
  to add fedora as the vendor and X-Fedora as a category.
* This package owns system directories which is a big no-no.
  Instead of /usr/bin/ use %{_bindir}/*
  Instead of /usr/lib/ use the %{python_sitelib}/dogtail/ syntax noted earlier.
  Instead of /usr/share/applications/ use %{_datadir}/applications/*
  /usr/share/icons/hicolor/.... I'm not sure what to do about.  The hicolor
  hierarchy isn't directly owned by anything we depend on but it seems to be
  expected by a good many gnome/gtk+ programs.  I'll ask fedora-extras for
  ideas.
* Please see:
 
http://www.fedoraproject.org/wiki/ScriptletSnippets#head-d37d740d062d3aa6013aab44a79de88a6c1fe533
  for an example of using gtk-update-icon-cache.  In particular, the touch of
  the hicolor directory, the use of || : to prevent errors (for instance,
  if a %_netsharedpath is being used on the installer's system), and calling
  gtk-update-icon-cache from a %postun as well as %post.

Cosmetic:
* The BuildArchitectures: tag is usually written as BuildArch:.  rpmbuild 4.4.1
  appears to recognize both syntaxes, though.
* Fedora Extras usually puts Requires/BuildRequires on separate lines for
  readability.
* Group should probably be Development/Tools

Highlights of passed checklist items:
* Name follows naming guidelines
* spec name matches package name
* OSI License (GPL)
* Copy of the GPL included
* Builds on an FC4 x86_64 box (noarch package)
* Permissions are okay
* Has a proper %%clean
* No %doc files are necessary for the operation of the program.
* Builds in mock

Fix these things up and I'll test out whether it works okay.

Another minor thing is changing the BuildRoot similar to my comment in the pyspi
review.

I believe I've made all the suggested changes. Oddly, the specfile in the SRPM
has 0600 permissions even though the one in the tarball has 0644 permissions.
I'm not sure how to fix that...

http://people.redhat.com/zcerza/dogtail/releases/FE/dogtail-0.5.1-1.src.rpm
http://people.redhat.com/zcerza/dogtail/releases/FE/dogtail.spec

Mustfix:
* URL was fine last time.  Source0 was the field that needed to be changed.  So
  they should look like this:
  URL: http://people.redhat.com/zcerza/dogtail/
  Source0:
http://people.redhat.com/zcerza/dogtail/releases/%{name}-%{version}.tar.gz
* desktop file: I'm sorry.  I think my instructions were ambiguous on this.
  The desktop file needs to be installed via desktop-file-install in order
  to have vendor: fedora and category: X-Fedora added.  To do this, you can
  do this::
    BuildRequires: desktop-file-utils
    [...]
    %install
    desktop-file-install $RPM_BUILD_ROOT/%{_datadir}/applications/sniff.desktop
      --vendor=fedora \
      --dir=$RPM_BUILD_ROOT/%{_datadir}/applications \
      --add-category X-Fedora \
      --delete-original

  I don't think this program needs to have update-desktop-database run on
  install and uninstall (it's not adding a MimeType key).  If that was just
  added as part of the confusion, then this can all be removed::
    Requires(post): desktop-file-utils
    Requires(postun): desktop-file-utils
    [...]
    [In %post and in %postun]
    update-desktop-database &> /dev/null || :

Fixed:
* rpmlint warnings taken care of.
* Source matches the one upstream version.
* Problems in the source tarball have ben resolved.
* Now Buildrequires python.
* at-api-devel is no longer BuildRequired.
* examples are no longer executable.
* __os_install_post removed.
* Macros are used for directory names.
* System directories are not owned.
* gtk-update-icon-cache is properly invoked.
* Cosmetic items have been cleaned up as well.

Good:
* Still builds in mock on FC4 x86_64.
* Installs and runs fine on FC4 x86_64.

Fix the two problems, bump the release, update the srpm, and I'll approve.

Thanks! Here's the updated SRPM:

http://people.redhat.com/zcerza/dogtail/releases/FE/dogtail-0.5.1-2.src.rpm

APPROVED

81b987b1f18f1741d87f937877a4b071  dogtail-0.5.1-2.src.rpm

One note:  Do you have an automated script that you use to build the packages? 
Because the dogtail-0.5.1.tar.gz file has changed between releases.  I checked,
though, and the changes between the two are just the changes to the spec file.

Modifying the upstream tarball without changing the release number is not a
great idea as it leads to slightly different packages in different distributions
and prevents reviewers from doing a simple md5sum to check that the upstream
sources are valid.  In this case, the changes are small and well known so it's
not a big deal.  Something to keep in mind for the future, though.

Everything important is fixed in this release and it still builds in mock and
runs.  The only thing I noticed is that the URL field now points at:: 
  http://people.redhat.com/zcerza/dogtail/releases/
instead of the webpage::
  http://people.redhat.com/zcerza/dogtail/

This is a minor issue.  No need to respin and re-review.  Just something you'll
want to change after you import the package into CVS and before pushing through
the buildsystem.

You're ready for Step 8::
  http://www.fedoraproject.org/wiki/Extras/NewPackageProcess

Thanks again, and sorry for the delay. I've just queued a build for the devel
branch, and requested an FC5 branch. The URL is fixed.

Builds succeeded! yay.