Bug 225792 - Merge Review: gfs2-utils
Summary: Merge Review: gfs2-utils
Keywords:
Status: CLOSED RAWHIDE
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: 429769
Blocks:
TreeView+ depends on / blocked
 
Reported: 2007-01-31 18:43 UTC by Nobody's working on this, feel free to take it
Modified: 2008-04-30 15:22 UTC (History)
6 users (show)

Fixed In Version: rawhide
Clone Of:
Environment:
Last Closed: 2008-04-30 15:22:00 UTC
Type: ---
Embargoed:
kevin: fedora-review+


Attachments (Terms of Use)

Description Nobody's working on this, feel free to take it 2007-01-31 18:43:00 UTC
Fedora Merge Review: gfs2-utils

http://cvs.fedora.redhat.com/viewcvs/devel/gfs2-utils/
Initial Owner: cfeist

Comment 1 Steve Whitehouse 2007-02-02 15:47:46 UTC
What do we need to do in order to resolve this?


Comment 2 Steve Whitehouse 2008-03-05 17:16:05 UTC
Josef, should we be doing something for this bz? Its been kicking about for a
while and I don't know what we need to do to resolve it? Can we just close it or
do we need to take some action first?

Comment 3 Josef Bacik 2008-03-05 19:14:08 UTC
Is gfs2-utils in fedora?  I don't remember why I have this b/c IIRC its already there.  If its 
not then it needs to be assigned to somebody who reviews packages for inclusion into 
fedora, as its been quite a while since i looked at/cared about fedoras packaging guidlines.

Comment 4 Chris Feist 2008-03-05 20:02:25 UTC
Looks like it's already in.  I'm closing this bug as CURRENTRELEASE.

Comment 5 Kevin Fenzi 2008-03-05 20:10:06 UTC
No, all packages that were in 'core' before the merge need to be reviewed and
make sure they meet all the guidelines, etc... 

So, someone needs to review this and confirm it's all good before we can close this.



Comment 6 Josef Bacik 2008-03-05 21:53:18 UTC
then somebody else needs to be assigned to this, like i said before i havent done a 
package review in a while.

Comment 7 Steve Whitehouse 2008-03-12 16:09:31 UTC
How do we get a reviewer assigned? According to the blurb on the Fedora site, it
seems that its supposed to happen automatically, but thus far nobody has taken
it on and its been open for over a year now.

Comment 8 Kevin Fenzi 2008-03-12 23:19:48 UTC
Well, there are currently 745 pending Reviews in the NEW state: 
http://fedoraproject.org/PackageReviewStatus/NEW.html

And a small handful of reviewers slowly processing them down... 

You could offer to review someone elses packages and have them review yours. 
You could bribe someone to review it. ;) 

Note that if you want it to be in the NEW pile for people to see, you should
assign it to nobody... 

Comment 9 Steve Whitehouse 2008-03-13 08:38:11 UTC
I'd seriously consider bribery if only I could find a list somewhere of
potential bribees :-) Do you know if there is such a list? Again the wiki is
silent on the matter.

Comment 10 Kevin Fenzi 2008-03-15 16:52:58 UTC
Yeah, anyone who is a current maintainer... ie, everyone in the 'cvsextras'
group. See: 

https://admin.fedoraproject.org/accounts/group/view/cvsextras

However, I am in a reviewing mood... so I will go ahead and do a review for
you... :)

Look for a full review in a bit here. 


Comment 11 Kevin Fenzi 2008-03-15 17:23:23 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 (GPLv2)
See below - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
See below - Sources match upstream md5sum:
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 has rm -rf RPM_BUILD_ROOT at top of %install

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.
See below - No rpmlint output.
OK - final provides and requires are sane.

SHOULD Items:

OK - Should build in mock.
OK - Should build on all supported archs
OK - Should function as described.
OK - Should have dist tag
See below - check for outstanding bugs on package.

Issues:

1. The License tag is not right. Should be "GPLv2"

2. You might ask upstream to include a copy of the GPL with the package.
Not a blocker.

3. Is there an upstream download URL you can place in the
Source0 line? Or can you comment where to get the source?
See: http://fedoraproject.org/wiki/Packaging/SourceURL

4. The URL seems to no longer be correct. That redirects me to:
http://sources.redhat.com/cluster/wiki

5. Does parallel make not work with this package?
If it does, please change the 'make' to 'make %{?_smp_mflags}'
If not, please add a note that it doesn't work.

6. You might add:
Requires(preun): /sbin/service
and stop the service in preun?
See:
http://fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-a6d7a1ed9d77dbb8d4af067378a79b838aebb20a

7. rpmlint says:

