Bug 176542 - Review Request: gnome-yum - GNOME interface for YUM
Review Request: gnome-yum - GNOME interface for YUM
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Chris Chabot
Fedora Package Reviews List
:
Depends On:
Blocks: FE-ACCEPT
  Show dependency treegraph
 
Reported: 2005-12-25 02:55 EST by András Tóth
Modified: 2007-11-30 17:11 EST (History)
2 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2006-05-08 00:11:22 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description András Tóth 2005-12-25 02:55:42 EST
Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-1.ta.1.fc4.src.rpm
Description: GNOME Interface for YUM
Comment 1 drago01 2005-12-25 04:00:51 EST
your spec file is autogenerated.
look at http://fedoraproject.org/wiki/PackagingGuidelines and edit your file by
hand.
Comment 2 András Tóth 2005-12-26 09:53:36 EST
(In reply to comment #1)
> your spec file is autogenerated.
> look at http://fedoraproject.org/wiki/PackagingGuidelines and edit your file by
> hand.

I modified gnome-yum.spec and create new src.rpm...

Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url:
http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-2.src.rpm
Comment 3 Panu Matilainen 2005-12-26 10:57:59 EST
This is not a full review but a couple of comments:
- The consolehelper symlinks don't belong to %post/%postun scriptlets, package
them along with other content in %install
- You should add dependency on usermode(-gtk) for the above
- GConf2, vte, gnome-vfs2 dependencies are probably automatically detected by
rpmbuild, if so they should be removed as redundant
- Requires: rpm is silly - you must have rpm installed to install an rpm anyway
- Adding 3rd party repositories containing known legally problematic packages is
not allowed, the fedora-repositories subpackage and any references to those
repositories needs to be removed.
- The for-loop for copying the docs is redundant, just use
  %doc ABOUT-NLS AUTHORS README... in %files section
- I'd suggest using %find_lang macro instead of the current
{_datadir}/locale/*/LC_MESSAGES/gnome-yum.mo in %files
Comment 4 Enrico Scholz 2005-12-26 11:31:13 EST
> - Requires: rpm is silly - you must have rpm installed to install an rpm anyway

not necessarily; chroot installations do not need rpm
Comment 5 András Tóth 2005-12-26 13:36:18 EST
- Okay. I remove all repositories from source package and I insert %find_lang
macro to the spec file.
- I can't solve the consolehelper symlinks with your solving and I don't modify
this problem in the end.
(error message:
   RPM build errors:
   Symlink points to BuildRoot: /usr/bin/gnome-yum =>
/var/tmp/gnome-yum-0.1.2-3-root-root/usr/bin/consolehelper)

Here is the next release:

Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url:
http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-3.src.rpm
Comment 6 Paul W. Frields 2005-12-26 14:00:16 EST
The URL for your spec is pointing to an old version, could you update it please?
Comment 7 Ignacio Vazquez-Abrams 2005-12-26 14:20:31 EST
- Missing BR: gtkhtml2-devel

(In reply to comment #3)
> - Requires: rpm is silly - you must have rpm installed to install an rpm anyway

Strictly speaking you need to have rpm-libs and a front-end. What's interesting
is that I don't see any path from gnome-yum to rpm-libs. Am I missing something
obvious?
Comment 8 drago01 2005-12-27 02:08:35 EST
BuildArch:       i386
an reason for that? what about x86_64 ?
Comment 9 András Tóth 2005-12-27 02:29:59 EST
(In reply to comment #6)
> The URL for your spec is pointing to an old version, could you update it please?

Use 'Refresh' button... I used it too.

(In reply to comment #8)
> BuildArch:       i386
> an reason for that? what about x86_64 ?

Huh! Truly. I remove it. Here is the 4th release:

Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url:
http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-4.src.rpm
Comment 10 Paul W. Frields 2005-12-27 16:54:12 EST
(In reply to comment #9)
> (In reply to comment #6)
> > The URL for your spec is pointing to an old version, could you update it please?
> 
> Use 'Refresh' button... I used it too.

I doubt that was the problem, since it was the first time I visited.  But I see
the right version now, so thanks.
Comment 11 Jef Spaleta 2005-12-30 12:25:26 EST
(In reply to comment #5)
> - I can't solve the consolehelper symlinks with your solving and I don't modify
> this problem in the end.
> (error message:
>    RPM build errors:
>    Symlink points to BuildRoot: /usr/bin/gnome-yum =>
> /var/tmp/gnome-yum-0.1.2-3-root-root/usr/bin/consolehelper)

There are two ways of doing this currently implemented in Extras

yumex in Extras has the symlinks created inside the Makefile in the source.
netgo in Extras uses a construction in the specfile in the %install section:

take a look at both the src.rpm for both yumex and netgo and choose a method
that best suits you.  Don't forget to add Requires: usermode  to make sure
consolehelper is installed on the system as a dependancy.

-jef
Comment 12 András Tóth 2005-12-31 06:28:32 EST
(In reply to comment #11)

I resolved this problem in the next release:
Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url:
http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-5.src.rpm
Comment 13 Chris Chabot 2006-01-12 16:38:08 EST
Missing build requires: gtkhtml2-devel (config fails without it)

(trying srpm from comment #12)
rpmlint isn't so happy either:
E: gnome-yum no-changelogname-tag
E: gnome-yum zero-length /usr/share/doc/gnome-yum-0.1.2/README
W: gnome-yum no-dependency-on usermode-consoleonly
E: gnome-yum non-executable-script /usr/share/gnome-yum/gyum-query.sh 0644
E: gnome-yum zero-length /usr/share/doc/gnome-yum-0.1.2/NEWS
W: gnome-yum non-conffile-in-etc /etc/pam.d/gnome-yum
E: gnome-yum zero-length /usr/share/doc/gnome-yum-0.1.2/TODO
W: gnome-yum non-conffile-in-etc /etc/security/console.apps/gnome-yum

Changelog is just completely missing .. shouldn't be :-)

Empty file warnings are easy to fix, just leave them out :-) The non-executable,
maybe a root,755,root %attr for it? 

The warning about the non conf file can be ignored, since it is a config file 

I see the spec file uses:
[ -n "$RPM_BUILD_ROOT" -a "$RPM_BUILD_ROOT" != "/" ] && rm -rf $RPM_BUILD_ROOT
This was a standard (a good while ago) but the current guidelines say to just
use: rm -rf $RPM_BUILD_ROOT

If no one else has stepped up for review yet, i'll be willing too, changing
blocker to FE-REVIEW. Fix the issues mentioned in the comments (including this
one :-)) and once thats done i'll do the full review
Comment 14 Chris Chabot 2006-01-12 16:53:17 EST
It would also be ok to do:
%{_datadir}/pixmaps/gnome-yum

in the files list, no need to type out all the file names really, and otherwise
you wouldn't own the directory

for the config file mentioned above, try putting %config(noreplace) before them
(pam.d/gnome-yum and console.apps/gnome-yum), which is proper thing to do with
config files

also it might be usefull to put gettext in the build requires, while i've never
seen a live fedora system without, mock does need to know about it else it might
trip over it
Comment 15 Chris Chabot 2006-01-12 17:00:30 EST
Desktop file should be parsed with desktop-file-install, adding the Fedora
category, and add desktop-file-utils to the buildrequires

Lastly there's no need for the: [ ! -f Makefile ] || make distclean
line is there? If gnome-yum does its releases properly it should never be
shipped with a Makefile
Comment 16 András Tóth 2006-01-14 09:02:42 EST
I repair (Comment #13-#15) bugs (exception rpmlint warning: 'W: gnome-yum
no-dependency-on usermode-consoleonly') and create a new package:

Spec Name or Url: http://gnome-yum.sourceforge.net/download/gnome-yum.spec
SRPM Name or Url:
http://gnome-yum.sourceforge.net/download/gnome-yum-0.1.2-6.src.rpm

Comment 17 Chris Chabot 2006-01-14 09:24:27 EST
Few last comments before i can do a complete formal review checklist:

- Its not Buildrequires but: BuildRequires: (notice the capital R)

- %changelog doesn't have to include the upstream (source) changes, but the
changes you made to the package, ie:

* Sat Jan 14 2006 András Tóth  <toth_bandi@users.sourceforge.net> - 0.1.2-6 
- Fixed desktop file vendor
- Owned complete datadir
- Changed clean section to be fedora compliant
- find_lang can use %{name} macro, doesn't need to hard-code 'gnome-yum'

And so on .. Your expected to add such a changelog entry for every 'release' of
the package you make (ie up the version or release field) describing what you
changed in the specfile. Its not required to list what changed in 'gnome-yum'
its self,only what you changed of the package / specfile

- attr for gyum-query.sh doesn't have to go thru a defattr, you could just do:
%attr(0755,root,root) %{_datadir}/gnome-yum/gyum-query.sh 

- desktop file install misses:
   --add-category X-Fedora

- Missing BuildRequires entry: libgnomeui-devel

Thats it i think, great progress so far! Once these final issues are resolved
i'll give it the final formal run thru
Comment 18 Chris Chabot 2006-01-14 09:34:53 EST
Last 2 small comments: 

- No ; needed after the done (in the for doc ... loop), also no ; needed after
the  rm -f line

- Could you please add:
   %dir %{_datadir}/pixmaps/gnome-yum
before the: %{_datadir}/pixmaps/gnome-yum line?

Thats all i could find :-)


Comment 20 Chris Chabot 2006-01-14 12:51:49 EST
Thanks that solves all the issues reported

However it looks like you modified the source.tar.bz2 file? MD5SUM now missmatches

md5sum:
43e4f678a9243c92297f0929e7d87efe  (original from sf.net) gnome-yum-0.1.2.tar.bz2
bc105a3711409e8bdd66522caa59b316  (from src.rpm) gnome-yum-0.1.2.tar.bz2

I presume you've done this to include the X-Fedora category? Might be easier to
use the original source and in your spec file do:

desktop-file-install --vendor fedora --delete-original       \                 
                                             
  --dir $RPM_BUILD_ROOT%{_datadir}/applications             \    
  --add-category X-Fedora \                                                    
       
  $RPM_BUILD_ROOT%{_datadir}/applications/gnome-yum.desktop    

It is a strict policy for Fedora Extra's that source tarbal must match the
original one (checked with md5sum) so unfortunatly i can't accept this package yet.

Please either update the upstream tarbal, or use the original one and add the
--add-category X-Fedora to the desktop-file-install

Other then that its looking great, and builds cleanly in mock too. I will give
it the formal review once the source tarbal issue is resolved.
Comment 21 András Tóth 2006-01-14 14:09:52 EST
I need modify the next files in the source package:
./configure.in and ./configure => remove gtkhtml2-devel checking
src/callbacks.c => remove one line: #include <libgtkhtml/gtkhtml.h>
./README,./TODO => to be not empty
./gnome-yum.spec => fix format bugs
./desktop.in => Add fedora category

If it is necessary I make a new release (v0.1.3). It is OK?
Comment 22 Chris Chabot 2006-01-14 14:18:22 EST
Making a 1.3.0 release would be preferable then yes

Or alternativly you could make a patch file and include it in the spec/rpm that
makes the changes you mentioned, but since your able to change this upstream,
making a new release would be easier yes :-)

As soon as you have that ready and with a new .spec / .src.rpm i'll give it the
formal review.

Thanks for the effort so far!
Comment 24 Chris Chabot 2006-01-14 17:10:07 EST
Looks good, the formal review:

MUST review items:
- Builds cleanly on FC5 devel.
- rpmlint has no output / complaints
- Source included matches upsteam source (md5sum)
- Package name meets guidelines
- spec file name is in %{name}.spec format
- Licence (GPL) is fedora extra's compatible & is included in spec
- Spec file is in (american) english
- Does not list buildrequires that are excepted in the package guidelines
- All build dependencies are listed
- No ldconfig needed
- All files have proper permissions
- Package is not relocatable
- No duplicate files in %files section
- No missing files in %files section
- Has a proper %clean section with rm -rf $RPM_BUILD_ROOT
- Uses macro's described in PackagingGuidelines
- No entries in %doc that are required for standard program operation
- No -devel package needed
- No directory-ownerships needed

Should items:
- Includes upstream licence file (COPYING)
- No insane scriplets
- No unnescesarry requires

rpmlint's only warning is:
W: gnome-yum no-dependency-on usermode-consoleonly

Mock builds cleanly for fc-devel

Please read:
- http://fedoraproject.org/wiki/Extras/NewPackageProcess and
- http://fedoraproject.org/wiki/Extras/Contributors and 
- http://fedoraproject.org/wiki/Extras/UsingCvsFaq

Carefully for the next steps


FE-APPROVED
Comment 25 Christian Iseli 2006-03-28 10:47:57 EST
Any particular reason why this package is not yet imported and built ?
Comment 26 Josh Boyer 2006-03-28 20:13:17 EST
Most likely because I just sponsored the packager this past weekend.
Comment 27 Michael Schwendt 2006-04-02 20:02:28 EDT
Next time please add a _valid_ email address in CVS owners.list,
where "valid" means it is used in your bugzilla account!
Comment 28 András Tóth 2006-04-03 01:35:14 EDT
Why the toth_bandi@users.sourceforge.net isn't valid e-mail address? 
Comment 29 András Tóth 2006-04-03 04:43:17 EDT
Okay. I tested this address and really it isn't working. The problem is the mail
system of sourceforge.net. I am waiting any times and if it is necessary I shall
change e-mail address...
Comment 31 Michael J Knox 2006-05-08 00:11:22 EDT
package is in the repos. Clsoing bug. 

Please remember to close package reviews once they have been imported into cvs
etc etc

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