Bug 431277 - Review Request: ocfs2-tools - programs for managing Ocfs2 file systems
Review Request: ocfs2-tools - programs for managing Ocfs2 file systems
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Jarod Wilson
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2008-02-01 17:45 EST by Mark Fasheh
Modified: 2009-05-22 12:56 EDT (History)
4 users (show)

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


Attachments (Terms of Use)
Latest ocfs2-tools spec file. (5.39 KB, text/x-rpm-spec)
2008-02-04 18:09 EST, Mark Fasheh
no flags Details
Latest ocfs2-tools spec file. (5.38 KB, text/x-rpm-spec)
2008-02-05 15:45 EST, Mark Fasheh
no flags Details
Suggested spec changes in diff format (5.33 KB, patch)
2008-02-20 15:41 EST, Jarod Wilson
no flags Details | Diff
Latest ocfs2-tools spec file, with diff applied (5.66 KB, text/x-rpm-spec)
2008-02-21 15:20 EST, Mark Fasheh
no flags Details
Latest ocfs2-tools spec file against latest ocfs2-tools git (5.86 KB, text/x-rpm-spec)
2008-02-21 20:20 EST, Mark Fasheh
no flags Details
Latest ocfs2-tools spec file, with minor changes (5.79 KB, text/x-rpm-spec)
2008-02-22 16:39 EST, Mark Fasheh
no flags Details

  None (edit)
Description Mark Fasheh 2008-02-01 17:45:42 EST
Spec URL: http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools.spec
SRPM URL: http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools-1.3.9-4.20080131git.src.rpm
Description: 

Ocfs2 is a shared disk cluster file system which has been included in the mainline Linux kernel since 2.6.16. The module is currently built as part of the standard Fedora kernel. This is a set of tools useful for managing an Ocfs2 file system. The package also includes "ocfs2console", a gui application which makes configuration of Ocfs2 clusters easier.
Comment 1 Mark Fasheh 2008-02-01 17:46:34 EST
The package builds successfully on my x86_64 and i386 systems. Rpmlint is
clean on the spec file, the SRPM, the ocfs2console rpm and the debuginfo
rpm. On ocfs2-tools-devel-1.3.9-4.20080131git.x86_64.rpm I get:

$ rpmlint  ocfs2-tools-devel-1.3.9-4.20080131git.x86_64.rpm
ocfs2-tools-devel.x86_64: W: no-documentation

Which I believe should be fine as no documentation is provided upstream.


On ocfs2-tools-1.3.9-4.20080131git.x86_64.rpm I get:

$ rpmlint ocfs2-tools-1.3.9-4.20080131git.x86_64.rpm
ocfs2-tools.x86_64: W: service-default-enabled /etc/init.d/ocfs2
ocfs2-tools.x86_64: W: service-default-enabled /etc/init.d/ocfs2
ocfs2-tools.x86_64: W: service-default-enabled /etc/init.d/o2cb
ocfs2-tools.x86_64: W: service-default-enabled /etc/init.d/o2cb
ocfs2-tools.x86_64: E: subsys-not-used /etc/init.d/o2cb

I believe an exception should be made in that the Ocfs2 cluster services
should be enabled by default. It's common for a user to have critical
software loaded at boot from an Ocfs2 file system and leaving the services
off by default (thus making their file systems unmountable) violates the
principal of least surprise, and could actually result in a loss of
availability.

