Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 210467 - Review Request: wallpapoz - Gnome Multi Backgrounds and Wallpapers Configuration Tool
Summary: Review Request: wallpapoz - Gnome Multi Backgrounds and Wallpapers Configurat...
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Kevin Fenzi
QA Contact: Fedora Package Reviews List
URL:
Whiteboard:
Depends On:
Blocks: FE-ACCEPT
TreeView+ depends on / blocked
 
Reported: 2006-10-12 11:44 UTC by Mamoru TASAKA
Modified: 2007-11-30 22:11 UTC (History)
0 users

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2006-10-17 17:33:48 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Mamoru TASAKA 2006-10-12 11:44:37 UTC
Spec URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SPECS/wallpapoz.spec
SRPM URL: http://www.ioa.s.u-tokyo.ac.jp/~mtasaka/dist/extras/development/SRPMS/wallpapoz-0.3-1.src.rpm
Description: 
This tool enables your Gnome desktop to have different 
wallpapers for different workspaces or virtual desktops.

Comment 1 Kevin Fenzi 2006-10-14 17:56:47 UTC
OK - Package meets naming and packaging guidelines
OK - Spec file matches base package name.
OK - Spec has consistant macro usage.
OK - Meets Packaging Guidelines.
OK - License (GPL)
OK - License field in spec matches
OK - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
7b3c2189f24f3cee48acb4085944b7c4  wallpapoz-0.3.tar.bz2
7b3c2189f24f3cee48acb4085944b7c4  wallpapoz-0.3.tar.bz2.1
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
OK - Package has correct buildroot
OK - Package is code or permissible content.
OK - Packages %doc files don't affect runtime.
OK - Package is a GUI app and has a .desktop file
OK - Package compiles and builds on at least one arch.
OK - Package has no duplicate files in %files.
OK - Package doesn't own any directories other packages own.
OK - Package owns all the directories it creates.
OK - No rpmlint output.
OK - final provides and requires are sane:

SHOULD Items:

OK - Should build in mock.
i386/x86_64 - Should build on all supported archs
OK - Should have dist tag
OK - Should package latest version

Issues:

1. The srcurl and icondir macros seem like macro overkill to me, and
make the spec harder to read IMHO. Not a blocker, but suggest removing them.

2. Does the install.py script not work? Why are you doing your own
install in %install. Could some of these changes get submitted upstream?
It looks like they have a custom install.py, perhaps you could get them
to switch to a more standard one?


Comment 2 Mamoru TASAKA 2006-10-15 05:43:30 UTC
Well, before fixing 1:
 
> 2. Does the install.py script not work? Why are you doing your own
> install in %install. Could some of these changes get submitted upstream?
> It looks like they have a custom install.py, perhaps you could get them
> to switch to a more standard one?

A. This script installs files directly under /usr . This is truly bad.

B. Also, I made a change for the file names and the directory the files 
   are installed.
B-1. This package installs 0755-permission .py files under /usr/bin .
      While this is allowed, I think this should be avoided and the name of
      *.py in /usr/bin should be changed. See the "Unnecessary Byte 
      compilation" of 
      http://fedoraproject.org/wiki/Packaging/Python .
B-2 This package installs 0644-permission .py files under /usr/bin .
     This file needs byte-compiling. I think this is not allowed. 
     So I moved this file to /usr/share/wallpapoz. This needs extra treatments
     about other installed files.

However, all the issues about "B" is fedora-specific and I wonder if I should
report this to upstream.