gfs2-utils.src: W: invalid-license GPL
gfs2-utils.x86_64: W: invalid-license GPL
gfs2-utils-debuginfo.x86_64: W: invalid-license GPL

These will be fixed by changing to 'GPLv2'

gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_convert.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_fsck.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_mount.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_tool.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/mkfs.gfs2.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_grow.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_quota.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2_jadd.8.gz
gfs2-utils.x86_64: W: spurious-executable-perm /usr/share/man/man8/gfs2.8.gz

For some reason your man pages are not the right mode.
Can you file a bug upstream to install them as 644?

gfs2-utils.x86_64: E: non-standard-executable-perm /sbin/gfs2_jadd 0775

Why 775 here and not 755?

gfs2-utils.x86_64: E: missing-mandatory-lsb-keyword Description in
/etc/rc.d/init.d/gfs2
gfs2-utils.x86_64: E: missing-mandatory-lsb-keyword Short-Description in
/etc/rc.d/init.d/gfs2

Might fix up the init script to have the lsb keywords in it.

gfs2-utils.x86_64: W: incoherent-init-script-name gfs2

This is due to the init script not being the same name as the package.
Ignore, unless you want to re-name it to 'gfs2-utils' instead?

8. Looking at the outstanding bugs against this package I see two:

224154 - Should just be closed now?

429769 - Can this be closed now? Is rawhide up to date?

If you could address all the above, then let me know I will re-run my checks
and we can get this closed out. Let me know if there are any questions.


Comment 12 Steve Whitehouse 2008-04-09 12:50:16 UTC
So far...

1. I've fixed the license in F-8, and I'll see if we can get the right things
added to the upstream source
3. I'll try and sort that out shortly
4. Fixed in F-8, will move changes to other versions shortly once we've worked
through all the issues in F-8
5. probably. I need to check that one
6. I'll look into that too
7. license fixed, man page perms are fixed in upstream git, but not yet in the
release tar ball. I'll try and get another release done.
8. There are more than just the two you've spotted, most of them depend on
getting changes that are already done in upstream into the rpm. I've just added
two more for build warnings in fact that we ought not to have.

Kevin, don't rerun your stuff quite yet, but please do take this as an
indication that we are working on the issues at the moment and it shouldn't be
too long before we are done.

Comment 13 Kevin Fenzi 2008-04-09 15:39:55 UTC
Excellent. Just let me know when you would like me to recheck things. 

Thanks for looking at it. 

Comment 14 Steve Whitehouse 2008-04-14 10:27:37 UTC
Ok. I think we've sorted out all of the above now. Please take a look and let us
know if you can still see anything missing. bz #224154 and #433592 are fixed by
the latest rawhide build. I'm not sure if I need to do anything to push that out
like I did for F-8? make update doesn't seem to work for devel. I marked the
bugs modified in the mean time.


Comment 15 Steve Whitehouse 2008-04-21 16:09:41 UTC
Also updated now in rawhide/F-9. Currently all three are identical to each other.

Comment 16 Kevin Fenzi 2008-04-29 02:36:13 UTC
Sorry for the delay here... ;( 

1-5 look good. 

6. You aren't stopping the service in preun, you might want to do that?

7. 
rpmlint now looks much better: 
gfs2-utils.x86_64: W: service-default-enabled /etc/init.d/gfs2
gfs2-utils.x86_64: E: missing-mandatory-lsb-keyword Description in /etc/init.d/gfs2
gfs2-utils.x86_64: E: missing-mandatory-lsb-keyword Short-Description in
/etc/init.d/gfs2
gfs2-utils.x86_64: W: incoherent-init-script-name gfs2

So, I don't see any further blocker issues here. You might look at item 6 again,
but otherwise things look good. This package is APPROVED. 

Feel free to close this bug RAWHIDE. 

Comment 17 Steve Whitehouse 2008-04-29 08:44:44 UTC
I added the Requires(preun): /sbin/service line as you'd suggested so what else
do I need to do in order to satisfy item 6?


Comment 18 Kevin Fenzi 2008-04-29 14:39:00 UTC
Simply make your preun: 

%preun
if [ $1 = 0 ]; then
        /sbin/service gfs2 stop >/dev/null 2>&1
        /sbin/chkconfig --del <script>
fi

Ie, stop the service before removing the init script and package... 


Comment 19 Steve Whitehouse 2008-04-29 14:47:14 UTC
Ah, I see now. I'll add that on the next update. Thanks.


Comment 20 Steve Whitehouse 2008-04-30 15:22:00 UTC
I've added the line to preun as suggested above in the rawhide package. Once f-9
is out properly, I'll add it to that as well on the next update. So I'm now
going to close this as suggested in comment #16.


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