Comment 2 Bill Nottingham 2008-02-01 18:05:40 EST
AutoReqProv: No <- why?
Comment 3 Bill Nottingham 2008-02-01 18:07:17 EST
As for the init script, it would seem to be logical that it just be added to the
list of types in netfs, or similar.
Comment 4 Mark Fasheh 2008-02-01 18:36:43 EST
(In reply to comment #2)
> AutoReqProv: No <- why?

It's completely unnecessary. A new spec file and srpm without AutoReqProv have
been uploaded.
Comment 5 Joel Becker 2008-02-01 18:41:47 EST
Regarding the init script, do you mean the ocfs2.init script that mounts and
unmounts ocfs2 volumes?  The o2cb.init script needs to be run in any case, so
I'm assuming you mean ocfs2.init, which is indeed similar to netfs.  If the
modifications to netfs would be accepted (I'd assume copying the NFS bits), I
think that would be excellent and ocfs2.init would not need to be installed.
Comment 6 Mark Fasheh 2008-02-01 19:05:01 EST
(In reply to comment #3)
> As for the init script, it would seem to be logical that it just be added to the
> list of types in netfs, or similar.

Looking at gfs2-utils, they seem to do the same thing as Ocfs2 (no mention of
Gfs2 in /etc/init.d/netfs either).

From /etc/init.d/gfs2:
  start)
        if [ -n "$GFS2FSTAB" ] 
        then
                action $"Mounting GFS2 filesystems: " mount -a -t gfs2
        fi
        touch /var/lock/subsys/gfs2
        ;;


Can we do it this way until netfs is fixed up to understand Ocfs2?
Comment 7 Rahul Sundaram 2008-02-01 21:26:59 EST
I can't sponsor you but a quick review nevertheless:
 
You can drop the Exclusiveos tag since this spec file is just for Fedora. 

Using a disttag is recommended

http://fedoraproject.org/wiki/Packaging/DistTag

You must drop all BuildRequires that are part of the default build root such as
bash. Refer

http://fedoraproject.org/wiki/Packaging/Guidelines#head-4cadce5e79d38a63cad3941de1dadc9d25d67d30-2

Why is there a Provides: ocfs2-tools-static = %{version}-%{release}? Can't you
dynamically link libraries? If not, you should provide a justification for
statically linking any libraries. 

Refer 

http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

The buildroot is non-standard. Use one of the alternatives specified in

http://fedoraproject.org/wiki/Packaging/Guidelines#head-b4fdd45fa76cbf54c885ef0836361319ab962473

/usr/share can be replaced by %{_datadir} and use %{_initrddir} instead of
/etc/init.d and so on to avoid hard-coding patches wherever applicable. 

Refer

http://fedoraproject.org/wiki/Packaging/RPMMacros

Python 2.5 path should not be hardcoded. You should use {python_sitelib} and
other guidelines outlined in

http://fedoraproject.org/wiki/Packaging/Python

Requires for ocfs2console and similar constructs should probably be {version} -
{release} instead of just {version}. 

Use make %{?_smp_mflags} if possible. 

You might want to branch to EPEL 5 when you get approved. 

http://fedoraproject.org/wiki/EPEL


  
Comment 8 Bill Nottingham 2008-02-04 12:30:18 EST
(In reply to comment #6)
> (In reply to comment #3)
> > As for the init script, it would seem to be logical that it just be added to the
> > list of types in netfs, or similar.
> 
> Looking at gfs2-utils, they seem to do the same thing as Ocfs2 (no mention of
> Gfs2 in /etc/init.d/netfs either).
> 
> From /etc/init.d/gfs2:
>   start)
>         if [ -n "$GFS2FSTAB" ] 
>         then
>                 action $"Mounting GFS2 filesystems: " mount -a -t gfs2
>         fi
>         touch /var/lock/subsys/gfs2
>         ;;
> 
> 
> Can we do it this way until netfs is fixed up to understand Ocfs2?
> 

Yes, it would just be nice to have a better infrastructure to do this rather
than a pile of init scripts. Something for the future...
Comment 9 Eric Sandeen 2008-02-04 14:21:07 EST
branching to EPEL is only useful if there is a RHEL5 ocfs2 module out there
somewhere... are there plans for that?
Comment 10 Rahul Sundaram 2008-02-04 15:03:20 EST
Ah yes, EPEL follows the same guidelines as Fedora and doesn't have external
kernel modules so forget I ever said that. 
Comment 11 Mark Fasheh 2008-02-04 18:07:30 EST
(In reply to comment #7)
> I can't sponsor you but a quick review nevertheless:

Thank you for the review - it has been very helpful and I've implemented the
vast majority of what you've pointed out:

I dropped the Exclusiveos tag and added a disttag. Also, I'm now using
%{_datadir}, %{_initrddir} and %{python_sitearch} whenever possible.

BuildRoot is now
"%{_tmppath}/%{name}-%{PACKAGE_VERSION}-%{PACKAGE_RELEASE}-root" which is one of
the approved alternates which you mentioned.

The requires line for ocfs2-tools-devel has been fixed - thanks for noticing that.


A new spec file / srpm has been uploaded and I'll attach the spec to this bug.
Since so much has changed, I bumped the release number:


Spec URL: http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools.spec
SRPM URL:
http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools-1.3.9-5.20080131git.fc8.src.rpm


The rest of my comments follow:

> You must drop all BuildRequires that are part of the default build root such as
> bash.

I think we're already clean with respect to buildrequires. There's certainly no
bash line in there - maybe you were looking at the requires instead?


> Why is there a Provides: ocfs2-tools-static = %{version}-%{release}? Can't you
> dynamically link libraries? If not, you should provide a justification for
> statically linking any libraries. 
> 
> Refer 
> 
>
http://fedoraproject.org/wiki/Packaging/Guidelines#head-2302ec1e1f44202c9cc4bcce24cb711266557ad7

The page you reference states:

"When a package only provides static libraries you can place all the files in
the *-devel subpackage. When doing this you also have to have a virtual Provide
for the static package:"

Ocfs2-tools is one such package, and I've followed the guidelines set out there.


> Use make %{?_smp_mflags} if possible. 

Unfortunately, ocfs2-tools doesn't support parallel builds at this time.

Thanks again!
Comment 12 Mark Fasheh 2008-02-04 18:09:02 EST
Created attachment 293957 [details]
Latest ocfs2-tools spec file.
Comment 13 Rahul Sundaram 2008-02-04 19:42:52 EST
I guess I misread the BuildRequires. 

Change the buildroot to

%{_tmppath}/%{name}-%{version}-%{release}-root

The guidelines recommend against mixing two styles 

http://fedoraproject.org/wiki/Packaging/Guidelines#head-f3d77b27a5d29dfc1f5600ef3fc836f2e317badf

The part of the guidelines, I wanted you to refer to regarding static libs was, 

"In general, packagers are strongly encouraged not to ship static libs unless a
compelling reason exists."

If you do have compelling reasons, it would be good to specify that in the
review. Other than those minor nit-picks, the latest spec looks ok to me. I did
a scratch build of the latest ocfs2 srpm and it builds and installs correctly.

http://koji.fedoraproject.org/koji/taskinfo?taskID=395837

A sponsor has to do a official review and approve your spec before you get
commit access.

http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored
Comment 14 Mark Fasheh 2008-02-05 15:43:04 EST
(In reply to comment #13)
> Change the buildroot to
> 
> %{_tmppath}/%{name}-%{version}-%{release}-root

Ahh, ok. Thanks - a new version of the package and spec have been uploaded:

Spec URL: http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools.spec
SRPM URL:
http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools-1.3.9-5.20080131git.fc8.src.rpm


> The part of the guidelines, I wanted you to refer to regarding static libs was, 
> 
> "In general, packagers are strongly encouraged not to ship static libs unless a
> compelling reason exists."
> 
> If you do have compelling reasons, it would be good to specify that in the
> review. 

Upstream doesn't build dynamic libraries because the ocfs2-tools api changes 
quite rapidly as file system features are added / expanded. As a result, it's
impossible to provide any sort of abi stability, so static libraries are
provided instead. Generally, people writing ocfs2 programs are encouraged to
submit their software to ocfs2-tools-devel@oss.oracle.com so that it can be
included in the upstream ocfs2-tools distribution. The libraries that are
provided in ocfs2-tools-devel are intended for interim development, and for a
small number of external programs including some fs test software.


> Other than those minor nit-picks, the latest spec looks ok to me. I did
> a scratch build of the latest ocfs2 srpm and it builds and installs correctly.

Thanks for testing!


> A sponsor has to do a official review and approve your spec before you get
> commit access.
> 
> http://fedoraproject.org/wiki/PackageMaintainers/HowToGetSponsored

Ok, I will add the FE-NEEDSPONSOR field to indicate my official request for
sponsorship.
Comment 15 Mark Fasheh 2008-02-05 15:45:33 EST
Created attachment 294042 [details]
Latest ocfs2-tools spec file.
Comment 16 Jarod Wilson 2008-02-20 15:39:32 EST
Eric asked me to take a look over the package as well, and I do have the power
to sponsor... Well, first, a few things w/the spec:

1) Rather than the compile_console define, I'd suggest:

%define with_console %{?_without_console: 0} %{?!_without_console: 1}

With that, you can do 'rpmbuild -bb --without console' to disable building the
console if you want.

2) the dist tag should be '%{?dist}'. The addition of the question mark lets the
package also build if for some reason dist isn't defined.

3) in the Requires/BuildRequires section, its not a mandate, but I personally
prefer to see things wrapped at 80 chars, just use multiple R/BR lines.

4) For anything with an initscript, the chkconfig R should be:

Requires(post): chkconfig
Requires(preun): chkconfig

You should also have this:

Requires(preun): /sbin/service

These are safeguards to make sure these bits are present when called from the
%post and %preun scripts, as they could otherwise be included in the same rpm
transaction, but not available when this  package needs 'em (slim chance of it
happening, but it can). More on the need for /sbin/service in a bit...

