This service will be undergoing maintenance at 00:00 UTC, 2016-09-28. It is expected to last about 1 hours
Bug 333491 - Review Request: preload - Preload is an adaptive readahead daemon
Review Request: preload - Preload is an adaptive readahead daemon
Status: CLOSED NEXTRELEASE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
low Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-10-16 00:34 EDT by Marc Wiriadisastra
Modified: 2008-01-20 00:21 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-01-20 00:21:10 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
kevin: fedora‑review+
kevin: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Marc Wiriadisastra 2007-10-16 00:34:26 EDT
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
Comment 1 Marc Wiriadisastra 2007-10-18 19:10:09 EDT
This is my first package and I need a sponsor.
Comment 2 Marc Wiriadisastra 2007-11-03 05:27:14 EDT
The package builds in mock.
[marc@strikeforce SRPMS]$ mock preload-0.4-1.fc8.src.rpm 

Comment 3 Adam Goode 2007-11-07 20:09:39 EST
Consider adding /lib64 into mapprefix.
Comment 4 Kevin Fenzi 2007-11-07 20:24:58 EST
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... 
Comment 5 Kevin Fenzi 2007-11-07 20:42:57 EST
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?
Comment 6 Marc Wiriadisastra 2007-11-08 06:37:47 EST
> 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.
Comment 7 Marc Wiriadisastra 2007-11-08 06:41:38 EST
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.
Comment 8 Marc Wiriadisastra 2007-11-08 07:03:57 EST
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.
Comment 9 Marc Wiriadisastra 2007-11-09 03:37:28 EST
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.
Comment 10 Behdad Esfahbod 2007-11-10 03:32:25 EST
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.
Comment 11 Kevin Fenzi 2007-11-30 23:15:12 EST
Any progress on this review request?
Comment 12 Marc Wiriadisastra 2007-12-02 04:24:42 EST
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 :)
Comment 13 Mamoru TASAKA 2008-01-02 20:41:53 EST
(Removing NEEDSPONSOR: bug 426733)
Comment 14 Marc Wiriadisastra 2008-01-05 12:34:15 EST
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?
Comment 15 Kevin Fenzi 2008-01-05 17:10:42 EST
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?
Comment 16 Marc Wiriadisastra 2008-01-05 20:34:33 EST
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?
Comment 17 Marc Wiriadisastra 2008-01-08 09:15:12 EST
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.
Comment 18 Kevin Fenzi 2008-01-08 12:06:33 EST
ok. Just let me know when that update is in, and I will get this formally
reviewed... 
Comment 19 Marc Wiriadisastra 2008-01-10 16:33:36 EST
The update is in and building it using mock is fine for devel now.

Comment 20 Kevin Fenzi 2008-01-11 13:03:09 EST
Can you post an updated spec/srpm?
Comment 21 Marc Wiriadisastra 2008-01-11 18:41:27 EST
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.
Comment 22 Kevin Fenzi 2008-01-15 23:59:57 EST
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? 
Comment 23 Marc Wiriadisastra 2008-01-16 05:31:12 EST
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
Comment 24 Kevin Fenzi 2008-01-16 22:20:34 EST
That looks much better. I see no further issues... this package is APPROVED. 
Comment 25 Marc Wiriadisastra 2008-01-17 05:41:55 EST
New Package CVS Request
=======================
Package Name: preload
Short Description: an adaptive readahead daemon
Owners: mwiriadi
Branches: F-8, devel
InitialCC: mwiriadi
Cvsextras Commits: yes
Comment 26 Kevin Fenzi 2008-01-17 12:08:51 EST
cvs done.
Comment 27 Marc Wiriadisastra 2008-01-17 16:44:06 EST
Package Change Request
======================
Package Name: preload
New Branches: F-7
Comment 28 Kevin Fenzi 2008-01-17 22:32:45 EST
cvs done.

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