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.
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.
AutoReqProv: No <- why?
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.
(In reply to comment #2) > AutoReqProv: No <- why? It's completely unnecessary. A new spec file and srpm without AutoReqProv have been uploaded.
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.
(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?
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
(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...
branching to EPEL is only useful if there is a RHEL5 ocfs2 module out there somewhere... are there plans for that?
Ah yes, EPEL follows the same guidelines as Fedora and doesn't have external kernel modules so forget I ever said that.
(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!
Created attachment 293957 [details] Latest ocfs2-tools spec file.
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
(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.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.
Created attachment 294042 [details] Latest ocfs2-tools spec file.
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...
Created attachment 295445 [details] Suggested spec changes in diff format
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.
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.
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. :)
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.
Created attachment 295679 [details] Latest ocfs2-tools spec file, with minor changes
(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." :)
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?
(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.
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).
Looks like we're at "Get a Fedora Account" on http://fedoraproject.org/wiki/PackageMaintainers/Join unless you already have one...
Mark, where are we at with getting this package into Fedora?
(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.
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?
Oh, my apologies, I was unaware you were actually waiting on me now. One sec... Okay, done. Import at will!
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
cvs done.
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
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.