5) I'd avoid the extra define for "config_console". I'd just tweak %config based
on the value of %with_console.

6) don't use '-n ocfs2-tools-devel', just use 'devel'. The %{name}- gets
automagically pre-pended.

7) your -devel package includes a .pc file, therefore, the -devel package must
Requires: pkgconfig.

8) drop the '-n ocfs2-tools-%{version}' from the %setup line, that's what it
does by default.

9) the spec should have a comment included as to why _smp_mflags can't be used.

10) CFLAGS and/or CPPFLAGS aren't being respected. This one appears to require
some source patching to get the build to use the standard Fedora flags.

11) no need to redirect anything to /dev/null on chkconfig commands

12) in %preun, before you chkconfig --del the services, you need to make sure
they're stopped first. Thus the need for /sbin/service.

13) %defattr's should be (-,root,root,-)

14) generally, one should use %dir for including a directory in a package, with
a subsequent line or lines covering what files in the dir should be included.


I believe just about everything, save the Fedora CFLAGS/CPPFLAGS not being
respected, should be fixed by the spec diff I'll attach in a second...
Comment 17 Jarod Wilson 2008-02-20 15:41:23 EST
Created attachment 295445 [details]
Suggested spec changes in diff format
Comment 18 Mark Fasheh 2008-02-21 15:20:54 EST
Created attachment 295556 [details]
Latest ocfs2-tools spec file, with diff applied

