Bug 225237 - Merge Review: acpid
Merge Review: acpid
Status: CLOSED RAWHIDE
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Kevin Fenzi
Fedora Package Reviews List
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2007-01-29 15:59 EST by Nobody's working on this, feel free to take it
Modified: 2007-11-30 17:11 EST (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2007-03-02 08:34:01 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
kevin: fedora‑review+


Attachments (Terms of Use)

  None (edit)
Description Nobody's working on this, feel free to take it 2007-01-29 15:59:14 EST
Fedora Merge Review: acpid

http://cvs.fedora.redhat.com/viewcvs/devel/acpid/
Comment 1 Kevin Fenzi 2007-02-03 12:11:34 EST
I'll review this package. 
Comment 2 Kevin Fenzi 2007-02-03 12:49:06 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 (GPL)
OK - License field in spec matches
See below - License file included in package
OK - Spec in American English
OK - Spec is legible.
OK - Sources match upstream md5sum:
3aff94e92186e99ed5fd6dcee2db7c74  acpid-1.0.4.tar.gz
3aff94e92186e99ed5fd6dcee2db7c74  acpid-1.0.4.tar.gz.1
OK - Package needs ExcludeArch
OK - BuildRequires correct
OK - Package has %defattr and permissions on files is good.
OK - Package has a correct %clean section.
See below - Package has correct buildroot
OK - Package is code or permissible content.

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 sane scriptlets.
See below - Should have dist tag
OK - Should package latest version
10 bugs - check for outstanding bugs on package.

Issues:

1. Buildroot should be changed to standard. Should add smp_mflags?

2. Might include COPYING, README, Changelog, TODO as doc files?

3. rpmlint our pal says:

rpmlint on ./acpid-1.0.4-5.i386.rpm
W: acpid conffile-without-noreplace-flag /etc/acpi/events/power.conf
W: acpid conffile-without-noreplace-flag /etc/acpi/events/video.conf
W: acpid conffile-without-noreplace-flag /etc/logrotate.d/acpid

Should all be noreplace?

E: acpid non-readable /usr/sbin/acpid 0750
E: acpid non-standard-executable-perm /usr/sbin/acpid 0750

Should this really be non readable by anyone? Why?
If so, perhaps a rpmlint bug should be filed?

W: acpid service-default-enabled /etc/rc.d/init.d/acpid
Should this really be enabled on all machines?
Are there cases where it might not be desired by default?

rpmlint on ./acpid-1.0.4-5.src.rpm
W: acpid strange-permission acpid.init 0755
W: acpid prereq-use /sbin/chkconfig, /sbin/service

Should perhaps be:

Requires(post): /sbin/chkconfig
Requires(preun): /sbin/chkconfig
Requires(preun): /sbin/service

See:
http://www.fedoraproject.org/wiki/Packaging/ScriptletSnippets#head-a6d7a1ed9d77dbb8d4af067378a79b838aebb20a

W: acpid setup-not-quiet

Should add -q to setup.

4. In the files section:

%verify(not md5 size mtime) %ghost %config(missingok,noreplace) /var/log/acpid

Why all this?

/usr/bin/acpi_listen
/usr/sbin/acpid

Should those have %{_bindir} and %{_sbindir} ?

/usr/share/man/man8/acpid.8.gz
/usr/share/man/man8/acpi_listen.8.gz

Should have %{_mandir} ?

5. You might look at the outstanding bugs on this package.
In particular the bugs asking for better scripts might stand to have a
response like "please submit your outstanding scripts for inclusion"
Comment 3 Phil Knirsch 2007-02-07 06:52:15 EST
1: Fixed Buildroot, add smp_flags. Also added the %{?dist} to the Release
2: Included doc files
3.1: Checked, all 3 should really be noreplace, fixed.
3.2: acpid can't be run as anything else but root, so there is no point in it
being executable or readable by everyone.
3.3: acpid isn't a network/remote service, so enabling it by default is safe.
Also even if a machine doesn't have ACPI (nowadays kinda rare, but still),
making it default will be the best choice the the majority of systems.
3.4: Why is the premission for an initscript strange with 0755, even in a srpm?
But fixed the PreReqs to use the correct modern style.
3.5: Added -q for %setup
4.1: Typical /var/log/FOO entry. The package itself doesn't package a real file,
but "supports" one being there. See other /var/log logfile supporting packages.
4.2: Fixed. Also fixed all the /etc occurences with %{_sysconfdir} and
/usr/share with %{_datadir}
4.3: Fixed.

Thanks for the review,

Read ya, Phil
Comment 4 Kevin Fenzi 2007-02-07 11:58:49 EST
Thanks for looking at those items... 
I am reassigning this back to me. I will hopefully be able to take a 
look later today. 
Comment 5 Kevin Fenzi 2007-02-07 21:01:48 EST
Thanks for the quick fixes and reply... 

1. good. ok. 

2. good. ok. 

3.1 good. ok. 

3.2 I suppose so. ok. 

3.3 good. ok. 

3.4 Not sure why it's complaining about that 0755 init script. ;( 
good on the prereqs. 

3.5 good. ok. 

4.1 Humm...I haven't seen that used for /var/log/FOO files before... I guess
there aren't many packages that make such files. Looking at for example
vixie-cron, it just has no mention of the log file at all. 
I guess the question is: do we want this package to remove the /var/log/acpid
log when the package is removed? Or should it linger around?
We should probibly decide what behavior is desired and make all the packages
that create /var/log files do the same thing... 

In fact I am hard pressed to find another package that does things the way
this one does. Do you have any example? scrollkeeper appears to ghost the log
file, but yum, x.org, vixie-cron just don't claim anything for their log file
(ie, it's unowned). 

4.2 good. ok. 

4.3 good. ok. 

Setting it back to you to comment on the 4.1 issue. 
When you do, please assign it back to me and change the fedora-review flag back
to ? 

Thanks again for the quick fixes... 
Comment 6 Phil Knirsch 2007-02-08 07:05:54 EST
Hm. I've looked at other packages with logfiles in /var/log, and several
actually  owned those files:

pam: %ghost %verify(not md5 size mtime) /var/log/faillog
scrollkeeker: %ghost %{_localstatedir}/log/scrollkeeper.log
setup: %ghost %attr(0644,root,root) %verify(not md5 size mtime) /var/log/lastlog

So there are a few other examples where the logfile is at least being referenced
by a package using a %ghost entry.

I mean, it boils basically down to what files a package should own. And
typically (and historically) those consisted of the files that were clearly and
distinctly connected to that package. There could be the packaged files or, like
e.g. for /var/cache files created during the lifetime of a package on a system.
And imo /var/log/acpid is pretty clearly connected to one specific package,
namely acpid. ;)

Maybe this point should really be brought up at the next Fedora meeting to see
what the opinion of others are on it. The above is just my personal view after
maintaining packages for quite some time, but i'd have no problem if the general
rule would be to keep /var/log files unreferenced.

Read ya, Phil
Comment 7 Kevin Fenzi 2007-02-13 14:19:07 EST
Hey Phil. 

Did we come to any conclusion based on the thread on Fedora Maintainers?
Or would you like me to try and get the packaging comittee to come up with a 
guideline for this case?

Personally, I think this package should not have any ownership of the log file.
Since logrotate will leave other versions of the log around, something is going
to be lingering on package removal anyhow. I guess a case could be made for
making a /var/log/acpid/ directory and putting logs in there, as that would make
selinux labeling easier, but thats all I can think of. 

Your thoughts?
Comment 8 Phil Knirsch 2007-02-14 07:55:34 EST
Hi Kevin.

After the discussion on f-m list i completely agree with your opinion that the
logfile shouldn't be owned at all, partially because of the logrotate reasons
but also because it doesn't really make sense if a lot of packages don't own the
logfiles because of undecidable situtions and just a few do.

It would and should be different with directories in /var/log/, but acpid
doesn't have one.

I'll rebuild the package without the ownership of the logfile today.

That would finish the review then iirc, right? :)

And thanks again for the good review, work and the really helpful comments. This
way we can improve the review guidelines as well, so everyone can benefit from it.

Read ya, Phil
Comment 9 Kevin Fenzi 2007-02-14 12:52:18 EST
In reply to comment #8: 

> I'll rebuild the package without the ownership of the logfile today.

Sounds good. 

> That would finish the review then iirc, right? :)

Yeah, thats the last blocking issue I see. 

>And thanks again for the good review, work and the really helpful comments.
>This way we can improve the review guidelines as well, so everyone can benefit 
>from it.

Thank you for quick fixes and good replies. :)
Comment 10 Kevin Fenzi 2007-02-23 21:49:53 EST
Sorry I didn't get back to this until now. 
I see you made the changes for the log file, so all looks good... 

This package is APPROVED. 
Per the new offical review process, I will leave it assigned to me with the
review flag as + (approved). Feel free to close it RAWHIDE now since it's been
pushed out... 
Comment 11 Phil Knirsch 2007-03-02 08:34:01 EST
Ok, closing it as RAWHIDE.

Read ya, Phil


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