Comment 3 Mamoru TASAKA 2006-10-15 17:10:59 UTC
Well, for 1:
I will fix for %srcurl (i.e. I won't use this), however,
honestly speaking, I don't want to write /usr/share/icons/hicolor/128x128/apps
many times because it is long.....

So, first please let me know if I should fix for B issue.

Comment 4 Kevin Fenzi 2006-10-15 23:07:28 UTC
Well, on the install.py script: Could you ask upstream to switch to a more 
standard install process? Are they responsive? I guess if they are not, you 
will have to keep the install process you have setup. It would be much better 
for upstream to have a sane install process however. 

On the macros: well, you only have to write %{_datadir}/icons/hicolor/ on 
making the inital spec file. If these files/directories don't change you 
shouldn't have to modify them again. I think the inital time investment is 
worth it to have a more readable spec, but as I said, it's not a blocker. 

Comment 5 Mamoru TASAKA 2006-10-16 07:28:15 UTC
Well,  I want to make it sure what issues are blocking this
bug report. You mean that 

* I should report upstream about installation process issue
  and wait for upstream's reaction before this package is
  approved? Until upstreamer fixes
  install.py, you won't approve this package?
* I should use %{_datadir}/icons/hicolor/ , not    
%{_datadir}/icons/hicolor/128x128/apps for %icondir macro?

Comment 6 Kevin Fenzi 2006-10-16 17:38:36 UTC
>* I should report upstream about installation process issue
>  and wait for upstream's reaction before this package is
>  approved? Until upstreamer fixes
>  install.py, you won't approve this package?

Sorry I wasn't clear there. I was hoping you could ping upstream about the 
issue, and perhaps wait a day or two to see if they were going to fix it 
quickly. If not, I would say go ahead and import and fix it later when 
they do upstream. 

>* I should use %{_datadir}/icons/hicolor/ , not    
>%{_datadir}/icons/hicolor/128x128/apps for %icondir macro?

No, this is not a blocker either... I think the macros make the spec file less 
readable, but there is not a hard rule against them. 

I will go ahead and APPROVE this package now, but you might want to see if 
upstream will quickly fix the install.py issues before importing (a few days?)

Comment 7 Mamoru TASAKA 2006-10-16 18:32:57 UTC
Well, before reporting to upstream, I want to ask you about..
A.
  By default, this package tries to install xml_processing.py into
  /usr/bin .  This file is called by /usr/bin/daemon_wallpapoz and
  /usr/bin/wallpapoz so usually this file (xml_processing.py) should
  be byte-compiled.
  However, this leaves non-executable files (.pyc and .pyo files)
  in /usr/bin and rpmlint claims about this (non-executable-in-bin).
  Also, by default xml_processing.py has 0644 permission. This is correct
  because xml_processing.py is just called by daemon_wallpapoz and 
  wallpapoz. However, leaving this file (xml_processing.py) under /usr/bin
  also calls rpmlint complaint.

  So I moved xml_processing.py to /usr/share/wallpapoz . However, this is
  somewhat fedora-specific. Also, moving xml_processing.py requires
  some fixes for two other python scripts.

  Then I wonder if I should ask for upstream to move this file 
  (xml_processing.py). I mean that I wonder to what degree I (and upstream)
  should obey the rule of "all files under /usr/bin should be executable" 
  Or just I should leave xml_processing.py under /usr/bin/ , set the permisson
  as 0755 and dont create  xml_processing.py{c,o} ?

B. 
   I usually think that python scripts under /usr/bin should not have the
   name of *.py , however, should I also ask for upstream to rename the files?
  (for this package, renaming .py files in /usr/bin also requires the fixes for
   the contents of wallpapoz.py and daemon_wallpapoz.py).

C.
   This package tries to install all files under /usr directory. This is
   truly bad for rpm building, however, if upstream doesn't care about
   rpm system and only think, I wonder how I should ask for upstream to make
   install.py have something like DESTDIR (upstream may just say,
   "anyway install should be done by root, so this is unnecessary").

In short, all I am concerned is that if I should ask for upstream to
adapt their package to the way which seems somewhat fedora-specific.......

Comment 8 Kevin Fenzi 2006-10-16 20:27:17 UTC
In reply to comment #7:

A: I think it is a good idea to not have .py/.pyc/.pyo files under /usr/bin. 
I think /usr/share/$name/ is a pretty good place for them. 

B: I agree. If they are executed by end users, I think it's much better for 
them to not have the .py on the end. 

C: I think it would affect the package on other distros as well as fedora 
extras. Does debian/ubuntu package this application? Suse?

I would suggest that upstream perhaps look at using python's distutils?
http://docs.python.org/dist/dist.html
That might also help them package for other distros and such. 

Comment 9 Mamoru TASAKA 2006-10-17 09:23:42 UTC
(In reply to comment #8)
.......

Kevin, thank you for comments. I've reported to upstream about
packaging issue + some other changes I have already added in spec
file.

Comment 10 Mamoru TASAKA 2006-10-17 14:29:59 UTC
Kevin,

I reported to upstream and upstream answered that he want to release
new version on Nov 15 (perhaps he want to improve this package 
functionally) in which my suggestion should be applied.

So I want to import this version (0.3) with the fixed by you and me applied.
Is it okay?

Comment 11 Kevin Fenzi 2006-10-17 16:08:27 UTC
In reply to comment #10: 

Yeah, you are welcome to go ahead and import/build this version. 
Once upstream fixes up their install you can update from there. 

Comment 12 Mamoru TASAKA 2006-10-17 17:33:48 UTC
Well:

* Imported to FE-devel, rebuild succeeded
* SyncNeeded is requested for FE-5
* added to owners.list, comps-fe6.xml.in

Now I close this bug as CLOSED NEXTRELEASE. Thank you for 
reviewing and approving this package!!


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