Jarod, Thank you for your review and your changes to the spec. I have looked
them over and incorporated your patch into a new version of ocfs2-tools.spec,
which I will attach. In the meantime, I'll look into the source changes
required to honor the fedora build flags.
Comment 19 Mark Fasheh 2008-02-21 20:20:54 EST
Created attachment 295577 [details]
Latest ocfs2-tools spec file against latest ocfs2-tools git

The following two urls contain an srpm and spec file for my latest work.

http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools-1.3.9-6.20080221git.fc8.src.rpm

http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools.spec

Ocfs2-tools upstream git repository has been updated with patches to honor
user-set cflags. My tests show that the entire Fedora CFLAGS line is passed
through to the build process. The spec file and srpm uploaded reflect these new
changes. Thanks again for pointing this out - we actually considered it a bug
that user-set CFLAGS weren't being honored.
Comment 20 Jarod Wilson 2008-02-22 13:42:24 EST
Excellent, looking much better! I have two tiny little changes left for the spec:

1) for the sake of consistency, use either '%{name} = %{version}-%{release}' or
'ocfs2-tools = %{version}-%{release}'  in both ocfs2console and
ocfs2-tools-devel. Right now, one has one form, one has the other. (Actually my
fault, since my earlier patch changed one and not the other)

2) pull the bits about grepping for CFLAGS out of the spec. I assume they were
just there to verify they were getting passed through correctly.

