Spec URL: http://www.mwiriadi.id.au/fedora-spec/preload/preload.spec SRPM URL: http://www.mwiriadi.id.au/fedora-spec/preload/preload-0.4-1.fc8.src.rpm Description: preload runs as a daemon and gathers information about processes running on the system and shared-objects that they use. This information is saved in a file to keep across runs of preload
This is my first package and I need a sponsor.
The package builds in mock. [marc@strikeforce SRPMS]$ mock preload-0.4-1.fc8.src.rpm
Consider adding /lib64 into mapprefix.
Hey Marc. Do you have any other packages ready to submit, or is this one it for now? If not, can you do some pre-reviews of other waiting packages to show you know the guidelines? I'll go ahead and look at reviewing this package and sponsoring you, if you can do one of the above it would be great. Look for a review of this package in a while...
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+) 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: 9c98bc97ec261925c9a40e5084c9c149 preload-0.4.tar.gz 9c98bc97ec261925c9a40e5084c9c149 preload-0.4.tar.gz.1 OK - BuildRequires correct See below - 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 have dist tag OK - Should package latest version Issues: 1. For your Source0: line use the http://downloads.sourceforge.net/... url as mentioned in: http://fedoraproject.org/wiki/Packaging/SourceURL 2. Can you add %{?_smp_mflags} to your make line? If that causes build problems please add a comment that it's disabled for that reason. 3. Why the "%verify(not md5 size mtime)" on your config files? Also, why %ghost on the state and log file? 4. You need some Requires because you have an init script. Take a look at: http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-a6d7a1ed9d77dbb8d4af067378a79b838aebb20a 5. rpmlint says: preload.x86_64: E: non-standard-dir-perm /usr/share/doc/preload-0.4 0644 That dir should be 755... This is likely due to your "%defattr(-,root,root,0644)". That should be "%defattr(-,root,root,-)" preload.x86_64: W: service-default-enabled /etc/rc.d/init.d/preload preload.x86_64: W: service-default-enabled /etc/rc.d/init.d/preload It's discouraged to default enable a service. Do you think this one should be default enabled? 6. Since you have a logrotate file, should "Requires: logrotate" ? 7. Not a blocker, but something to note: Upstream hasn't released a new version since 2006. Is upstream dead? Are you willing to provide security fixes, etc?
> Issues: > > 1. For your Source0: line use the http://downloads.sourceforge.net/... url > as mentioned in: http://fedoraproject.org/wiki/Packaging/SourceURL Fixed this in the latest. > 2. Can you add %{?_smp_mflags} to your make line? > If that causes build problems please add a comment that it's disabled for > that reason. > Doesn't work added that to the changelog > 3. Why the "%verify(not md5 size mtime)" on your config files? > Also, why %ghost on the state and log file? The original person that made the spec had that in there so I left it. I have adjusted it to noreplace only. The ghost is to add the files and remove them on uninstall is that not correct? > > 4. You need some Requires because you have an init script. > Take a look at: > http://fedoraproject.org/wiki/Packaging/ScriptletSnippets?action=show&redirect=ScriptletSnippets#head-a6d7a1ed9d77dbb8d4af067378a79b838aebb20a Added in the latest version. > > 5. rpmlint says: > > preload.x86_64: E: non-standard-dir-perm /usr/share/doc/preload-0.4 0644 > > That dir should be 755... > This is likely due to your "%defattr(-,root,root,0644)". > That should be "%defattr(-,root,root,-)" Fixed in the latest version. > > preload.x86_64: W: service-default-enabled /etc/rc.d/init.d/preload > preload.x86_64: W: service-default-enabled /etc/rc.d/init.d/preload > > It's discouraged to default enable a service. > Do you think this one should be default enabled? > This is interesting I tested the latest version and rpmlint still generates the error however after installing the actual rpm it wasn't enabled? I would love feedback on how to fix this so rpmlint doesn't generate the error. > 6. Since you have a logrotate file, should "Requires: logrotate" ? Fixed in the latest version. > > 7. Not a blocker, but something to note: Upstream hasn't released a new > version since 2006. Is upstream dead? Are you willing to provide security > fixes, etc? I've emailed him based on the email from here and on sf.net so I'll wait for a reply. I would say that I wouldn't be able to provide security fixes since I don't know C if it was Java that would be fine but not C. Updated files http://mwiriadi.id.au/fedora-spec/preload/ I have other packages however I wanted to submit one at a time to fedora since dealing with multiple issues at the same time will confuse me.
Additional Console output to explain what I mean. [root@strikeforce i386]# rpmlint preload-0.4-2.fc8.i386.rpm preload.i386: W: service-default-enabled /etc/rc.d/init.d/preload preload.i386: W: service-default-enabled /etc/rc.d/init.d/preload [root@strikeforce i386]# rpm -i preload-0.4-2.fc8.i386.rpm [root@strikeforce i386]# service preload status preload is stopped [root@strikeforce i386]# chkconfig --list preload preload 0:off 1:off 2:off 3:off 4:off 5:off 6:off [root@strikeforce i386]# As I said I'm confused why this is the case based on rpmlint.
There are a couple of patches from debian's side I've contacted the debian maintainer to try and get hold of the patches. Some are enhancements other's are bug fixes so I'll be adding a patch to it.
Patching completed. One of the variables is verbose I have set it to 1 since it was writing to the /var/log/preload.log file every 30 seconds. Still have the same issue about service-default-enabled still unsure how to fix that. The patch has been explaining in the changelog. %ghost is still used since my understanding is that when the package is erased it will delete the file as explained previously. (correct me if I'm wrong.) It should work with /lib64 packages are located below. http://mwiriadi.id.au/fedora-spec/preload/ I have also heard from Behdad (upstream) I have advised him of this bug and he has advised me he will comment relating to your questions.
Hi, So, here's preload's story: - I did it as a Google Summer of Code for Fedora in Summer 2005 - Spent the year after writing a Masters thesis about it. Available here: http://www.cs.toronto.edu/~behdad/preload.pdf - Got a job with Red Hat's Desktop team after that So I've been just around the corner :-). Lemme first mention that preload is no magic bullet. In my measurements, any kind of readahead may slow down the boot process on some configurations. Nevertheless it's good to have it packaged and able to experiment with. I'm particularly curious to see if we can hook it to gdm to prefetch while waiting for login information. Over the past two years I've got a few preload patches, and found some more over the net. I'm not finished committing most of those. If you follow the mailing list you'll see: http://sourceforge.net/mailarchive/forum.php?forum_name=preload-devel The remaining issue is how to improve readahead.c. Some points: - I fixed a bug that was causing no sort being done at all, not even on file names. That should fix the main problem. - I've received a patch for request merging and parallel readahead using fork(). I'm not happy preload spawning hundreds of processes, so I need to adjust it a bit. - The Debian patch, that is in Marc's proposed package too, uses block address to sort, but it's incorrect as it always gets the block number of the first block. That's just plain wrong. Fortunately the fix is easy. So, what I'm doing on Sunday (busy tomorrow) is to implement: - Sort on block (requires CAP_SYS_RAWIO, aka root), fallback on inode, fallback on filename. - Request merging - Optionally parallel them. Probably turn it on by default, with a max fork limit set. What I've done so far that may be of interest to you: - Upstreamed most of the easy fixes. - Added /lib64 and /var/cache (to cover fontconfig caches) to mapprefix. - Added ionice to init script. - Dropped the upstream spec file. So, I'll be making a release, hopefully on Monday, with all the goodness. And yes, I'll be maintaining it.
Any progress on this review request?
Still awaiting a new version from upstream. When that has been done I will submit the updated package. My understanding was that it should have been completed by now however upstream has told me real life at Red Hat is busy :)
(Removing NEEDSPONSOR: bug 426733)
Upstream still has not released a new version would it be better to close this or grab the patches from upstream and release this 0.4 version with the patches?
You could spin up a package with the patches for now, and we can get it reviewed and in, and you can update to the new version once it's released? How does that sound?
I have built it for F8 with the patches but when I try to build it using mock this is the error I get. Spec file and SRPM: http://mwiriadi.fedorapeople.org/packages/preload/preload.spec http://mwiriadi.fedorapeople.org/packages/preload/preload-0.4-4.fc8.src.rpm In file included from /usr/include/glib-2.0/glib.h:74, from common.h:20, from cmdline.c:23: /usr/include/glib-2.0/glib/gtestutils.h:108: error: expected identifier or '(' before '/' token /usr/include/glib-2.0/glib/gtestutils.h:109: error: expected identifier or '(' before '/' token /usr/include/glib-2.0/glib/gtestutils.h:210: error: expected identifier before '/' token /usr/include/glib-2.0/glib/gtestutils.h:224: error: expected specifier-qualifier-list before '/' token make[2]: *** [cmdline.o] Error 1 make[2]: Leaving directory `/builddir/build/BUILD/preload-0.4/src' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/builddir/build/BUILD/preload-0.4' make: *** [all] Error 2 error: Bad exit status from /var/tmp/rpm-tmp.73504 (%build) I'm not to sure if thats an issue with preload or the devel package itself?
I spoke to Mathias and he said that was a glib2 error and that it should be fixed for devel this week or next week.
ok. Just let me know when that update is in, and I will get this formally reviewed...
The update is in and building it using mock is fine for devel now.
Can you post an updated spec/srpm?
http://mwiriadi.fedorapeople.org/packages/preload/preload-0.4-4.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/preload/preload.spec It's the same spec and SRPM as previously since the glib2 error had nothing to do with preload in the sense that it wasn't preload's fault.
a few issues: 1. You don't seem to be applying all the patches, just patch0? Any reason for that? 2. You seem to have <script> in a few places, where it should be the name of the init script (preload). 3. You have a macro in your changelog. You need to escape it with %% there to prevent rpm from expanding it. 4. I am seeing it set to on by default in the init script. You would need to patch the preload.init.in file to prevent that... In particular the # chkconfig: 2345 05 95 and # Default-Start: 2 3 4 5 lines. Can you fix up those things?
That is why I shouldn't package when it's late at night and I'm tired. :( http://mwiriadi.fedorapeople.org/packages/preload/preload-0.4-5.fc8.src.rpm http://mwiriadi.fedorapeople.org/packages/preload/preload.spec
That looks much better. I see no further issues... this package is APPROVED.
New Package CVS Request ======================= Package Name: preload Short Description: an adaptive readahead daemon Owners: mwiriadi Branches: F-8, devel InitialCC: mwiriadi Cvsextras Commits: yes
cvs done.
Package Change Request ====================== Package Name: preload New Branches: F-7