Now for the full review checklist:

Fedora Package Review: ocfs2-tools
----------------------------------

MUST Items:

* rpmlint output acceptable (post full output w/waiver notes where needed):

ocfs2-tools.x86_64: W: service-default-enabled /etc/rc.d/init.d/ocfs2

This one is fine, per earlier discussion in the bug.

ocfs2-tools.x86_64: W: service-default-enabled /etc/rc.d/init.d/o2cb
ocfs2-tools.x86_64: E: subsys-not-used /etc/rc.d/init.d/o2cb

This script is a bit terrifying... Can you elaborate a bit on what this one does
and why it also needs to be on by default?

ocfs2-tools-devel.x86_64: W: no-documentation

Not a problem.


* Meets Package Naming Guidelines

* spec file name matches %{name}, in the format %{name}.spec

* The package meets the Packaging Guidelines

* open-source compatible license and meets fedora legal reqs (GPLv2)

* License field in spec matches actual license

* Includes text of license(s) in its own file, file in %doc

* spec file legible and in American English

* sources used match the upstream source, as provided in spec URL. Verify with
md5sum (if no upstream URL, source creation method must be documented and can be
verified using diff).

* produces binary rpms on at least one arch (tested f8/x86_64)

* No ExcludeArch

* BuildRequires are sane

* no locales

* no shared libs

* not relocatable

* package owns all directories it creates

* no duplicates in %files

* Permissions on %files sane

* %clean includes rm -rf %{buildroot}/$RPM_BUILD_ROOT

* macros used consistently

* package contains code

* No need for docs sub-package

* files in %doc aren't required for package to work

* Header files in -devel package

* Static libs present, explained, and packaged in accordance with the guidelines

* package Reqs: pkgconfig if pkgconfig(.pc) files present

* no versioned libs

* -devel packages requires base package NVR

* no libtool archives

* GUI app (ocfs2console), includes a %{name}.desktop file, installed with
desktop-file-install in the %install section 

* doesn't own files or folders other package own

* %install starts with rm -rf %{buildroot}/$RPM_BUILD_ROOT

* filenames in packages are valid UTF-8


SHOULD Items (not absolutely mandatory, but highly encouraged)

* package should build in mock: built in f8/x86_64 mock chroot

* package should build on all supported architectures: only tested x86_64 myself

* package should function as expected: untested by me, but I assume it does

* scriptlets are sane

* subpackages other than -devel require the base package using a fully versioned
dependency

* pkgconfig files in -devel pkg



So basically, just the two little things on the spec side, and a bit more
explanation of the o2cb initscript, and I'm prepared to approve the package and
do the whole sponsor thing. :)
Comment 21 Mark Fasheh 2008-02-22 16:38:11 EST
Fixed the spec file stuff - yeah the grep lines were a total mistake on my part
:) New versions of the spec and srpm can be found in the usual place:

http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools-1.3.9-6.20080221git.fc8.src.rpm

http://oss.oracle.com/~mfasheh/fedora/ocfs2-tools.spec


Regarding the init scripts, the Ocfs2 cluster services are enabled by default
because it's quite common for a user to have critical software loaded at boot
from an Ocfs2 file system and leaving the services off by default (thus making
their file systems unmountable) violates the principal of least surprise, and
could actually result in a loss of availability.
Comment 22 Mark Fasheh 2008-02-22 16:39:46 EST
Created attachment 295679 [details]
Latest ocfs2-tools spec file, with minor changes
Comment 23 Jarod Wilson 2008-02-22 17:01:07 EST
(In reply to comment #21)
> Regarding the init scripts, the Ocfs2 cluster services are enabled by default
> because it's quite common for a user to have critical software loaded at boot
> from an Ocfs2 file system and leaving the services off by default (thus making
> their file systems unmountable) violates the principal of least surprise, and
> could actually result in a loss of availability.

What I'm not clear on is what functionality is provided by each of the two
initscripts individually. I was thinking the ocfs2 initscript did the mounting
of ocfs2 file systems, but I'm not quite clear what the o2cb one does. Hrm, I
suppose there's probably some documentation on it somewhere in the tarball,
eh?... But if you could quickly explain what each does individually and why both
need to be on for all installs, I'd appreciate it and I'll probably just say
"oh, ok, approved." :)
Comment 24 Jarod Wilson 2008-02-22 17:07:01 EST
Okay, nevermind, I poked at the tarball more. So the various cluster services
are first started up by o2cb.init, and then the actual volumes are mounted by
ocfs2.init. And all nodes mounting the ocfs2 file system need to be running the
cluster services, they can't simply mount the volume w/o the services running.
Right?
Comment 25 Mark Fasheh 2008-02-22 17:29:37 EST
(In reply to comment #24)
> Okay, nevermind, I poked at the tarball more. So the various cluster services
> are first started up by o2cb.init, and then the actual volumes are mounted by
> ocfs2.init. And all nodes mounting the ocfs2 file system need to be running the
> cluster services, they can't simply mount the volume w/o the services running.
> Right?

Right - o2cb.init is for cluster services and ocfs2.init handles the file system.
Comment 26 Jarod Wilson 2008-02-25 15:16:53 EST
One last little thing I missed the first few times through the spec:

in the desktop-file-install bit, vendor should be set to "Fedora", since this
package will be in the Fedora repositories. Trivial fix, just make it before
building in koji, please.

Package APPROVED.

Now we gotta figure out the rest of the pieces of getting you sponsored... (I
always have to do some research for this part, don't do it that often).
Comment 27 Jarod Wilson 2008-02-25 15:23:20 EST
Looks like we're at "Get a Fedora Account" on
http://fedoraproject.org/wiki/PackageMaintainers/Join unless you already have one...
Comment 28 Jarod Wilson 2008-03-31 12:30:58 EDT
Mark, where are we at with getting this package into Fedora?
Comment 29 Mark Fasheh 2008-04-01 20:27:54 EDT
(In reply to comment #28)
> Mark, where are we at with getting this package into Fedora?

Sorry, I got hung up on the contributers agreement. I got approval to agree to
it today, so things should go much quicker from now.
Comment 30 Mark Fasheh 2008-04-16 14:41:32 EDT
Hey Jarod, I believe I followed the instructions we wrote about a couple weeks
ago, but haven't heard much since then. Is there anything I'm missing?
Comment 31 Jarod Wilson 2008-04-17 17:13:58 EDT
Oh, my apologies, I was unaware you were actually waiting on me now. One sec...
Okay, done. Import at will!
Comment 32 Mark Fasheh 2008-04-17 19:48:19 EDT
New Package CVS Request
=======================
Package Name: ocfs2-tools
Short Description: Tools for managing the Ocfs2 cluster file system
Owners: mfasheh
Branches: F-8
InitialCC: 
Cvsextras Commits: yes
Comment 33 Kevin Fenzi 2008-04-17 22:05:50 EDT
cvs done.
Comment 34 Fedora Update System 2008-04-22 18:37:55 EDT
ocfs2-tools-1.3.9-7.20080221git.fc8 has been pushed to the Fedora 8 testing repository.  If problems still persist, please make note of it in this bug report.
 If you want to test the update, you can install it with 
 su -c 'yum --enablerepo=updates-testing update ocfs2-tools'.  You can provide feedback for this update here: http://admin.fedoraproject.org/updates/F8/FEDORA-2008-3245
Comment 35 Fedora Update System 2008-04-29 17:01:40 EDT
ocfs2-tools-1.3.9-7.20080221git.fc8 has been pushed to the Fedora 8 stable repository.  If problems still persist, please make note of it in this bug report.

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